Closed RachithP closed 7 months ago
@RachithP
navmesh_env.py
to navmesh.py
to match the name of the class in the file. In my previous comment, I didn't mean that we must name all files that provide higher-level functionality as *_env.py
. I just meant that we should cleanly separate standalone functions in _utils.py
files, and higher-level functionality in other files.cpp
files. If something doesn't need to be in a header, it shouldn't be. We can make exceptions for extremely simple one-liner functions, if putting them in a header would make it so we don't need a cpp
file at all. But the service implementations don't meet that criteria.@RachithP
- Rename
navmesh_env.py
tonavmesh.py
to match the name of the class in the file. In my previous comment, I didn't mean that we must name all files that provide higher-level functionality as*_env.py
. I just meant that we should cleanly separate standalone functions in_utils.py
files, and higher-level functionality in other files.
Made these changes in https://github.com/isl-org/spear/pull/279.
- Move all service functionality that doesn't need to be in a header into
cpp
files. If something doesn't need to be in a header, it shouldn't be. We can make exceptions for extremely simple one-liner functions, if putting them in a header would make it so we don't need acpp
file at all. But the service implementations don't meet that criteria.
Since our EngineService
class is templated, we can't move it to a cpp
file. However, only constructors of other service classes are templated, so we can move their member functions to a cpp
file. Made these changes in https://github.com/isl-org/spear/pull/279 .
Note that to maintain symmetry with the constructors, I kept the destructors in the header file.
Note that to maintain symmetry with the constructors, I kept the destructors in the header file.
Great choice. It's nice to see the constructor next to the destructor, even if we could move the destructor to a cpp file.
Please address outstanding comments.