Skip to content

[Won't Merge] Refactoring #18

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

jaku-jaku
Copy link

Hi Rohan,
Here is my revision of the viewer:

Major refactoring:

  • C style implementation
  • reduced callback processing
  • constant time keyboard callback action through hashing instead of linear time with if-else
  • separate on-screen rendering and built-in multi camera off-screen rendering
viewer.process_safe() # process interactions and updates from callbacks
viewer.update_safe() # update mujoco engine
viewer.render_safe() # render on screen
self.mj_viewer.render_sensor_cameras_safe() # render cameras off screen

Let me know if you have any comments.
Thank you for your initial works.

Best
Jack

jaku-jaku added 10 commits July 26, 2022 17:13
+ custom lib & credits to original repo and mujoco-py
+ mujoco_viewer thread safe and refactored entirely from mujoco_viewer
  v0.1.0
+ efficient and responsive key callback
+ resize callback
+ co-develop along with mujoco-python-engine https://github.com/jaku-jaku/jx-mujoco-python-engine
+ [wip] mujoco camera buffer backend
+ modified examples to fit our lib
+ viewer minor fixes on mouse interactions
@rohanpsingh
Copy link
Owner

Hi @jaku-jaku
I took a brief look at your changes, I think some of the general use functionalities could've be merged here too :)

Nevertheless, since you opened issue #16 I've been working on rendering multiple cameras on the side, besides some other cool features. Hope I can spend more time on this soon.

constant time keyboard callback action through hashing instead of linear time with if-else

Did this give you noticeably better performance?

Thanks.

@jaku-jaku
Copy link
Author

jaku-jaku commented Aug 15, 2022

Hi @jaku-jaku I took a brief look at your changes, I think some of the general use functionalities could've be merged here too :)

Yeah, but I kinda refactored almost everything into my personal style I preferred, that's why it might be hard to merge in. I personally feel the refactored version will be easier to collaborate on.

The main intention was to make the code architecture clean and easy to scale with more key registrations in the future, and serve as GUI interface in joint with main engine code. The rendering will be invoked by the engine thread, so separating the GUI callbacks and Rendering pipeline will be a good measure to void any potential bugs.

Lemme know your thoughts.
An example of the implementation will be: https://github.com/UW-Advanced-Robotics-Lab/simulation-mujoco-summit-wam

Nevertheless, since you opened issue #16 I've been working on rendering multiple cameras on the side, besides some other cool features. Hope I can spend more time on this soon.

Nice, many thanks!
I was attempting as well, and I have added that pipeline in. Later, I also notice 'dm_control' has the pipeline for multi-camera, but I have already worked based on your viewer. So I rendered off screen in viewer, and rendered on-screen with cv2 thread in my engine. The result looks like:
waterloo_steel

Did this give you noticeably better performance?

It's not much noticeable, but I just feel the If-else statement (used in mujoco-py) is quite itchy (from C/C++ POV), and can be buggy.

So, in the engine code, I can run in the update functions (or maybe separate thread in the future):

        # - render current view:
        for i in range(10):
            mujoco.mj_step(self.mj_model._model, self.mj_data._data) # stepping couple times
        self.mj_viewer.process_safe() # process GUI interfaces (keys registered till this update and mouse interactions) that was registered in callbacks
        self.mj_viewer.update_safe() # update the scene via mujoco2.2.x
        self.mj_viewer.render_safe() # render the raw main viewer off screen, and display on_screen with overlays
        self.mj_viewer.render_sensor_cameras_safe() # iterate through camera configurations, update the scene and render cameras off-screen
  • key map registration with bit operations (which is a common practice in C with one 32-bit flag, since it is an atomic operation)
  • separating the GUI callbacks and render processes
  • process all the UI input requests before the rendering at once

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants