feedback on the current state of my engine

[hollow token]

I need some feedback on the current state of my engine before I continue its development to make sure I'm not writing spaghetti code and have to rewrite everything every time I add something.
here's the link: https://github.com/rickisgone/gears-game-engine
if you can pick it apart as much as you can, I'm still learning and having an objective review would be helpful for improving my knowledge of the language

[lunar quail]

Feedback for the Application alone:

• Why class Application owns other classes like Window?

What's the reason it owns… a window? And why only one? And exactly one?

A game engine should manage windows, but a single window, especially directly inside the Application, always present, is definitely misplaced.
Most likely you'll want to use a window manager. For some automated tests, you won't want windows at all, and for some games you will want multiple windows.

• Why class Application has virtual dtor?

Is there any reason you want to pay the cost of abstraction? What do you want to use the abstraction for?
It looks like you've learned some ancient C++ tutorials and you are stuck using virtual functions, but have you ever asked yourself why are you using them?

• Why class Application is not final?

Do you plan to make multiple Application versions with the same base?
Why do you need multiple different applications inside your game engine?
Have you considered programming multithreaded application or using multiple programs managed by another one?

• Why do you use headers in such a huge project as game engine?

Why don't you reduce compile times by using modules?
What's the reason you're copy‐pasting headers using preprocessor?
It's likely yet another huge issue related to previous feedback.

​

​

• Why Application ctor has parameters such as: width, height, windowName?

What's common between them and the application itself?
It seems it's echo of the wrong ownership. This approach will lead to passing random arguments all over the codebase, and eventually lead to std::moveing huge amount of data, copying or even costly casting in order to read, creating huge and redundant mess in your ever‐growing codebase.

• Why Application::run is pure virtual?

What's the reason you require inheritors to implement it?
Why it has no default implementation?
It's likely related to previous feedback.

• What's Application::run?

Why it's not documented?
When you ask for feedback, the code should be at least documented.

What does it do? Does it starts an application? Which one?
If so, can the application not be started? If not, why does the run ever exists instead of Application's ctor?

• Why you've explicitly deleted copy special member functions, but kept other smfs implicit?

What's your purpose here? Do you plan to change Application's ownership? If so, why?
It seems you should likely stick with the rule of three/five/zero.

• Why width and height arguments are signed?

Do you plan using negative values? All of them?
You're likely using wrong type as size can't be negative.
Finally, why are you using ambigous int?

​

[hollow token]

damn

that's a lot for the application alone

yep

I respect that

[hollow token]

txd dude, I'll be fixing as much as I can asap

[hollow token]

also, regarding modules, the intellisense for vscode doesn't support them yet, so I'm deciding whether I want to be looking at files filled with errors until they make it work or not, also I'm not certain of what I should make into a module and what I should keep as a header + implementation file

cause I was actually thinking about switching to modules

[hollow token]

https://pastebin.com/mUZgEa7J im trying to switch to modules and the file should be right tho im getting lots of errors

file(GLOB MODULES "engine/*.cppm")
target_sources(Gears PUBLIC FILE_SET Gears_modules TYPE CXX_MODULES FILES
   ${MODULES}
)
```and im also making sure to be compiling the modules correctly

[hollow token]

the errors:

https://pastebin.com/BWzdC8yB

[lunar quail]

As a rule of thumb, look at the first error message only.
However, at this point you're asking for help, not feedback.

[junior raptor]

I'm under impression modules aren't really production-ready yet. I wouldn't call not using them a mistake

[junior raptor]

Why you've explicitly deleted copy special member functions, but kept other smfs implicit?
It automatically deletes the move functions too, there's no need to do that manually.
Why width and height arguments are signed?
Making them unsigned doesn't help you guard against negative values. They'll just silently overflow and you'll end up with the same issue.

[hollow token]

kinda

iirc they only work decently with clang and MSVC currently

[hollow token]

also, the glfw functions take int as the window measure

that's why I put them as int and not uint32_t

[hollow token]

tho the vulkan functions take uint32_t as parameters

[hollow token]

and even there they are kinda buggy

[lunar quail]

I'm using modules with GCC and MSVC on a daily basis. Some exotic features like partitions and pmfs are not working on GCC, but i'm far way from calling it not production ready. It's thwarting people still refuse it, rationalizing against using questionable arguments. But some people still use C, so I'm not surprised. Yet calling it not production ready is far from factual.

[lunar quail]

I've asked about all smfs and it was a rhetoric question ※ C.21

It automatically deletes the move functions too, there's no need to do that manually.
That's not true. They won't be implicitly created, but won't be deleted ※ Howard Hinnant's Table

Finally, size can't be negative. Using signed for size is irrational. The fact some API uses wrong type is not a justification for using it too. Using wrong types confuses end‐users and makes code unclear and hard to read and understand.

[hollow token]

either way

I made the window measure uint32_t

tho with the modules I'm still getting weird errors

and I'm trying to switch from the Application class from owning a Window to having a std::vector of Window*

tho I'll have to refactor a lot of code

[lunar quail]

It's always better to do so early — less work in the future.

[junior raptor]

If we're linking core guidelines, here's another one 😛
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-nonnegative

They're not infallible though, there's some BS in them. (E.g. they recommend brace initialization everywhere... Maybe you don't disagree with that, but I'm sure you'll find things you disagree with.)

[junior raptor]

They won't be implicitly created, but won't be deleted
Well, fair, but my point still stands. Deleting copy operations is enough to make the class non-copyable and non-movable

[lunar quail]

That's unrelated. We're not avoiding negative values as we can't have them on size (well, as long as the value is correct).

[junior raptor]

I believe that's exactly what it advises against, but 🤷‍♂️

I guess I'm just pointing out that most of this feedback is opinionated.

[lunar quail]

The code logic should prevent overflows, not the type itself.
What ES.106 tells is that unsigned alone won't prevent negative values.
It's hard not to have opinionated feedback against signed size, yet I believe this aspect is exhaused.

[hollow token]

I've switched what i thought should've been a module to one

[bright tulip]

are you making pullrequests on your own project each time?

[hollow token]

im pushing on dev for when the change doesnt work yet and i have to work on it on another pc

tho im currently on the same pc so thats kinda useless

[hollow token]

I've basically fixed everything apart from the window ownership, which I'm still rewriting to be a std::vector<Window*>

and probably the ctor part

[hollow token]

nvm, I switched to writing a WM class and I'll just make it own that

[hollow token]

ok i added formatting to the logger

and im now working on the WM class

[gray hamlet]

what it s the scope of the project?

[hollow token]

make a functioning game engine