-
-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Code review #1
Comments
Thank you for your feedbacks and suggestions, mate! I'll try to fix every single issue you have pointed out in the next tutorials. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
In
OpenGL-3D-Game-Tutorial-Series/Tutorial8_EntitySystem_Basics/Game/MyGame.cpp
Line 41 in 4d38177
not missing calling OEntitySystem::update ?
In
OpenGL-3D-Game-Tutorial-Series/Tutorial8_EntitySystem_Basics/OGL3D/source/OGL3D/Entity/OEntitySystem.cpp
Line 41 in 4d38177
I find sad to set each field this way, this is error-prone (missing one, i.e.) better to use the constructor to pass these parameters: the compiler will warn you in case of missing one.
In
OpenGL-3D-Game-Tutorial-Series/Tutorial8_EntitySystem_Basics/OGL3D/source/OGL3D/Entity/OEntitySystem.cpp
Line 54 in 4d38177
You use m_entitiesToDestroy for not interfering deleting elements when iterating on them, but why not for createEntityInternal ? I guess you can create entities during the entity->onUpdate ? I guess a fake solution is to use mutex because creating entities at run-time will create a dead lock. But point to take into account when threading your game. Another solution would be unordered_map (confer section Iterator validity https://cplusplus.com/reference/unordered_map/unordered_map/erase/ vs https://cplusplus.com/reference/map/map/erase/)
Risk of recursivity
OpenGL-3D-Game-Tutorial-Series/Tutorial8_EntitySystem_Basics/OGL3D/source/OGL3D/Entity/OEntitySystem.cpp
Line 44 in 4d38177
In
OpenGL-3D-Game-Tutorial-Series/Tutorial8_EntitySystem_Basics/OGL3D/source/OGL3D/Entity/OEntity.cpp
Line 32 in 4d38177
In
OpenGL-3D-Game-Tutorial-Series/Tutorial8_EntitySystem_Basics/OGL3D/include/OGL3D/Entity/OEntity.h
Line 44 in 4d38177
The text was updated successfully, but these errors were encountered: