m1maker / NGT

New open source game toolkit
https://ngtcode.dev/
20 stars 11 forks source link

Urgent: tasks that MUST be done #34

Open ethindp opened 4 months ago

ethindp commented 4 months ago

So, a few things need to be done, ASAP, before you go adding anything else:

  1. Get rid of all the DLLs/libs/binaries in the repository. This is a source code repo, not a source code/binary repository.
  2. Switch to using CMake. Seriously. You can use a starter template like this one to get going. For dependency management, use something like vcpkg or conan. If you need dependencies that aren't in the vcpkg or conan repositories, use something like cpm.cmake.
  3. Switch to using non-windows-specific libraries and instead use platform abstractions. If you need UI support, either use a library like wx or qt directly, or, if you want a bit of abstraction, try something like xtd.
  4. Stop putting everything into one file. This is incredibly bad practice and makes your code very difficult to work with. One way is to put each class in it's own file. Another way is to categorize classes (e.g. one file for audio, one file for IO, one file for crypto, etc.).
  5. Stop using both OpenAL and Miniaudio together. Pick one or the other. When it comes to audio playback and basic audio handling, both can do the exact same thing. Using both simultaneously not only complicates your code but risks problems later when you port to other platforms because not all platforms handle audio IO the same way, and a platform may open a device exclusively or something. If you want HRTF or whatnot, use OpenAL, FMOD, or use SteamAudio alongside Miniaudio (or use something like LabSound).
  6. Implement error checking. Out of all of the above points, this is by far one of the most important. Right now, glancing through the code reveals a ton of function calls to C APIs (yes, C, not C++) that return error results, but you don't verify those results to confirm that the call your making succeeds. Not every library you ever use in C++ will throw exceptions. Do not assume that a library will throw an exception unless this is explicitly documented or otherwise obvious, and if your working with C libraries, always assume that the result you get back can be either (1) NULL, (2) an error value, or (3) the value you want, unless the function signature explicitly indicates that no result is returned. If the library uses out parameters, always check those out parameters to ensure that they aren't NULL or otherwise undefined.

All of this needs to be done. As it currently stands, this repository is an utter mess, and so is the code, and if you want this to get off the ground then it needs to be maintainable, easy to work with, reusable, and well-organized. Using CMake and the template above to get you going is going to go a long way towards that goal. Using platform abstractions will dramatically reduce the amount of code you need to write, and will make your project a lot easier to understand because your intent will be far more obvious. You should only use raw platform APIs if you have no other option. If you do use raw platform APIs, abstract them so that your code is reusable and people who want to use them don't need to worry about how it works under the hood.

ethindp commented 4 months ago

Oh, also, one last suggestion: run a static analyzer over your entire project. Something like clang-tidy will do (once you've done all the above steps). Clang-tidy isn't perfect, and some of what it recommends might be redundant or not useful, but it will give you tips on modernizing your code and preventing vulnerabilities.

brightening-eyes commented 1 month ago

hello, in addition, dependabot can also help to avoid many of the errors also in code, there are some libraries like poco's library and so on. if someone wants to compile the code with another compiler (lets say clang over msvc), they might get into abi incompatibilities, and/or crashes, or rebuild the libraries themselves. having cmake and making ngt depend on those libs will solve this problem. also beside vcpkg submodules can be considered. the same is true with angelscript, openssl and so on and so forth going to encryption side of things, libtomcrypt can be used instead of openssl. it is way way smaller in size. about calling c libraries, the same is true with some of the C++ stuff. (in bytecode stream reader memcpy is used with std::vector together). although it works, but using only c functions or c++ ones makes the code more readable. instead of std::vector you can use char* to read/write bytecodes with memcpy, realloc etc, or with std::vector you need to use std::copy instead of memcpy. this was for readability. about cmake, I cant agree more with @ethindp using it will give lots of advantages. namely, more generators like Ninja, better compiler support for different types of builds (for example CMAKE_BUILD_TYPE=Release will give -o3 on clang to make the code faster etc) for screen reader support, I suppose universal speech seems better (no lgpl stuff). about ngt.cpp stuff, look at the code. its more than 1000 lines (which is not an issue by itself) but when it comes to different functions, those are not structured at all. sdl functions (like window management etc), engine message callback and so on are in this file. somehow structuring them will make everything better. coming to main.cpp, for each flag asIScriptEngine and registration of things are done multiple times. as the engine needs to be initialized (both for compilation and running, it is better to call them once). this will reduce the size of the output binary as well.

ethindp commented 1 month ago

@brightening-eyes Honestly I don't think the maintainer of this project cares all that much about our suggestions. He still hasn't done the majority of what others and I recommended, and though he's sort-of organized the repository, there's still a bunch of stuff that shouldn't be there. And the more functionality that's added, the harder that reorganizing this and restructuring it is going to become.

brightening-eyes commented 1 month ago

another big thing is exception handling suppose in script someone does something wrong and C++ gives an exception. if those are not handled, the app crashes in the worst case. runtime errors are one of those things. in each and every function that may throw C++ exceptions, you should call in try/catch (and then forward the exception to the engine to handle). another issue is that winforms.cpp is there, but I havent seen its usage anywhere. (dont worry, the linker gets rid of it but something to keep in mind when maintaining the code). and as the window management is handled by sdl, I don't get why this is here. when watching the code again, the keycodes should be handled as enums, not a typedef to int. this is because the engine can check for the values and if they are out of range, instead of sdl, the engine will manage it and errors about it. @ethindp there are a lot to do, but I suppose implementing features is the concern for the maintainer. because there are lots and lots of vcxproj stuff there like backups and remotes (which is not needed even and should be placed in .gitignore) I also found some object files there (the code should be compiled without them but I do not get why they are here).

ethindp commented 1 month ago

I mean, adding features can come later, @m1maker needs to focus on fixing the bugs and cleaning up stuff first. They shouldn't be in any rush to make something as fast as possible. That's a horrible idea and we all know where that goes.