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

Fix inconsistent lighting in GLES2 #30570

Merged
merged 1 commit into from Jul 17, 2019

Conversation

@SonerSound
Copy link
Contributor

SonerSound commented Jul 13, 2019

Fixes #28819.
Tested on the editor with the provided minimal production project. Although this fix definitely works, there might be a need to clear all the light_index's from all LightInstances to make sure this wouldn't occur in other renderers. I might be wrong on that though. Thanks @clayjohn for all the help.

Commit description:

  • Issue was possibily being caused by duplicating a light even when that light was not in the render_light_instances array.
    bugsquad edit: Also closes #26269
@SonerSound SonerSound requested a review from reduz as a code owner Jul 13, 2019
Copy link
Contributor

clayjohn left a comment

I noticed while testing out the behaviour under 3.1.1 that the bug was only present in the editor, and that it was also occurring when selecting different nodes in the editor, not just from moving the camera around. Accordingly, I think that there is something more going on.

This PR does fix the bug though. So it's probably best to merge it for now and then figure out the underlying cause later.

drivers/gles2/rasterizer_scene_gles2.cpp Outdated Show resolved Hide resolved
@Chaosus Chaosus added this to the 3.2 milestone Jul 14, 2019
Issue was possibily being caused by duplicating a light even when that
light was not in the render_light_instances array.
@SonerSound SonerSound force-pushed the SonerSound:gles2_inconsistent_fix branch from af80e6b to 545bf86 Jul 14, 2019
@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Jul 15, 2019

This PR does fix the bug though. So it's probably best to merge it for now and then figure out the underlying cause later.

Won't merging this ad hoc fix mean that we will never investigate nor fix the underlying cause, if its symptoms are now hidden?

@clayjohn

This comment has been minimized.

Copy link
Contributor

clayjohn commented Jul 15, 2019

Won't merging this ad hoc fix mean that we will never investigate nor fix the underlying cause, if its symptoms are now hidden?

Likely yes, but also, this will likely need to be rewritten for 4.0. So this is more like a band-aid until then.

@clayjohn

This comment has been minimized.

Copy link
Contributor

clayjohn commented Jul 15, 2019

@akien-mga After spending some more time on this, I am confident that this is an equivalent solution to the one used in GLES3. GLES3 does something similar, but because of the implementation, GLES3 has to track an additional variable. Because of the way GLES2 is, we can avoid allocating an extra uint64_t and keeping track of the additional variable. However, this solution requires that we compare two light isntances, im not sure how fast that is. The other "GLES3" solution may be slightly faster. I have implemented it here clayjohn@188fe8c

@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Jul 16, 2019

Because of the way GLES2 is, we can avoid allocating an extra uint64_t and keeping track of the additional variable. However, this solution requires that we compare two light isntances, im not sure how fast that is. The other "GLES3" solution may be slightly faster. I have implemented it here clayjohn/godot@188fe8c

This reads slightly more self-documenting to me, though I don't have a strong preference either way. Both you and @SonerSound spent quite some time on this, so I'll let you decide which solution you prefer and we can merge that :)

@clayjohn

This comment has been minimized.

Copy link
Contributor

clayjohn commented Jul 16, 2019

We prefer SonerSounds solution here. Lets merge it. :)

@akien-mga akien-mga merged commit 1d13567 into godotengine:master Jul 17, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Jul 17, 2019

Thanks!

@SonerSound SonerSound deleted the SonerSound:gles2_inconsistent_fix branch Jul 17, 2019
@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Nov 12, 2019

Cherry-picked for 3.1.2.

@EzraT

This comment has been minimized.

Copy link

EzraT commented Nov 13, 2019

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.