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

[3.x] Fixes depth sorting of meshes with transparent textures #50721

Merged
merged 1 commit into from
Jul 22, 2021

Conversation

TokisanGames
Copy link
Contributor

@TokisanGames TokisanGames commented Jul 21, 2021

Currently, VisualServer calculates object depth from the camera. Then it overwrites that information with shadow data, then sends the wrong information to the renderer, which is used to sort all objects.

It is incorrect for all objects, but there's likely another reason in OpenGL or the renderer as to why opaque objects are sorted correctly.

This problem becomes apparent with transparent objects, when shadows are calculated. It affects all light types. However Omni and Spotlights have another caveat in that they don't calculate shadows every frame, so it's only wrong on the frames they do.

This PR moves the unused camera depth calculation to after the shadow calculation. The correct information is sent to the renderer which now properly sorts objects based on camera depth.

image

Fixes #43253
See issue for more details.

Bugsquad edit: Fixes #49389.

@TokisanGames TokisanGames requested a review from a team as a code owner July 21, 2021 23:57
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.

Great work tracking down this bug and finding a solution. I am very happy with this fix and it should fix a problem that has been plaguing a lot of users.

As a bonus, I think having the correct depth_layer may result in a small performance improvement.

@akien-mga Let's merge this for the next beta, but we will need to keep our eyes open for depth sorting regressions. To be clear, there shouldn't be any regressions, as the behaviour is now just the same as when there are no lights in the scene.

@Calinou
Copy link
Member

Calinou commented Jul 22, 2021

Is this something we need to fix in master too?

@akien-mga akien-mga merged commit 0c1d52f into godotengine:3.x Jul 22, 2021
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@TokisanGames
Copy link
Contributor Author

TokisanGames commented Jul 22, 2021

Thanks guys.

I saw the 3.3 cherry pick. If you want, I'll make a different pr as the code is slightly different. Should I? 3.3 needs it.

I'll also see about master. I skimmed over the code but haven't found what happened to the visual server yet.

@TokisanGames TokisanGames deleted the fix-alpha-ordering branch July 22, 2021 10:25
@akien-mga
Copy link
Member

Yes if the code changed significantly, a dedicated PR for 3.3 would be welcome. That would prevent me from wrongly resolving the merge conflicts and introducing a bug :)

@EzraT
Copy link

EzraT commented Jul 22, 2021

Many thanks! Very happy to see this one fixed!

This PR has likely also fixed: #49389

@TokisanGames
Copy link
Contributor Author

TokisanGames commented Jul 22, 2021

@akien-mga This change is safe to cherry-pick for 3.3 and works better than the original 3.3 version. (edit: Though you'll need to manually remove the moved lines from above, as they are different enough git won't recognize them).

The distance calculation from 3.3

ins->depth = near_plane.distance_to(ins->transform.origin);
ins->depth_layer = CLAMP(int(ins->depth * 16 / z_far), 0, 15);

The new calculation from 3.x

Vector3 aabb_center = ins->transformed_aabb.position + (ins->transformed_aabb.size * 0.5);
ins->depth = near_plane.distance_to(aabb_center);
ins->depth_layer = CLAMP(int(ins->depth * 16 / z_far), 0, 15);

This new calculation fixes occlusion when the trees are overlapping as shown here in both 3.3 and 3.x:

@akien-mga
Copy link
Member

This code was changed in #43506 (@QbieShay), it should probably be cherry-picked first. But we were waiting for validations that it works as expected in 3.4 betas.

@QbieShay
Copy link
Contributor

Oooh thank you, I missed that.

@TokisanGames
Copy link
Contributor Author

Re: master

Looking through the code it is structured quite a bit differently. I don't see an obvious case where shadow mapping is writing over the same variable as the camera distance.

I was able to recreate some assets and scenes in GD4 to test. It couldn't read any of my previous scenes or assets except the texture files. I don't see any occlusion issues with the trees under any lights with or without shadows.

However, I don't see depth_draw_alpha_prepass in the material options, and the transparent surfaces don't cast any shadows.

There are quite a lot of issues including the floating trunk shadows, no alpha shadows, splotchy dark areas on the alpha meshes. Transmission is gone too. But as it is there is no depth occlusion issue. If depth_draw_alpha_prepass or whatever is necessary to have the transparent meshes draw shadows is put back in, it may present the occlusion issue, but as I wrote, I don't see anything obvious from the code.

image

@Calinou
Copy link
Member

Calinou commented Jul 22, 2021

However, I don't see depth_draw_alpha_prepass in the material options, and the transparent surfaces don't cast any shadows.

See #50124 and other issues.

Transmission is gone too.

It's replaced by Back Lighting 🙂

@akien-mga
Copy link
Member

Cherry-picked for 3.3.3.

@TokisanGames
Copy link
Contributor Author

Other areas I've tested:

GD 4 may have an issue but there's so much changed code (VisualServer where the bug was is gone), and broken code that we can't really test yet and I didn't see anything obvious looking at the code.

Depth sorting is a big problem in MMI, but that is independent from, and out of scope of this change.

The only issue I've seen related to meshes is if you have two trees close to each other maybe overlapping and your player is in the middle. If you turn left one object overlaps, turn right the other overlaps so you get a popping effect. It's odd if you have a long sideways tree overhanging a roof with transparency on the thatch. However, these edge cases are also out of scope and not a big deal compared to what this merge fixed.

@kubajz22
Copy link

kubajz22 commented Aug 8, 2021

Thank you. This issue was the primary reason why I left Godot for Unreal engine. I will consider coming back. Now we just need a terrain editor like Unreal's. :D

@EzraT
Copy link

EzraT commented Nov 16, 2022

I'm guessing developers are probably already aware of this, but just in case, this issue is still present in the latest Godot 4 beta. (Beta5 as of writing)
Video: https://streamable.com/ni92le

@TokisanGames
Copy link
Contributor Author

@EzraT This issue is for 3.5 only, and the problem was z-order of multiple objects. In GD4 it is a problem with faces, as it occurs on the same object.

#63675

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.

None yet

7 participants