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 draw order of transparent materials with multiple directional lights #47129

Merged

Conversation

mortarroad
Copy link
Contributor

Fixes #47078

@mortarroad mortarroad requested a review from a team as a code owner March 18, 2021 12:30
@akien-mga akien-mga added this to the 3.3 milestone Mar 18, 2021
@mortarroad
Copy link
Contributor Author

Never mind, this doesn't seem to work in all cases yet.

@mortarroad
Copy link
Contributor Author

OK, now it also respects the material priority among multiple transparent shaded materials.
I'm not sure if we really want to support that too; I was mostly concerned about unshaded materials being drawn below shaded ones.
Though it is pretty useful to avoid flickering due to sorting.

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.

On a preliminary review, this looks quite good. I need to spend more time reviewing before I approve though.

I have a few performance concerns in the back of my mind, but I think overall it shouldn't be too bad as not many people use multiple directional lights.

@mortarroad
Copy link
Contributor Author

Another related issue, that does not get fixed by this (i.e. it did not work before, nor does it now):
If a MeshInstance is on a layer with a directional light, but not on the layer with the first directional light in the scene, it will only be drawn in the additive pass.

That means you can't have a scene with two transparent meshes and two lights, with the first mesh being lit by the first light and the second mesh lit by the second light.

Fixing that would require grouping the elements by lights...
We should definitely document that there are issues with transparent objects being lit by directional lights.

Copy link
Contributor

@JFonS JFonS left a comment

Choose a reason for hiding this comment

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

The code makes sense. I did some testing and everything seems to work as expected :)

I don't know if it's worth merging for 3.3 though. On one side, if we are fixing multiple directional lights in 3.3 we might as well do the best we can. On the other, we are introducing more chances for regressions in a late release candidate, for something that is not very widely used...

tl;dr
Looks great, we need to decide if we merge before or after 3.3 :)

@akien-mga akien-mga modified the milestones: 3.3, 3.4 Mar 26, 2021
@akien-mga
Copy link
Member

I think it's better to play it safe and merge for 3.4. The two commits should be squashed into one I guess (or if the second one makes sense as standalone, it should have a clearer title).

@mortarroad mortarroad force-pushed the 3.x-fix-directional-light-order branch from d71bd37 to 63b7b77 Compare March 26, 2021 11:32
@mortarroad
Copy link
Contributor Author

Squashed and rebased to latest 3.x.

@akien-mga akien-mga merged commit 25b1705 into godotengine:3.x Apr 28, 2021
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants