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

Point light shadows #3051

Merged
merged 17 commits into from
Dec 9, 2021
Merged

Conversation

WilliamLewww
Copy link
Contributor

@WilliamLewww WilliamLewww commented Jul 26, 2021

Currently only directional and spotlights cast shadows. The pull request adds point light shadow capabilities.

Addresses issue #2083

point_shadows

point_shadows

Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
gazebo/rendering/PointLightShadowCameraSetup.cc Outdated Show resolved Hide resolved
gazebo/rendering/PointLightShadowCameraSetup.cc Outdated Show resolved Hide resolved
gazebo/rendering/PointLightShadowCameraSetup.hh Outdated Show resolved Hide resolved
worlds/point_light_shadows_demo.world Show resolved Hide resolved
worlds/point_light_shadows_demo.world Show resolved Hide resolved
WilliamLewww and others added 2 commits August 17, 2021 10:02
Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
gazebo/rendering/Light.cc Show resolved Hide resolved
gazebo/rendering/PointLightShadowCameraSetup.cc Outdated Show resolved Hide resolved
gazebo/rendering/PointLightShadowCameraSetup.cc Outdated Show resolved Hide resolved
gazebo/rendering/PointLightShadowCameraSetup.hh Outdated Show resolved Hide resolved
Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

@scpeters
Copy link
Member

I just merged with gazebo11, which should fix the ABI checker

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.

@ahcorde
Copy link
Contributor

ahcorde commented Nov 30, 2021

@osrf-jenkins retest this please

@scpeters
Copy link
Member

scpeters commented Dec 1, 2021

I see some ogre log errors:

567: �[0m/var/lib/jenkins/workspace/gazebo-ci-pr_any-ubuntu_auto-amd64-gpu-nvidia/gazebo/test/integration/ogre_log.cc:80: Failure
567: Expected equality of these values:
567:   line.find("error")
567:     Which is: 19
567:   std::string::npos
567:     Which is: 18446744073709551615
567: 13:47:56: Compiler error: reference to a non existing object in shadow_caster_ignore_heightmap.program(21)
567: /var/lib/jenkins/workspace/gazebo-ci-pr_any-ubuntu_auto-amd64-gpu-nvidia/gazebo/test/integration/ogre_log.cc:80: Failure
567: Expected equality of these values:
567:   line.find("error")
567:     Which is: 19
567:   std::string::npos
567:     Which is: 18446744073709551615
567: 13:47:56: Compiler error: invalid parameters in point_light_shadow_demo.material(33)

I think the shadow_caster_ignore_heightmap.program error was not introduced by this PR, but the point_light_shadow_demo.material error looks new. Do you think these are real issues or a problem with the build machine? It would be great if you could fix both of them

@iche033
Copy link
Contributor

iche033 commented Dec 1, 2021

567: 13:47:56: Compiler error: reference to a non existing object in shadow_caster_ignore_heightmap.program(21)

I think vertex_program shadow_caster_vp_glsl just needs to be defined again inside shadow_caster_ignore_heightmap.program

567: 13:47:56: Compiler error: invalid parameters in point_light_shadow_demo.material(33)

That points to this line:

param_named_auto lightPos0 light_position_world_space 0

Does the param light_position_world_space exist? I'm mainly looking at this ogre source file and does not see it defined there.

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

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

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.

CI looks ok to me. I'll wait for a rendering expert to approve though

Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

maybe @iche033 wants to review this. For me it's fine

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 to me, just some minor comments

worlds/point_light_shadows_demo.world Outdated Show resolved Hide resolved
worlds/point_light_shadows_demo.world Outdated Show resolved Hide resolved
gazebo/rendering/PointLightShadowCameraSetup.hh Outdated Show resolved Hide resolved
Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
uniform sampler2DShadow shadowMap4;
uniform sampler2DShadow shadowMap5;

uniform vec4 lightPos0;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove this var? looks like it's not being used

Copy link
Member

Choose a reason for hiding this comment

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

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.

Demo works well for me. Just one minor comment on unused var

@scpeters scpeters self-requested a review December 8, 2021 06:42
@ahcorde ahcorde merged commit 505297a into gazebosim:gazebo11 Dec 9, 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.

None yet

4 participants