Skip to content
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

Add mujoco rgbd rendering #1229

Merged

Conversation

DavidPL1
Copy link
Contributor

@DavidPL1 DavidPL1 commented Oct 23, 2024

Description

Adds a new render_mode to MuJoCo environments for when rgb and depth images are required as observations, e.g. to create point clouds.

Fixes #1226

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist:

  • I have run the pre-commit checks with pre-commit run --all-files (see CONTRIBUTING.md instructions to set it up)
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Open Questions

  • Currently this change was applied to all v5 and v4 environments. Is that correct or should I create v6 envs?
  • In case just changing v5 and v4 is fine, should this feature be added to the version history docstrings of the respective envs?

Copy link
Member

@pseudo-rnd-thoughts pseudo-rnd-thoughts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me other than the comment.
@Kallinteris-Andreas could you review as well

gymnasium/utils/passive_env_checker.py Outdated Show resolved Hide resolved
@DavidPL1
Copy link
Contributor Author

@pseudo-rnd-thoughts
Looks like Run PyTest / build-all (3.8, >=1.21,<2.0) for some reason got stuck at apt-get -y update can you trigger it again?

@pseudo-rnd-thoughts
Copy link
Member

I can't from my phone but will be home in an hour or so and can do then

Copy link
Collaborator

@Kallinteris-Andreas Kallinteris-Andreas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. need to update the index page (https://gymnasium.farama.org/environments/mujoco/ ) to document the available render_modes

  2. add a test in test_mujoco_rendering.py named test_rgbd_array, where we assert the output of test_rgbd_array is a proper combination of rgb_array and depth_array

  3. I am not convinced that the correct output format is a tuple of (rgb_image, depth_image) while it is the most human-readable format, it is not ideal for CNNs and would require an additional renderWrapper (for the cases where it used as addRenderObservation wrapper)
    edit: RGB-D CNNs do not appear to be "mature", see "RGB-D Object Recognition Using Deep Convolutional Neural Networks" as an example the data is preprocessed anyway, so long as the decision is done intentionally, I am fine with the current choice. @DavidPL1 you may be more familiar with depth CNNs what is your oppinon

Thanks

@@ -281,6 +281,10 @@ def render(
seg_ids[geom.segid + 1, 1] = geom.objid
rgb_img = seg_ids[seg_img]

if render_mode == "rgbd_array":
Copy link
Collaborator

@Kallinteris-Andreas Kallinteris-Andreas Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section (L259-L289) was not written to support 3 render types, it should be re-written to be clearer, into 2 stages, first stage collects the images, and the second stage returns the correct images.

if render_mode in ["depth_array", "rgbd_array"]:
    depth_img = depth_arr.reshape(self.viewport.height, self.viewport.width)
if render_mode in ["rgb_array", "rgbd_array"]:
    rgb_img = ...
if render_mode == "rgb_array":
    return ...
elif render_mode == "depth_array":
    return ...
elif render_mode == "rgbd_array":
    return ...

@DavidPL1
Copy link
Contributor Author

  1. need to update the index page (https://gymnasium.farama.org/environments/mujoco/ ) to document the available render_modes

  2. add a test in test_mujoco_rendering.py named test_rgbd_array, where we assert the output of test_rgbd_array is a proper combination of rgb_array and depth_array

Will do after we settle on the type

  1. I am not convinced that the correct output format is a tuple of (rgb_image, depth_image) while it is the most human-readable format, it is not ideal for CNNs and would require an additional renderWrapper (for the cases where it used as addRenderObservation wrapper)
    edit: RGB-D CNNs do not appear to be "mature", see "RGB-D Object Recognition Using Deep Convolutional Neural Networks" as an example the data is preprocessed anyway, so long as the decision is done intentionally, I am fine with the current choice. @DavidPL1 you may be more familiar with depth CNNs what is your oppinon

I'm am also not too familiar with RGB-D CNNs myself. After a quick read on the topic, researchers seem to perform all kinds of processing steps for alternative representations (voxels, pointclouds, ...). Usually RGB-D cameras provide access to two separate streams and datasets also provide the modalities separated.
Still, from my point of view both ways, tuple or 4D-image, would be fine. And custom wrappers should be used to get a favored representation.

I opted for a tuple because I thought it achieves the same thing as the rendering twice alternative (which I have already seen), returning the same types as the individual images. Also I'm using an existing Wrapper combining the two images into a point cloud.
Though I must admit that the naming rgbd_array might mislead into thinking a single array is returned.

@Kallinteris-Andreas
Copy link
Collaborator

Kallinteris-Andreas commented Oct 26, 2024

The Open3D python library seems to store the rgbd data on a tuple https://www.open3d.org/docs/latest/tutorial/Basic/rgbd_image.html#RGBD-images
I can not figure out how OpenCV structures the storage of rgbd images

A tuple of rgb and depth should be fine, as it is easily convertible to all other formats

Naming should perhaps be rgbd_tuple?

@DavidPL1
Copy link
Contributor Author

DavidPL1 commented Oct 28, 2024

judging by the function calls of e.g. rgbd::warpFrame taking both as separate inputs, OpenCV seems to also store image and depth separately.

Then I'll stick with the tuple implementation. rgbd_tuple also sounds good to me.

@Kallinteris-Andreas
Copy link
Collaborator

  1. add a test in test_mujoco_rendering.py named test_rgbd_tuple, where we assert the output of test_rgbd_tuple is a proper combination of rgb_array and depth_array

@Kallinteris-Andreas
Copy link
Collaborator

@pseudo-rnd-thoughts All I want from you is to tell us if you are okay with the name of the new render_mode being "rgbd_tuple"

@DavidPL1
Copy link
Contributor Author

  1. add a test in test_mujoco_rendering.py named test_rgbd_tuple, where we assert the output of test_rgbd_tuple is a proper combination of rgb_array and depth_array

See a72f44d or do you want to add something more to the test?


def test_rgbd_tuple():
"""Assert that rgbd_tuple is the proper combination of rgb and depth images as tuple"""
env = gymnasium.make("Ant-v5", camera_id=0, render_mode="rgbd_tuple").unwrapped
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

render once with rgb_array and depth_array and assert that the resulting images are the same

Copy link
Member

@pseudo-rnd-thoughts pseudo-rnd-thoughts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rgbd_tuple sounds good to me.

@Kallinteris-Andreas Kallinteris-Andreas merged commit 988999c into Farama-Foundation:main Oct 28, 2024
13 checks passed
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.

[Proposal] Add RGB-D Render Mode to MuJoCo Environments
3 participants