Add SSAO support to the VTK renderer

[Vinggui]

Although Drake has many supported VTK features, SSAO (Screen-Space Ambient Occlusion) can add significant visual improvement to the renderer and its sensor images.

This PR adds a new parameter that allows VTK renderer to also perform SSAO passes. To set it up, follow the same design of other VTK parameters:

vtk_param.enable_ssao = true;
drake::geometry::render::SsaoParameter ssao_params{};
vtk_param.ssao_params = ssao_params;

---

This change is 

[jwnimmer-tri]

@drake-jenkins-bot ok to test

[jwnimmer-tri]

+@SeanCurtis-TRI for feature review please, when you have downtime / gaps in our ongoing internal reviews.

+(release notes: feature)

Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers

---

geometry/render_vtk/render_engine_vtk_params.h line 209 at r1 (raw file):

   * See https://www.kitware.com/ssao/ for details.
   */
  bool enable_ssao{false};  

Generally in our configs we do not separate out "bool enable_ssao" from the presence or absence of the optional SsaoParameter details. It is plausible to remove this flag, and instead rely on ssao_params.has_value() to determine whether or not SSAO is enabled?

[jwnimmer-tri]

@drake-jenkins-bot retest this please

[jwnimmer-tri]

Ping to @SeanCurtis-TRI -- make sure this is on your dashboard and gets reviewed.

[SeanCurtis-TRI]

Sorry for the very slow review of this. I was worried about some of the inherent traps of SSAO that might be lurking in VTK's implementation. It turns out, I was right.

I've made a number of comments below. I have actually addressed all of my own comments. They can be found in this branch. If you like, I can simply push my commit to your branch and this PR will update with them. I didn't want to do that unilaterally, because, after all, it is your branch. :)

My branch also includes a patch for our internally built VTK to address a defect in VTK I found while reviewing its impact on Drake.

@SeanCurtis-TRI reviewed 3 of 7 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: 11 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers

---

geometry/render_vtk/render_engine_vtk_params.h line 209 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Generally in our configs we do not separate out "bool enable_ssao" from the presence or absence of the optional SsaoParameter details. It is plausible to remove this flag, and instead rely on ssao_params.has_value() to determine whether or not SSAO is enabled?

Agreed.This is now resolved.

---

geometry/render_vtk/render_engine_vtk_params.h line 32 at r2 (raw file):

  }

  double radius{0.5};

nit: The single biggest challenge for adding this feature is documenting these properties sufficiently well that a user can make intelligent decisions about when and how to tweak these values (and the trade offs inherent in those decisions).

Of particular note would be the units on some of these numerical values.

Let me know if you need help with this.

---

geometry/render_vtk/render_engine_vtk_params.h line 32 at r2 (raw file):

  }

  double radius{0.5};

nit: It's important that the default parameters be useful. I found that the kernel size and blur settings produced really noisy ambient occlusion. BY default, we should produce reasonable effects and inform the user how to tweak it if it feels too slow. So, we should pick more appropriate values.

---

geometry/render_vtk/render_engine_vtk_params.h line 33 at r2 (raw file):

  double radius{0.5};
  double bias{0.01};

BTW The quick and dirty VTK webpage provides a simple example for some of these parameters. More particularly, the radius is 100X bigger than the radius (although both get scaled by a scene normalizing factor).

Do we have a reason to favor a different default ratio?

(This is probably related to my previous note about documenting these parameters.)

---

geometry/render_vtk/render_engine_vtk_params.h line 207 at r2 (raw file):

   * geometry, producing images with better qualities and higher fidelity.
   * See https://www.kitware.com/ssao/ for details.
   * Non-specified parameters will use default values:

nit: Don't document the default values here. They are set in the definition of SsaoParameter and those default values will render into the doxygen (and python documentation). That is sufficient. Copying them here just makes maintenance more difficult. A single source of truth is always best.

(You also don't need to indicate that "non-specified parameters will use default values. That's a fundamental trait.)

---

geometry/render_vtk/render_engine_vtk_params.h line 215 at r2 (raw file):

   * blur: false
   */
  std::optional<render::SsaoParameter> ssao_params{};

BTW When I was writing up the yaml, I felt this really looked better as simply ssao. And even if it isn't more aesthetically pleasing, it's certainly more terse. :) Fewer characters (without loss of clarity) is always nice.

---

geometry/render_vtk/test/render_engine_vtk_params_test.cc line 35 at r1 (raw file):

    path: /path/to/environment_image.hdr
exposure: 1.0
enable_ssao: false

nit: It's still worth putting ssao into this yaml to make sure it's tested.

---

bindings/pydrake/geometry/geometry_py_render.cc line 419 at r2 (raw file):

  {
    using Class = geometry::render::SsaoParameter;

nit: We need unit tests for this.

---

geometry/render_vtk/test/internal_render_engine_vtk_test.cc line 3102 at r2 (raw file):

// Like WholeImageCustomParams, except we introduce the SSAO pass to the render
// engine parameters to detect differences.
TEST_F(RenderEngineVtkTest, WholeImageCustomParamsAndSSAO) {

nit: I think we want a slightly different testing strategy.

This test basically duplicates the other WholeImage.... test. It simply enables some SSAO. As such, it is better off rolled into that bigger test as just one more feature being simultaneously exercised.

That said, a dedicated test that makes sure that all of the parameters have an affect is worthwhile. That, for example, would've detected that the blur parameter wasn't being used successfully.

---

geometry/render_vtk/internal_render_engine_vtk.cc line 980 at r2 (raw file):

    ssao_pass->SetIntensityScale(ssao_parameter.intensity_scale);
    ssao_pass->SetIntensityShift(ssao_parameter.intensity_shift);
    if (!ssao_parameter.blur) {

nit: This isn't correct.

As you can see in the code, blur defaults to off. This PR's code assumes that the blur is on by default. This means, we never turn blurring on (regardless of the parameter value).

The simple solution is:

ssao_pass->SetBlur(ssao_parameter.blur);

---

geometry/render_vtk/test/whole_image_custom_and_SSAO_color.png line 0 at r2 (raw file):
BTW I'm a bit curious as to the provenance of the reference image. When I run this code my machine with a GeForce RTX 4090, I get a much noisier occlusion texture. It's most apparent on the otherwise unoccluded top surface of the cylinder.

Any thoughts?

At the end of the day, this specific image will go away and be replaced with an updated whole_image_custom_color.png reference image.

[SeanCurtis-TRI]

I've removed my VTK patch. In discussion with VTK, I've realized it isn't a defect. Just entirely counterintuitive. :)

[SeanCurtis-TRI]

BTW Given how long this feedback has been in coming, I recognize that this may no longer be on your radar. If you don't respond in the next week or so, I'll merge my changes and submit this for platform review. Either way, I appreciate you getting the ball rolling.

Reviewable status: 11 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers

[Vinggui]

Thanks for the updates! I will catch up on this this Monday Sept 22nd.

[Vinggui]

geometry/render_vtk/render_engine_vtk_params.h line 32 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: The single biggest challenge for adding this feature is documenting these properties sufficiently well that a user can make intelligent decisions about when and how to tweak these values (and the trade offs inherent in those decisions).

Of particular note would be the units on some of these numerical values.

Let me know if you need help with this.

Thanks for the insight on that. I saw that your commit already added much more explanation. Do you feel that we need even more related to this point?

[Vinggui]

geometry/render_vtk/render_engine_vtk_params.h line 32 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: It's important that the default parameters be useful. I found that the kernel size and blur settings produced really noisy ambient occlusion. BY default, we should produce reasonable effects and inform the user how to tweak it if it feels too slow. So, we should pick more appropriate values.

I agree with your opinion, but I do not have better values to suggest at this moment. Sticking to the default seems like a good start until we gather more experience with performance/quality of this feature.

[Vinggui]

geometry/render_vtk/render_engine_vtk_params.h line 33 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW The quick and dirty VTK webpage provides a simple example for some of these parameters. More particularly, the radius is 100X bigger than the radius (although both get scaled by a scene normalizing factor).

Do we have a reason to favor a different default ratio?

(This is probably related to my previous note about documenting these parameters.)

I think that the values in that example are just a best result that the author found. We could argue that the sceneSize should be something more dynamic to what it is been rendered, but I am not sure we can keep changing that value without making new instances.

I think that going for the default given by the VTK team is that neat solution, but I will take any other value if you find some better result.

[Vinggui]

geometry/render_vtk/render_engine_vtk_params.h line 207 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Don't document the default values here. They are set in the definition of SsaoParameter and those default values will render into the doxygen (and python documentation). That is sufficient. Copying them here just makes maintenance more difficult. A single source of truth is always best.

(You also don't need to indicate that "non-specified parameters will use default values. That's a fundamental trait.)

👍Good point. Thanks!

[Vinggui]

geometry/render_vtk/render_engine_vtk_params.h line 215 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW When I was writing up the yaml, I felt this really looked better as simply ssao. And even if it isn't more aesthetically pleasing, it's certainly more terse. :) Fewer characters (without loss of clarity) is always nice.

👍I agree. Thanks!

[SeanCurtis-TRI]

Just to be clear, if you like I can simply push my commit onto your branch. I don't actually expect you to go comment by comment and modify your own branch. You can if you want, I just wanted to emphasize that there is a simpler path.

Reviewable status: 11 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers

[Vinggui]

geometry/render_vtk/test/whole_image_custom_and_SSAO_color.png line at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW I'm a bit curious as to the provenance of the reference image. When I run this code my machine with a GeForce RTX 4090, I get a much noisier occlusion texture. It's most apparent on the otherwise unoccluded top surface of the cylinder.

Any thoughts?

At the end of the day, this specific image will go away and be replaced with an updated whole_image_custom_color.png reference image.

I ran the bazel tests in a virtual desktop machine (linux). Interesting that you got a different result for using another GPU.
Did you run it headless as well?

[Vinggui]

Yes, I took my time to read it all and also observe your changes. LGTM. I will just pull it here. Thanks for the help!

[Vinggui]

I mean. Go ahead and push it plz.

[SeanCurtis-TRI]

Ok, I've pushed.

BTW I just noticed you made the change in master. I was slightly nervous about pushing to your master. In the future, you might find it cleaner to create a dedicated branch for your PR. But, as is, I've pushed to your master and I believe things are safe and good. (Let me know if I messed something up.)

Reviewable status: 6 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Vinggui)

---

geometry/render_vtk/render_engine_vtk_params.h line 32 at r2 (raw file):

Previously, Vinggui (Vinicius Guimaraes) wrote…

Thanks for the insight on that. I saw that your commit already added much more explanation. Do you feel that we need even more related to this point?

The documentation I added was my best effort at being complete. If you're content with it, we can call it good. If you think there's something missing, then we can fix it accordingly.

---

geometry/render_vtk/render_engine_vtk_params.h line 32 at r2 (raw file):

Previously, Vinggui (Vinicius Guimaraes) wrote…

I agree with your opinion, but I do not have better values to suggest at this moment. Sticking to the default seems like a good start until we gather more experience with performance/quality of this feature.

Based on working on the test, the new settings are what I'd propose as the starting point. It means we have at least one image where the ambient occlusion looks good. We may revisit the defaults later if it turns out I overtuned to the wrong scene.

---

geometry/render_vtk/render_engine_vtk_params.h line 33 at r2 (raw file):

Previously, Vinggui (Vinicius Guimaraes) wrote…

I think that the values in that example are just a best result that the author found. We could argue that the sceneSize should be something more dynamic to what it is been rendered, but I am not sure we can keep changing that value without making new instances.

I think that going for the default given by the VTK team is that neat solution, but I will take any other value if you find some better result.

After documenting the parameters, I'm sufficiently content to go with the new defaults; even if I don't have mathematical proofs for their correctness.

---

geometry/render_vtk/test/whole_image_custom_and_SSAO_color.png line at r2 (raw file):

Previously, Vinggui (Vinicius Guimaraes) wrote…

I ran the bazel tests in a virtual desktop machine (linux). Interesting that you got a different result for using another GPU.
Did you run it headless as well?

That's a good point. I haven't run it against mesa. Now that I've pushed my commit to your branch, CI will get a chance to run against and we'll see what it says (it runs software drivers).

[SeanCurtis-TRI]

+a:@jwnimmer-tri for platform review, come Monday.

@SeanCurtis-TRI reviewed 4 of 4 files at r8.
Reviewable status: LGTM missing from assignees jwnimmer-tri(platform),SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Vinggui)

---

geometry/render_vtk/test/whole_image_custom_and_SSAO_color.png line at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Ok, we've passed CI! I'm going to do another once over and then we'll forward it for platform review.

Ok, I've done my last scrub.

[jwnimmer-tri]

@jwnimmer-tri reviewed 1 of 7 files at r1, 4 of 8 files at r3, 1 of 5 files at r6, 6 of 6 files at r9, all commit messages.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Vinggui)

---

geometry/render_vtk/render_engine_vtk_params.h line 17 at r9 (raw file):

namespace geometry {

namespace render {

Why are these specific params in the render namespace, while all of the other VTK structs (e.g., EnvironmentMap) are not? (Also, the namespace render is not consistent with the subdirectory name.)

If we want it to be in namespace render like the light params, then its definition needs to be in geometry/render, not geometry/render_vtk.

---

geometry/render_vtk/render_engine_vtk_params.h line 277 at r9 (raw file):

  /** An optional SSAO (screen-space ambient occlusion) parameters set. When
   specified, VTK enable screen-space ambient occlusion configured by the

typo

Suggestion:

 enables

---

geometry/render_vtk/test/internal_render_engine_vtk_test.cc line 2360 at r9 (raw file):

//   1. Provide a base-line image to validate the default values as visually
//      reasonable.
//   2. Show that all of the parameters effect the rendered result. Note: the

typo

Suggestion:

affect

---

geometry/render_vtk/internal_render_engine_vtk.cc line 981 at r9 (raw file):

    ssao_pass->SetBlur(ssao_parameter.blur);
    full_passes->AddItem(ssao_pass);
  } else {

nit My brain struggled to follow the order of passes. Some re-ordering would probably have helped?

Suggestion:

  if (parameters_.ssao.has_value()) {
    vtkNew<vtkOpaquePass> opaque_pass;

    vtkNew<vtkCameraPass> ssao_camera_pass;
    ssao_camera_pass->SetDelegatePass(opaque_pass);

    vtkNew<vtkSSAOPass> ssao_pass;
    ssao_pass->SetDelegatePass(ssao_camera_pass);

    const auto& ssao_parameter = parameters_.ssao.value();
    ssao_pass->SetRadius(ssao_parameter.radius);
    ssao_pass->SetBias(ssao_parameter.bias);
    ssao_pass->SetKernelSize(ssao_parameter.sample_count);
    ssao_pass->SetIntensityScale(ssao_parameter.intensity_scale);
    ssao_pass->SetIntensityShift(ssao_parameter.intensity_shift);
    ssao_pass->SetBlur(ssao_parameter.blur);

    full_passes->AddItem(ssao_pass);
  } else {

---

geometry/render_vtk/test/ssao_reference.png line 0 at r9 (raw file):
BTW This reminds me of some minecraft monster from the nether.

[jwnimmer-tri]

FYI be sure to rebase locally against the very tip of master. I suspect the new binding will need a small tweak with the new docstrings.

[SeanCurtis-TRI]

Done

Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Vinggui)

---

geometry/render_vtk/render_engine_vtk_params.h line 17 at r9 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Why are these specific params in the render namespace, while all of the other VTK structs (e.g., EnvironmentMap) are not? (Also, the namespace render is not consistent with the subdirectory name.)

If we want it to be in namespace render like the light params, then its definition needs to be in geometry/render, not geometry/render_vtk.

I'd missed the fact that it was different from all of the others. D'oh.

That said, the namespacing of of this stuff is insane.

[SeanCurtis-TRI]

From me.

Reviewable status: 1 unresolved discussion, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Vinggui)

[jwnimmer-tri]

CI says we need a regenerate?

@jwnimmer-tri reviewed 4 of 7 files at r10, 3 of 3 files at r11, all commit messages.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Vinggui)

[Vinggui]

[jwnimmer-tri]

CI says we need a regenerate?

Yes, there was a verb edit pending. I pushed it now.

@jwnimmer-tri reviewed 2 of 2 files at r12, all commit messages.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Vinggui)

[jwnimmer-tri]

@SeanCurtis-TRI the macOS CI is grumpy:

[9:27:11 AM]  [ RUN      ] RenderEngineVtkTest.Ssao
[9:27:11 AM]  geometry/render_vtk/test/internal_render_engine_vtk_test.cc:341: Failure
[9:27:11 AM]  Expected: (num_acceptable / static_cast<float>(ref_image.size())) >= (conformity), actual: 0.98868978 vs 0.99
[9:27:11 AM]  Google Test trace:
[9:27:11 AM]  geometry/render_vtk/test/internal_render_engine_vtk_test.cc:2419: Default image
[9:27:11 AM]  
[9:27:11 AM]  [  FAILED  ] RenderEngineVtkTest.Ssao (803 ms)

[Vinggui]

Increase tolerance just a little more?

[jwnimmer-tri]

Possibly. I'm leaving it to @SeanCurtis-TRI to decide (who will be back in the office on Monday).

Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Vinggui)

[jwnimmer-tri]

FWIW I also recommend that we land #23374 upgrade first, then merge up this branch, and then see where things stand.

[SeanCurtis-TRI]

I've loosened the image tolerance to accommodate for mac. I've also done another merge from master. However, we still want to wait for #23374 to merge before landing this.

@SeanCurtis-TRI reviewed 2 of 3 files at r11, 2 of 2 files at r12, 3 of 3 files at r13, all commit messages.
Reviewable status: 1 unresolved discussion, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Vinggui)

---

a discussion (no related file):
Blocking for #23374 and the opportunity to see if it changes anything.

[jwnimmer-tri]

@jwnimmer-tri reviewed 3 of 3 files at r13, all commit messages.
Reviewable status: 1 unresolved discussion, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Vinggui)

[SeanCurtis-TRI]

@SeanCurtis-TRI reviewed 1 of 1 files at r14, all commit messages.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Vinggui)

---

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Blocking for #23374 and the opportunity to see if it changes anything.

Merged, merged, and passes locally. Let's see what CI thinks of that.

[SeanCurtis-TRI]

@SeanCurtis-TRI reviewed 3 of 6 files at r9, 3 of 7 files at r10.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Vinggui)

[jwnimmer-tri]

@jwnimmer-tri reviewed 1 of 1 files at r14, all commit messages.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Vinggui)

[jwnimmer-tri]

The macOS build is happy.

Our CI is a bit overloaded today, so the -debug build is very slow. I ran a full debug build locally with no problems just now, so I'll go ahead and squash this into master now.

Thanks for the new feature @Vinggui!

[SeanCurtis-TRI]

Huzzah!