Support ptex mesh - step 3: better naming #196
Conversation
-) rename the buffers on GPU side in PTexMeshData; -) rename variables in PTexMeshDrawable;
facebook-github-bot
added
the
CLA Signed
|
This PR is ready to be reviewed. Thanks. |
|
By the way, please take a look at the CI failure. It says |
src/esp/assets/PTexMeshData.cpp
Outdated
| @@ -25,8 +25,6 @@ | |||
| static constexpr int ROTATION_SHIFT = 30; | |||
| static constexpr int FACE_MASK = 0x3FFFFFFF; | |||
|
|
|||
| namespace Cr = Corrade; | |||
There was a problem hiding this comment.
What is the rationale for getting rid of the shorter namespace alias? We are using this pervasively in other parts of the code (e.g., ResourceManager) and I don't think readability is impacted.
There was a problem hiding this comment.
I understand it may save you a couple of characters when coding. But I actually think it does decrease the readability. When I started this project and read this file, I actually searched what the "Cr" was.
Yes, it is used in a couple of files. But I see more places are using Corrade:: directly. And I prefer this way since it is straightforward.
There was a problem hiding this comment.
grep -rl 'Cr = Corrade' src/esp | wc shows 7 cpp files for me. In my opinion, namespace aliasing in cpp files is quite reasonable, anyone interested in looking at implementation detail will quickly see the alias at the top of the file and understand. @mosra @erikwijmans @msbaines do you have opinions about this?
There was a problem hiding this comment.
Also, I would say this change (removing a namespace alias) is orthogonal to the "member name improvements" in this PR. If we make a decision one way or the other, I would still advocate namespace alias changes be done in a separate PR regardless.
There was a problem hiding this comment.
No preference from me. Cor<tab> vs Cr<tab> are about the same length. I can both see an argument for increasing readability by using Corrade and increasing readability by reducing the length of names (Cr::Utility::Directory::MapDeleter seems better than Corrade::Utility::Directory::MapDeleter, for instance).
There was a problem hiding this comment.
I think @mosra's suggestion is very reasonable to me.
What is most important in cases like this is to have:
- Consistent and agreed upon conventions of referring to modules. This should be documented somewhere so the convention is clear and that the issue is not revisited.
- No arbitrary changing of convention without discussion so that code can remain consistent
- Small to the point PRs that are as close to "one change" as possible so that PRs do not feel like an overhead.
There was a problem hiding this comment.
This should be documented somewhere
👍 Someone should be taking notes on what to populate a Coding Style document with :)
I would advocate against making namespace alias changes as part of this PR.
Definitely. Even if you (or anybody else as a PR author) don't like the conventions used, changing them only adds extra lines to the diff, extra overhead for each reviewer.
If done separately in a dedicated PR instead, this change could be simply a global replace of Magnum:: with Mn::, magnum. with mn. and then adding namespace Mn = Magnum or import magnum as mn (and removing using namespace Magnum; if it's still somewhere) until everything compiles again and tests pass. Then, if it's really just that, with no other changes requiring special attention, such a PR would be extremely easy to review.
There was a problem hiding this comment.
Do we have an existing coding style document? I can help track what we need to put into the coding style document.
There was a problem hiding this comment.
Indeed we do, will share with all in the dev slack channel -- thanks @angelxuanchang !
There was a problem hiding this comment.
I like the namespace alias. Currently, it's a bit confusing in that it is not followed everywhere. Once we resolve that, I think it will become second nature.
|
Let me know if you have more comments about this PR before I merge it. Thanks. |
|
Sincerely thank all the reviewers' work. Merge it. |
…igation tasks. (facebookresearch#196) Redesign Habitat-API to support continues actions and not pure navigation tasks. New class for Task Actions: Added registry for actions classes. We use a class not a method for action definition to provide relevant action space that uses Task/Dataset instance's information like number of words for answer vocabulary, resolution of frame etc. Change step function interface to support action parameters Added step function to the task with actions defined as task's methods Provide action space for each type of action Add reset function to Actions Added teleport action as example of parameterized action. Provide functionality to register several actions of same type using config only Create ActionSpace GYM space to describe action space in elegant way for multiple actions. Propagate action space through env. Moved Simulator calls to EmbodiedTask: Move is_episode_over functionality to Task class from Simulator, handle capturing finished conditions for different tasks differently Remove simulator STOP action Make Task methods reset and step provide observations that include sim observations Add possible actions to config and task Move from SimulatorAction list to defined Tasks in actions, handle non-stop actions Check usage of *args, **kwargs for all sensors, action, measures. Use dict as return type for act methods. Made VecEnv independent from Env step signature.
* Support ptex mesh - step 3: better naming -) rename the buffers on GPU side in PTexMeshData; -) rename variables in PTexMeshDrawable; * minor * minor * minor
This PR is to rename a number of buffers, textures to avoid ambiguity and increase the readability of the code.
-) rename the buffers on GPU side in PTexMeshData;
-) rename variables in PTexMeshDrawable;
Motivation and Context
It caused a lot of confusion in the past, making it difficult to read, maintain and debug.
How Has This Been Tested
Types of changes
Checklist