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

Optionally disable "render back faces" for the shadow caster #3117

Merged

Conversation

WilliamLewww
Copy link
Contributor

Added a new SDF parameter ignition:shadow_caster_render_back_faces to specify if back faces should be rendered by the shadow caster.

<scene>
  <ignition:shadow_caster_render_back_faces>0</ignition:shadow_caster_render_back_faces>
  <background>0.0 0.0 0.0 1</background>
</scene>

Currently, the scene is hard-coded to always render back faces.

Based off pull request: #3048

@scpeters scpeters requested a review from iche033 October 7, 2021 17:48
Copy link
Contributor

@iche033 iche033 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, just some minor comments

gazebo/physics/WorldPrivate.hh Outdated Show resolved Hide resolved
gazebo/physics/World.cc Outdated Show resolved Hide resolved
this->dataPtr->shadowCasterRenderBackFaces =
this->dataPtr->sdf->GetElement("scene")->
Get<bool>("ignition:shadow_caster_render_back_faces");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks fine. I think if we plane to add more shadow params in the future, we can consider adding a <shadows> </shadows> section to group these parameters

@scpeters
Copy link
Member

scpeters commented Oct 7, 2021

we could consider adding this new field to scene.proto in the ign-msgs main branch, similar to gazebosim/gz-msgs#179

Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

just made one very minor comment. Otherwise looks good.

Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

we included a test in #3048. would it be hard to do so here as well?

gazebo/physics/World.cc Outdated Show resolved Hide resolved
…terMaterialName

Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
@WilliamLewww
Copy link
Contributor Author

we included a test in #3048. would it be hard to do so here as well?

I added a test for the shadow caster render back faces.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
Copy link
Member

we included a test in #3048. would it be hard to do so here as well?

I added a test for the shadow caster render back faces.

thanks! the test looks great. I just fixed one more xml glitch in ef25689 and will merge once CI has finished again

@scpeters
Copy link
Member

we included a test in #3048. would it be hard to do so here as well?

I added a test for the shadow caster render back faces.

thanks! the test looks great. I just fixed one more xml glitch in ef25689 and will merge once CI has finished again

unfortunately the new test is failing on macOS

I'll try to reproduce and debug the failure on my laptop

@WilliamLewww
Copy link
Contributor Author

we included a test in #3048. would it be hard to do so here as well?

I added a test for the shadow caster render back faces.

thanks! the test looks great. I just fixed one more xml glitch in ef25689 and will merge once CI has finished again

unfortunately the new test is failing on macOS

* https://build.osrfoundation.org/job/gazebo-ci-pr_any-homebrew-amd64/2574/testReport/junit/(root)/ShadowCasterRenderBackFacesTest/ShadowCasterBackFaces/

I'll try to reproduce and debug the failure on my laptop

Could this be the same thing that made the test fail for #3048? I remember resorting to excluding the test on MacOS.

@scpeters
Copy link
Member

the test passes on my laptop; let's just merge it as is

@scpeters scpeters merged commit 04fd631 into gazebosim:gazebo11 Oct 15, 2021
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.

3 participants