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

RenderEngineVtk can be forced into PBR mode #22170

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 Nov 13, 2024

Adds a new parameter to RenderEngineVtkParams. Rather than waiting for the engine's lighting model to get promoted based on its content, it can be pushed into that mode upon construction.

This also fixes a bug in which cloned instances forgot whether or not phong materials should be automatically promoted to PBR.


This change is Reviewable

@SeanCurtis-TRI SeanCurtis-TRI added release notes: fix This pull request contains fixes (no new features) release notes: feature This pull request contains a new feature labels Nov 13, 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) for the new RenderEngineVtkParams field.
+(release notes: fix) for the correction in post-clone behavior.

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

@jwnimmer-tri
Copy link
Collaborator

FYI The change-detector test render_engine_vtk_params_test needs a small fix now for CI.

@SeanCurtis-TRI
Copy link
Contributor Author

-- commits line 15 at r2:
This introduces some cruft so I could better assess the nature of the CI test failure.

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.

Blast! Schrodinger's CI strikes again. Instrumenting it so I could see the cause of failure, made the failure go away. :-P I'm going to try some quick variants to see where the transition is.

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)

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: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)


geometry/render_vtk/test/internal_render_engine_vtk_test.cc line 2128 at r3 (raw file):

  Render(__LINE__, "clone with ball", clone, &camera, &clone_image, &depth,
         &label);
  EXPECT_EQ(clone_image, pbr_image);

Increasing the image size and removing the streaming, the test passed in R3. So, if we go all the way back to the original image size, what happens next?

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: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)


geometry/render_vtk/test/internal_render_engine_vtk_test.cc line 2128 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Increasing the image size and removing the streaming, the test passed in R3. So, if we go all the way back to the original image size, what happens next?

It fails at the original size....

Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

Good day, @SeanCurtis-TRI . This PR does not yet have a reviewer assigned. Is it ready for review yet? If yes, then please assign a feature reviewer. If not, then please label it “status: do not review”.

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

Adds a new parameter to RenderEngineVtkParams. Rather than waiting for the
engine's lighting model to get promoted based on its content, it can be
pushed into that mode upon construction.

This also fixes a bug in which cloned instances forgot whether or not
phong materials should be automatically promoted to PBR.
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: needs platform reviewer assigned, needs at least two assigned reviewers


geometry/render_vtk/test/internal_render_engine_vtk_test.cc line 2128 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

It fails at the original size....

For now, a todo and a reduced size.

@SeanCurtis-TRI
Copy link
Contributor Author

-(status: do not review)

@sherm1
Copy link
Member

sherm1 commented Nov 21, 2024

Is this ready for feature review now, Sean?

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.

+@zachfang for feature review, please.

Reviewable status: LGTM missing from assignee zachfang, needs platform reviewer assigned, needs at least two assigned reviewers

Copy link
Contributor

@zachfang zachfang 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 2 of 3 files at r1, 1 of 3 files at r2, 2 of 2 files at r5.
Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers


geometry/render_vtk/test/internal_render_engine_vtk_test.cc line 1966 at r5 (raw file):

//  3. RenderEngineVtkParams::force_to_pbr is set to `true`.
//
// (1) is confirmed by this test.

nit.

Suggestion:

Is confirmed

geometry/render_vtk/test/internal_render_engine_vtk_test.cc line 2042 at r5 (raw file):

// a promotion criterion.
TEST_F(RenderEngineVtkTest, PbrMaterialSettingSurvivesCloning) {
  // TODO(SeanCurtis-TRI) Inexplicably, the full size (640x480) cameras causes

nit. camera causes or cameras cause.

Code quote:

 cameras causes

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 release notes: fix This pull request contains fixes (no new features)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants