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

Revert change to default depth clear value in draw_list_begin() #90828

Merged
merged 1 commit into from
Apr 18, 2024

Conversation

clayjohn
Copy link
Member

Fixes: #90627

The reverse Z PR change the default clear depth specified by draw_list_begin(). This was convenient as many uses of draw_list_begin() should be clearing to 0.0 now. However, this broke the lightmapper which uses the RD API outside of the renderer and does not use reverse Z. It will also break any user project which uses the RD API directly and relies on the default clear value.

Ultimately, its better to revert back to the previous default value as this change was technically a breaking change.

@clayjohn clayjohn added this to the 4.3 milestone Apr 18, 2024
@clayjohn clayjohn requested a review from a team as a code owner April 18, 2024 02:12
@akien-mga akien-mga merged commit 9c05ff2 into godotengine:master Apr 18, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@darksylinc
Copy link
Contributor

How is it that an external component can use the RD API without Reverse Z?

Setting the default value to Normal Z when the engine internal doesn't support Normal Z will cause a lot of headaches in the future.

Alternatively introduce draw_list_begin2 and deprecate draw_list_begin

@clayjohn
Copy link
Member Author

How is it that an external component can use the RD API without Reverse Z?

Setting the default value to Normal Z when the engine internal doesn't support Normal Z will cause a lot of headaches in the future.

Alternatively introduce draw_list_begin2 and deprecate draw_list_begin

When a user uses the RD API they create their own framebuffers, render passes etc. So they can use whatever depth representation they want.

The RD API itself doesn't use reverse z or non reverse z, the way that Vulkan doesn't use reverse z or non reverse z. The decision is totally in user space.

@clayjohn clayjohn deleted the RD-draw_list_depth branch April 21, 2024 00:16
@darksylinc
Copy link
Contributor

But isn't the Reverse Z sent through the projection matrix? Or are they dodging the projection matrix (which is almost always a bad idea).

@clayjohn
Copy link
Member Author

They supply their own projection matrix. The RD API is it's own thing. It's just an abstraction layer. We use it for our renderer internally. But users can use it for whatever they like.

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.

Reverse Z depth broke LightmapGI
3 participants