Registry #204
Registry #204
Conversation
facebook-github-bot
added
the
CLA Signed
docs/pages/new-actions.rst
Outdated
| @@ -20,8 +20,8 @@ actuation_spec contains any parameters needed by that control function. | |||
| See ``habitat_sim/agent/controls/default_controls.py`` for more example | |||
| controls. | |||
|
|
|||
| Controls are registered using the `habitat_sim.agent.register_move_fn()` | |||
| function. This function takes the functor to register and, optionally, the name | |||
| Controls are registered using `habitat_sim.registry.register_move_fn`. | |||
There was a problem hiding this comment.
Oh, I was always appending the () to function references (to distinguish them from variables/properties/modules)... and I did that for the soon-to-be-PR'd habitat-api docs rework as well. Please let me know if I should stop doing that :)
There was a problem hiding this comment.
I like that! I didn't realize that's why you had done that so it didn't register that I should keep it.
| r"""registry is a central source of truth in Habitat-Sim | ||
|
|
||
| Taken from Pythia, it is inspired from Redux's | ||
| concept of global store. registry maintains mappings of various information |
There was a problem hiding this comment.
This registry is slightly different than the one in habitat-api
AH, that's why this docstring felt like a deja vu to me :)
| from habitat_sim.nav import * | ||
| from habitat_sim.agent import * | ||
| from habitat_sim.simulator import * | ||
| from habitat_sim.bindings import * |
There was a problem hiding this comment.
Maybe at some point it will be good to move away from * imports.
There was a problem hiding this comment.
As the comment above says :) I kept this for backwards compatibility and docs already show the names inside the nav / agen / ... submodules, so it's "just" about gradually moving the code to do that as well.
There was a problem hiding this comment.
Ah, missed the comment, sorry :)
habitat_sim/registry.py
Outdated
| return re.sub("([a-z0-9])([A-Z])", r"\1_\2", s1).lower() | ||
|
|
||
|
|
||
| class registry: |
There was a problem hiding this comment.
In Pythia, I actually annotated the registry file for documentation on registry. See https://github.com/facebookresearch/pythia/blob/master/pythia/common/registry.py. Singleton pattern also saves from PEP8 violations of lowercase class names. Though, this will also work. :)
There was a problem hiding this comment.
I tried annotating the file, but the docs where fighting with me :/ This made the docs happy
There was a problem hiding this comment.
https://learnpythia.readthedocs.io/en/latest/common/registry.html seems to render fine. Maybe some difference in sphinx configurations.
There was a problem hiding this comment.
Ah. I remember now what the main thing was. habitat_sim.registry.register_move_fn cross-links correctly with the class but not the instance.
There was a problem hiding this comment.
in any case, if there's something the docs aren't handling correctly (or could handle better), please complain loudly ;)
There was a problem hiding this comment.
This is kinda a strange case where I want to document a singleton and the cross-link to the methods on it. I am not sure if any doc engine current supports that :)
There was a problem hiding this comment.
Adding habitat_sim.registry = type(habitat_sim.registry) to docs/conf.py let's me use a singleton and have the docs get generated for it almost exactly as they should (it says that habitat_sim.registry is a class, but whatevs)
|
|
||
| return controller | ||
|
|
||
| if controller is None: |
docs/docs.rst
Outdated
| @@ -82,11 +82,19 @@ | |||
| We currently have the following actions added by default. Any action not | |||
| registered with an explict name is given the snake case version of the | |||
| class name, i.e. ``MoveForward`` can be accessed with the name | |||
| ``move_forward``. | |||
| ``move_forward``. See `habitat_sim.registry.register_move_fn`, `habitat_sim.agent.SceneNodeControl`, | |||
| and `habitat_sim.agent.ActuationSpec` | |||
There was a problem hiding this comment.
Just FYI: the referencing works also relatively to currently documented name -- so in this case it could be a (shorter)
See `registry.register_move_fn`, `SceneNodeControl`, and `ActuationSpec`as well -- of course the longer the displayed prefix is, the less potentially confusing the name is, so whichever feels best for you :) But generally omitting habitat_sim wouldn't hurt readability at all I think.
Switch to a proper registry pattern
Motivation and Context
With PR #189 adding another thing that makes sense to also use a registry for, it is time to swap to a proper registry pattern!
This registry is slightly different than the one in habitat-api as it's just a class instead of a global singleton instance. The global singleton instance was hard to generate proper docs for while generating docs for a class was super simple. I don't believe there is any functional difference between the two.
How Has This Been Tested
Via the existing tests.
Note that the CI is expected to fail for this PR as habitat-api has an example which uses habitat-sim's registry that will need to be updated.
Types of changes