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

Stereo rendering: Fix omni lights #92186

Merged
merged 1 commit into from
May 22, 2024

Conversation

BastiaanOlij
Copy link
Contributor

@BastiaanOlij BastiaanOlij commented May 21, 2024

This one was super dumb.

Turns out setting stereo rendering for sky set uniform location 5 to the uniform buffer containing Multiview data for the sky.
Geometry rendering however uses this to access the omni light buffer.
The result is that the rendering was accessing the incorrect data.

@BastiaanOlij BastiaanOlij added bug topic:rendering topic:xr topic:3d cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels May 21, 2024
@BastiaanOlij BastiaanOlij added this to the 4.3 milestone May 21, 2024
@BastiaanOlij BastiaanOlij self-assigned this May 21, 2024
@BastiaanOlij BastiaanOlij requested a review from a team as a code owner May 21, 2024 04:26
@BastiaanOlij
Copy link
Contributor Author

@clayjohn I think this could use a rework at some point so that we rebind the correct uniform buffers after we perform the sky process, but for now I've solved it the same way this was already solved for the conflict with SCENE_DATA_UNIFORM_LOCATION and SKY_DIRECTIONAL_LIGHT_UNIFORM_LOCATION.

@akien-mga
Copy link
Member

#82278 might be the same issue, does this fix it too?

@decacis
Copy link
Contributor

decacis commented May 21, 2024

Can confirm this fixes the issue!

4.3-dev6 This PR
Lionfish-First-Appropriate Quagga-Blind-Nervous

Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

I just tested if this fixes issue #82278 (using the MRP from issue #89662) and it does! (I've updated the description to add that issue)

Other than a small issue in a comment, the code changes look good to me!

drivers/gles3/rasterizer_scene_gles3.h Outdated Show resolved Hide resolved
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

I agree this has gotten quite ugly. We should probably reorganize at some point, but this change is fine for now

@wiseblank
Copy link

Awesome debugging, been troubled by this as well.

@akien-mga akien-mga added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label May 22, 2024
@akien-mga akien-mga merged commit 256f52f into godotengine:master May 22, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@BastiaanOlij I see you flagged this for a 4.2 cherrypick, which makes sense. Do you think it would also cherry-pick and work properly for 4.1?

@BastiaanOlij
Copy link
Contributor Author

@akien-mga I think this has been in there for a long time so probably yes, but I think someone might want to test that hypothesis.

@BastiaanOlij BastiaanOlij deleted the fix_stereo_omni_lights branch May 22, 2024 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release topic:rendering topic:xr topic:3d
Projects
Status: Very Bad
6 participants