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

Allow ModelVisualizer to consume render camera configuration #21347

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SeanCurtis-TRI
Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI commented Apr 22, 2024

The user can now pass a yaml file representing a CameraConfig file into model visualizer. This allows for more direct ability to configure render cameras.


This change is Reviewable

@SeanCurtis-TRI SeanCurtis-TRI added the release notes: feature This pull request contains a new feature label Apr 22, 2024
Copy link
Contributor Author

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

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

+(release notes: feature)
cc @rcory

Note to reviewer:

Simplest way to test this is to create a file (foo.yaml) with content akin to this:

renderer_class: RenderEngineVtk
width: 512
height: 256

Then run ModelVisualizer:

bazel run //tools:model_visualizer_private -- package://drake_models/tri_homecart/homecart.dmd.yaml --show_rgbd_sensor --camera_config /path/to/foo.yaml

Editing the size of the image should change what you see in the rendered output. Feel free to play with any of the other settings as you see fit.

Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers


bindings/pydrake/visualization/model_visualizer.py line 105 at r1 (raw file):

             "add an environment to the camera."
    )
    args_parser.add_argument(

BTW Thoughts on having to specify both --show_rgbd_sensor and the --camera_config. Would we want it so that the latter implies the former?


bindings/pydrake/visualization/model_visualizer.py line 113 at r1 (raw file):

             "file will be applied: (1) X_PB is always ignored, because the "
             "camera is affixed to the Meshcat browser camera. For various "
             "reasons, if cast_shadows is set to True, the displayed render "

BTW To see this shadow issue, change your yaml to:

renderer_class: !RenderEngineVtkParams
  lights:
    - type: "spot"
      cone_angle: 30
      direction: [0, 0, -1]
      frame: "world"
      position: [0, 0, 1.5]
      intensity: 10
      attenuation_values: [1, 0.5, 0.1]
  cast_shadows: True
width: 512
height: 256

Copy link
Contributor Author

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

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

+@jwnimmer-tri for feature review, please.

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

The user can now pass a yaml file representing a CameraConfig file into
model visualizer. This allows for more direct ability to configure
render cameras.
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

a discussion (no related file):
Having pondered this a while, I'm pretty sure this tips us over the edge for how this tool's argparse stuff should work. Let's hop on a call to review.

The gist of my idea goes like:

(1) Ephemeral arguments like filename, open_window, loop_once, etc. remain as-is.

(2) Scene configuration moves to yaml wholesale, disappearing from the command line.
(2.a) The render camera would be a sub-struct over the overall ModelVisualizerConfig
(2.b) Other sub-structs would be things like VisualizationConfig, MeshcatParams, environment map, etc.
(2.b) The "show triads" option should really be in VisualizationConfig and not a modelvis-specific option. For now it can live in the yaml separately but we should push the code down to C++ and add to the sim-wide config struct later.

This makes configuring model vis more like how we configure full simulations. I expect that in the long the similarity will be more intuitive for users, easier to maintain for us, require ever-fewer bespoke glue stuff in this tool. Really, model vis should be just like any other simulation, except that it uses joinsliders instead of dynamics. (Simulations should have a "Reload model files" button too!)


Copy link
Contributor Author

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Having pondered this a while, I'm pretty sure this tips us over the edge for how this tool's argparse stuff should work. Let's hop on a call to review.

The gist of my idea goes like:

(1) Ephemeral arguments like filename, open_window, loop_once, etc. remain as-is.

(2) Scene configuration moves to yaml wholesale, disappearing from the command line.
(2.a) The render camera would be a sub-struct over the overall ModelVisualizerConfig
(2.b) Other sub-structs would be things like VisualizationConfig, MeshcatParams, environment map, etc.
(2.b) The "show triads" option should really be in VisualizationConfig and not a modelvis-specific option. For now it can live in the yaml separately but we should push the code down to C++ and add to the sim-wide config struct later.

This makes configuring model vis more like how we configure full simulations. I expect that in the long the similarity will be more intuitive for users, easier to maintain for us, require ever-fewer bespoke glue stuff in this tool. Really, model vis should be just like any other simulation, except that it uses joinsliders instead of dynamics. (Simulations should have a "Reload model files" button too!)

I buy that.

However, I have one thought. It was convenient to make this PR, because I'd hacked this together at the beginning of the year to prepare some demos. The ability to do this kind of configuration has value now (Rick was wrestling with doing this kind of configuration in a reasonable loop). How high a priority would assign to the introduction of ModelVisualizerConfig? It seems that we run the risk of perfect becoming the enemy of good if this simply falls to the bottom of the infinite todo list.

That said, short-term, @rcory can simply grab this branch and apply it locally if he needs to do some rendering configuration stuff. Eventually, we'll provide the more robust mechanism.


Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I buy that.

However, I have one thought. It was convenient to make this PR, because I'd hacked this together at the beginning of the year to prepare some demos. The ability to do this kind of configuration has value now (Rick was wrestling with doing this kind of configuration in a reasonable loop). How high a priority would assign to the introduction of ModelVisualizerConfig? It seems that we run the risk of perfect becoming the enemy of good if this simply falls to the bottom of the infinite todo list.

That said, short-term, @rcory can simply grab this branch and apply it locally if he needs to do some rendering configuration stuff. Eventually, we'll provide the more robust mechanism.

Rick should just be running his simulator (for 0 seconds), not this new feature. His simulator should already have 100% of config knobs he needs, and should exactly reproduce during preview what a simulation user would see during rollout. (This preview tool will have a gap versus what he's working with for his sim.) I understand that sim has some rough edges. We can volunteer to help fix those, which would then have more durable value than this more narrow preview feature.

In any case, I don't think it will be that much work to rewrite this PR to the new spelling. I was imagining it would happen as part of this PR review cycle. Even just opening up the regular VisualizationConfig knobs for users would be a big win. I'm willing to allocate some of our project time to move this forward.


Copy link
Contributor Author

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Rick should just be running his simulator (for 0 seconds), not this new feature. His simulator should already have 100% of config knobs he needs, and should exactly reproduce during preview what a simulation user would see during rollout. (This preview tool will have a gap versus what he's working with for his sim.) I understand that sim has some rough edges. We can volunteer to help fix those, which would then have more durable value than this more narrow preview feature.

In any case, I don't think it will be that much work to rewrite this PR to the new spelling. I was imagining it would happen as part of this PR review cycle. Even just opening up the regular VisualizationConfig knobs for users would be a big win. I'm willing to allocate some of our project time to move this forward.

While I hesitate to put words in Rick's mouth, my impression is that the simulation infrastructure is so unwieldy that the latency between making a change and seeing the result is massive. But, as you say, if that's a problematic issue for them, then they probably don't have the right knobs.

Ok, I can spend a couple of hours to shift this towards ModelVisualizerConfig.


Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

a discussion (no related file):

... my impression is that ...

Yup, the three of us had an email thread about it.

But, as you say, if that's a problematic issue for them, then they probably don't have the right knobs.

... and that fault extends to a lot more kinds of edit-test-iterate cycles than just the render engine config.

The one nice thing in this PR that isn't as easily solved in the putative short-simulation workflow is the ability to move the Meshcat camera in the browser, and watch the render image update live. In most cases, the simulation workflow would have a rigid camera posture. (And FYI that's main reason I'm voting for reworking this PR to the new spelling, instead of abandoning it.)


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: feature This pull request contains a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants