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

Restore RenderEngineVtk MultiMaterialObject test for EGL #22053

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 Oct 16, 2024

The crux of this was that rainbow_stripes.png has an alpha channel. When we enabled shadows in VTK, we swapped out rendering pipelines which treated meshes with those images differently. The test never really noticed that these changed. Changing the backend merely caused the change to manifest in a way that the test noticed (merely by happenstance).

The solution:

  • Remove the alpha channel.
  • Modify the test so it will be more sensitive to those kinds of changes.
    • Refactor the CompareImage code to use more widely (removing its behavior of saving the image to compare).
    • Make use of VTK's automatically save rendered image functionality.
    • Update callsites to CompareImage accordingly.

This does mean we don't have a test of transparent things. This is not a problem:

  1. We only accidentally had one before.
  2. We don't support transparency with any kind of uniformity or reliability.
  3. We need a test that tracks what we do support and document supported transparency appropriately (a follow up PR).

Resolves #22044


This change is Reviewable

The crux of this was that rainbow_stripes.png has an alpha channel. When
we enabled shadows in VTK, we swapped out rendering pipelines which treated
meshes with those images differently. The test never really noticed that
these changed. Changing the backend merely caused the change to manifest
in a way that the test noticed (merely by happenstance).

The solution:
 - Remove the alpha channel.
 - Modify the test so it will be more sensitive to those kinds of changes.
   - Refactor the CompareImage code to use more widely (removing its
     behavior of saving the image to compare).
   - Make use of VTK's automatically save rendered image functionality.
   - Update callsites to CompareImage accordingly.

This does mean we don't have a test of transparent things. This is not a
problem:
  1. We only accidentally had one before.
  2. We don't support transparency with any kind of uniformity or
     reliability.
  3. We need a test that tracks what we *do* support and document
     supported transparency appropriately (a follow up PR).
@sherm1
Copy link
Member

sherm1 commented Oct 18, 2024

Please choose a feature reviewer if this is ready, or switch to Draft.

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.

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, missing label for release notes (waiting on @SeanCurtis-TRI)

@SeanCurtis-TRI SeanCurtis-TRI added the release notes: none This pull request should not be mentioned in the release notes label Oct 18, 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: none)

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

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.

:lgtm: feature.

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: 3 unresolved discussions, needs at least two assigned reviewers


geometry/render_vtk/test/internal_render_engine_vtk_test.cc line 309 at r1 (raw file):

// located at `ref_filename` to within the given tolerance.
template <typename ImageType>
void CompareImages(const ImageType& test_image, const std::string& ref_filename,

nit This is a resource path, not a filename. Maybe "ref_resource" or "ref_resource_path" or "ref_respath" or something?

Code quote:

ref_filename

geometry/render_vtk/test/internal_render_engine_vtk_test.cc line 996 at r1 (raw file):

          "drake/geometry/render/test/meshes/rainbow_box.obj"))};

  // The camera has a smaller image (640 x 480), a narrower field of view (so

typo

Suggestion:

480 x 360

geometry/render_vtk/test/internal_render_engine_vtk_test.cc line 1040 at r1 (raw file):

    // PBR. OBJs get rendered using Phong materials. It leads to slightly
    // different levels of illumination, requiring a bit more tolerance (about
    // 10%). When looking at the images, they should be qualitatively similar.

BTW Another way to do this would be to use two different goal images (both committed into git), so that tolerance is tight in both cases. Maybe that's better? You can decide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: none This pull request should not be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RenderEngineVtk cloning yields rendering artifacts
3 participants