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

Use circular fade instead of linear fade for distance fade #50294

Merged

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Jul 8, 2021

master version of #69959.

This makes distance fade look the same regardless of the camera angle, for all distance fade modes (Pixel Alpha, Pixel Dither, Object Dither). Distance fade now behaves like fog in this regard.

This has a minimal performance cost, but the quality increase is worth it. When viewing a plane that fully covers the screen, I get 215 FPS instead of 218 FPS with Pixel Dither in 4K, but this does not reflect the typical use case of distance fade (which is for LOD purposes). Besides, there are likely other ways to improve dithering efficiency (such as reducing branching or using interleaved gradient noise).

Thanks to @mrjustaguy for providing the formula here: godotengine/godot-proposals#2963 (comment)

This can be cherry-picked without conflicts to the 3.x branch. I tested this PR on the 3.x branch on both GLES3 and GLES2, and it works as expected there too. This PR can be merged independently of #50297 as both can work together.

This closes #53853 and partially addresses godotengine/godot-proposals#2963.

Testing project: test_distance_fade_circular.zip

Preview

Note: These images should be viewed on a desktop device at 1:1 resolution by clicking them. Otherwise, they will not look as expected due to the nature of dithering.

Pixel Alpha

Pixel Alpha

Pixel Dither

Pixel Dither

scene/resources/material.cpp Outdated Show resolved Hide resolved
@PolyVector
Copy link

Orthographic projections should still use a linear fade, or it will cause artifacts while panning.

@Calinou
Copy link
Member Author

Calinou commented Jul 26, 2021

Orthographic projections should still use a linear fade, or it will cause artifacts while panning.

I guess I have to apply the same if statement as done here:

code += "\tif (PROJECTION_MATRIX[3][3] != 0.0) {\n";
//orthogonal matrix, try to do about the same
//with viewport size

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.

My suggested changes are functionally equivalent to your code except shorter and much faster.

scene/resources/material.cpp Outdated Show resolved Hide resolved
scene/resources/material.cpp Outdated Show resolved Hide resolved
scene/resources/material.cpp Outdated Show resolved Hide resolved
@Calinou Calinou force-pushed the distance-fade-use-circular-fade branch 2 times, most recently from 0b84915 to ffba867 Compare August 23, 2022 17:01
@Calinou
Copy link
Member Author

Calinou commented Aug 23, 2022

Rebased and tested again, it works as expected for Pixel Alpha and Pixel Dither. However, Object Dither no longer seems to be faded irrespective of camera angle. Rotating the camera without changing its position will change the entire object's opacity, which is unexpected.

Edit (2022-08-31): Fixed thanks to clayjohn's review suggestion.

I added a testing project to OP.

@clayjohn Do your suggestions handle orthogonal camera projections automatically?

Edit (2022-08-31): It seems distance fade makes objects invisible in orthogonal camera, even before this PR. I'm confused at how the distance is supposed to be calculated when using an orthogonal camera anyway.

@Calinou Calinou force-pushed the distance-fade-use-circular-fade branch from ffba867 to 4d077ed Compare August 31, 2022 15:32
@elvisish
Copy link

elvisish commented Dec 1, 2022

Could this be reviewed again for 3.6?

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.

The code changes are fine. My only concern is with the comment which will quickly become out of date.

I am fine with substituting Akien's suggested comment above.

@Calinou Calinou force-pushed the distance-fade-use-circular-fade branch from 4d077ed to 69e42cd Compare December 5, 2022 21:35
This makes distance fade look the same regardless of the camera angle,
for all distance fade modes (Pixel Alpha, Pixel Dither, Object Dither).
Distance fade now behaves like fog in this regard.
@Calinou Calinou force-pushed the distance-fade-use-circular-fade branch from 69e42cd to d926be7 Compare December 5, 2022 21:35
@Calinou
Copy link
Member Author

Calinou commented Dec 5, 2022

The code changes are fine. My only concern is with the comment which will quickly become out of date.

I am fine with substituting Akien's suggested comment above.

Done 🙂

@akien-mga akien-mga merged commit e19ead8 into godotengine:master Dec 6, 2022
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

This can be cherry-picked without conflicts to the 3.x branch. I tested this PR on the 3.x branch on both GLES3 and GLES2, and it works as expected there too.

This has significant conflicts now, so would be worth a dedicated backport.

@Calinou Calinou deleted the distance-fade-use-circular-fade branch December 12, 2022 13:24
@elvisish
Copy link

This can be cherry-picked without conflicts to the 3.x branch. I tested this PR on the 3.x branch on both GLES3 and GLES2, and it works as expected there too.

This has significant conflicts now, so would be worth a dedicated backport.

I thought this was for 3.x? This was started before 4.0 was even released.

@Calinou
Copy link
Member Author

Calinou commented Dec 12, 2022

I thought this was for 3.x? This was started before 4.0 was even released.

The code in this PR changed quite a bit compared to its first revision, and master started diverging more and more over time. The more time goes on, the fewer PRs can be cleanly cherry-picked from master to 3.x.

@elvisish
Copy link

I thought this was for 3.x? This was started before 4.0 was even released.

The code in this PR changed quite a bit compared to its first revision, and master started diverging more and more over time. The more time goes on, the fewer PRs can be cleanly cherry-picked from master to 3.x.

There were some original pushes last year that never made it, surely those would have been relevant for 3.x then as they would be now? @clayjohn did a revision at some point, what caused it to be shelved until 3.x became non-priority?

@Zireael07
Copy link
Contributor

@elvisish Many PRs take a looong time to get merged, for whatever reason (personally I think it's the sheer number of PRs open vs small number of contributors who can merge)

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.

Material distance fade is based on depth, not distance from the camera
6 participants