-
Notifications
You must be signed in to change notification settings - Fork 2.6k
[FEATURE] Interactive viewer plugins #2004
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
base: main
Are you sure you want to change the base?
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. |
98601bb to
1e672b0
Compare
|
@Milotrince If you could finish this PR I would be happy to review it and merge it :) |
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 :) |
5272896 to
8e54491
Compare
7639573 to
0a14941
Compare
|
Small comment on snapshots PR: https://huggingface.co/datasets/Genesis-Intelligence/snapshots/discussions/6 |
| # Register keybindings | ||
| from pyglet.window import key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add from pyglet.window import key in your new keybindings module, so that users can do:
from genesis.ext.pyrender.interaction.keybindings import keyThis would avoid breaking encapsulation of pyglet, that should be an implementation details for users.
Similarly, it would be great to expose your keybindings module outside ext, that is also supposed to be an implementation details for the end user. This would be relaying not the same mechanism I just mentioned. Ideally the user should be able to replace genesis.ext.pyrender.interaction.keybindings by genesis.vis.keybindings
|
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). |
| self.rpms = [self.thrust] * 4 | ||
| self.rpms = np.clip(self.rpms, 0, 25000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure about your formula?
This is a bit dirty. It would be more clean to allocate rpm once (ndarray) and update its value by reference. By the way, there is no need to clip self.thrust? Here you keep incrementing something that is out of range, which is weird, because the user won't be able to decelerate for a while if accelerating too much beyond the limit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming the formula is correct:
self.thrust = min(max(self.thrust + self.thrust_delta, 0), 25000)
self.rpms[:] = self.thrustThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, it is really relevant to store both thrust and rpms if they are both the same but expended? Am i missing something?
| scene.viewer.register_keybinds( | ||
| *direction_keybinds(key.UP, "move_forward", (1.0, 1.0, -1.0, -1.0)), | ||
| *direction_keybinds(key.DOWN, "move_backward", (-1.0, -1.0, 1.0, 1.0)), | ||
| *direction_keybinds(key.LEFT, "move_left", (-1.0, 1.0, -1.0, 1.0)), | ||
| *direction_keybinds(key.RIGHT, "move_right", (1.0, -1.0, 1.0, -1.0)), | ||
| Keybind(key.SPACE, KeyAction.HOLD, name="accelerate", callback=controller.accelerate), | ||
| Keybind(key.LSHIFT, KeyAction.HOLD, name="decelerate", callback=controller.decelerate), | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be nice to have a way to detect conflicting key bindings with the default viewer controls, then raise an exception if any, or just a warning eventually.
| target_pos = [robot_init_pos.copy()] # Use list for mutability in closures | ||
| target_R = [robot_init_R] # Use list for mutability in closures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand. numpy arrays are already mutable. You can modify then in place. So doing this should not be necessary.
| target_pos[0] = robot_init_pos.copy() | ||
| target_R[0] = robot_init_R |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just update them in place: target_pos[:] = robot_init_pos, target_R[:] = robot_init_R
|
|
||
| import numpy as np | ||
| from huggingface_hub import snapshot_download | ||
| from scipy.spatial.transform import Rotation as R |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of scipy should be avoided if possible, but it was already there before your PR.
| stop = False | ||
| step_count = 0 | ||
|
|
||
| while not stop: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the stop mechanism had to be removed in favor of try-except?
| def move_forward(): | ||
| target_pos[0] += KEY_DPOS | ||
|
|
||
| def move_backward(): | ||
| target_pos[0] -= KEY_DPOS | ||
|
|
||
| def move_right(): | ||
| target_pos[1] -= KEY_DPOS | ||
|
|
||
| def move_left(): | ||
| target_pos[1] += KEY_DPOS | ||
|
|
||
| def move_down(): | ||
| target_pos[2] -= KEY_DPOS | ||
|
|
||
| def move_up(): | ||
| target_pos[2] += KEY_DPOS | ||
|
|
||
| def roll_ccw(): | ||
| target_euler[0] += KEY_DANGLE | ||
|
|
||
| def roll_cw(): | ||
| target_euler[0] -= KEY_DANGLE | ||
|
|
||
| def pitch_up(): | ||
| target_euler[1] += KEY_DANGLE | ||
|
|
||
| def pitch_down(): | ||
| target_euler[1] -= KEY_DANGLE | ||
|
|
||
| def yaw_ccw(): | ||
| target_euler[2] += KEY_DANGLE | ||
|
|
||
| def yaw_cw(): | ||
| target_euler[2] -= KEY_DANGLE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very verbose for little benefit. What about only exposing translate, rotate, with some "axis: int, is_negative: bool" argument or just axis: np.ndarray, and use partial to keybind them? That would be more flexible and simpler I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we move this feature in a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same.
| target_entity.set_qpos(np.concatenate([target_pos, target_quat])) | ||
| # Initialize target pose | ||
| target_pos = robot_init_pos.copy() | ||
| target_R = [robot_init_R] # Use list to make it mutable in closures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as before. No need to use list encapsulation. Just modify in-place the pre-allocated array.
|
|
||
| # Robot teleoperation callback functions | ||
| def move(dpos: tuple[float, float, float]): | ||
| global target_pos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this global line entirely. It is not needed and probably confusing for users.
| ray_direction_world = ti_normalize(ti_transform_by_quat(ray_dir_local, link_quat), gs.EPS) | ||
|
|
||
| # --- 2. BVH Traversal --- | ||
| # FIXME: this duplicates the logic in LBVH.query() which also does traversal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why FIXME disappeared? You actually implemented the fix?!
Description
nice-to-have feature, allows users to integrate interactive viewer plugins.
Related Issue
N/A
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):
Screen.Recording.2025-11-17.at.18.03.51.mov
Checklist:
Submitting Code Changessection of CONTRIBUTING document.