Skip to content
This repository was archived by the owner on Feb 3, 2025. It is now read-only.

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>
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>
@chapulina chapulina requested a review from j-rivero October 11, 2021 18:49
@chapulina chapulina requested a review from ahcorde November 8, 2021 19:47
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>
@WilliamLewww WilliamLewww force-pushed the wlew/point_light_shadows branch from 86db565 to dc83f9a Compare December 1, 2021 22:37
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>
@chapulina chapulina requested a review from scpeters December 6, 2021 19:55
@scpeters scpeters requested a review from ahcorde December 6, 2021 21:23
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

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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants