[FEATURE] Interactive viewer plugins #2004
Conversation
|
This feature looks very convenient! |
|
Do you consider this as a "universal" plugin or something pyrender-specific? This would give you the answer :)
I'm not a fan of these objects. Overly complicated and not very pythonic in my view.
That would be great! But we need a way to expose this in a clean way. |
Milotrince
force-pushed
the
viewer_plugin
branch
2 times, most recently
from
98601bb to
1e672b0
Compare
|
@Milotrince If you could finish this PR I would be happy to review it and merge it :) |
Milotrince
force-pushed
the
viewer_plugin
branch
from
1e672b0 to
343e8cb
Compare
|
@duburcqa Could I get your opinion on the state of the PR? there are some design decisions I'd like your input on!
I still need to add tests. |
Sure ! I will do it this week ! Could you fix the conflicts in the meantime? Thank you :) |
Milotrince
force-pushed
the
viewer_plugin
branch
2 times, most recently
from
7639573 to
0a14941
Compare
|
Small comment on snapshots PR: https://huggingface.co/datasets/Genesis-Intelligence/snapshots/discussions/6 |
|
Could you split the introduction of You can open a (stacked) PR for the second part right away, merging on top of this branch instead of main (with draft status). |
Milotrince
force-pushed
the
viewer_plugin
branch
from
f530fdc to
f89c2e8
Compare
Ok, I removed the
I can't directly open a stacked PR on the Genesis repo since the parent branch lives on my forked repo. I opened one on my fork for sake of preview https://github.com/Milotrince/Genesis/pull/3/changes |
|
Thank you for the PR! After addressing those comments, I think I'm good with this PR. |
There was a problem hiding this comment.
Did you validate that it works fine for all possible user arguments that may be impacted by your changes?
parser.add_argument("--ipc", action="store_true", default=False, help="Enable IPC coupling")
parser.add_argument(
"--mode",
type=str,
default="playback",
choices=["interactive", "record", "playback"],
help="Mode: interactive, record (save trajectory), or playback (replay trajectory, default)",
)
parser.add_argument(
"--trajectory",
type=str,
default="grasp_cloth1.csv",
help="Trajectory file to load (for playback mode, default: grasp_cloth1.csv)",
)
parser.add_argument("--list", action="store_true", help="List available trajectories and exit")
parser.add_argument("-v", "--vis", action="store_true", default=False, help="Show Genesis viewer")
parser.add_argument("--vis_ipc", action="store_true", default=False, help="Show IPC GUI")
Overall, I really like what you are doing with this PR. I think it is going in the right direction and is more future proof than what we currently have. |
Overall, this is a good idea I think. But it could be nice to provide a list of plugins, and make it empty to disable them all, no? Moreover, I would recommend to keep the options |
ah yeah i think a list of ViewerPlugin objects would make more sense. Which do we prefer for the usage, the options/registration pattern or |
|
What does |
how about this: Viewer stores a list of plugins, e.g. |
I think this mechanism is more natural and less disruptive compared to what we have now. What do you think @Milotrince ? |
Milotrince
force-pushed
the
viewer_plugin
branch
from
65d8a04 to
a9e212e
Compare
|
@Milotrince CI is failing! |
Milotrince
force-pushed
the
viewer_plugin
branch
from
9979288 to
6dbdea0
Compare
|
updated snapshot pr: https://huggingface.co/datasets/Genesis-Intelligence/snapshots/discussions/7 |
| class _LazyPygletKeyModule: | ||
| """ | ||
| Lazy load pyglet to avoid premature initialization. | ||
|
|
||
| Note | ||
| ---- | ||
| Importing pyglet before OpenGL context is created can lead to segmentation faults on some platforms. | ||
| """ | ||
|
|
||
| _module = None | ||
|
|
||
| def __getattr__(self, name): | ||
| if type(self)._module is None: | ||
| from pyglet.window import key as pyglet_key | ||
|
|
||
| type(self)._module = pyglet_key | ||
| return getattr(type(self)._module, name) | ||
|
|
||
| def __dir__(self): | ||
| if type(self)._module is None: | ||
| from pyglet.window import key as pyglet_key | ||
|
|
||
| type(self)._module = pyglet_key | ||
| return dir(type(self)._module) | ||
|
|
||
|
|
||
| Key = _LazyPygletKeyModule() |
There was a problem hiding this comment.
I would recommend to just from pyglet.window import key as pyglet_key on top of every single method that is calling pyglet_key. Another option would be to make pyglet_key an attribute of KeyBindings class at init (via self.pyglet_key = pyglet_key).
There was a problem hiding this comment.
Ok I see why you need this. Because this part is user facing... What about exposing our own enum class for keys, that we will internally convert to pyglet. Breaking pyglet encapsulation by asking users to pass pyglet enum is problematic. First, we do not want to get struck with pyglet backend, in the future we may extend this to more backends. Second, the users may try to import pyglet themselves, which would cause segfault.
| from genesis.vis.viewer_plugins import DefaultControlsPlugin | ||
|
|
||
| self._viewer_plugins.append(DefaultControlsPlugin()) |
There was a problem hiding this comment.
Why is it necessary to delay import?
There was a problem hiding this comment.
I just merge your snapshot PR (probably a bit too fast but it is not a big deal). Did you remove old now relevant screenshots if any?
Disabling shadows is not a good unit test, because it is not enabled in the first place if no GPU is detected. Your wireframe and disable shadow systematically (see other unit tests doing this).
3 participants
Description
nice-to-have feature, allows users to integrate interactive viewer plugins.
pynputgs.vis.keybindings.Keybindcallback registrationgs.options.ViewerOptions:disable_keyboard_shortcutsreplaced withdisable_default_keybindsdisable_help_textRelated Issue
Resolves #713
Motivation and Context
ViewerPluginenables directly interfacing with pyviewer mouse/keyboard controls.pynputHow Has This Been / Can This Be Tested?
added
tests/test_viewer.pyScreenshots (if appropriate):
Checklist:
Submitting Code Changessection of CONTRIBUTING document.