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 volumetric fog NaN values in textures from starting at a zero Vector2. #80992

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

viksl
Copy link
Contributor

@viksl viksl commented Aug 25, 2023

Reported issues: #78500

This attempts to fix these issues in the comments: #78500 (comment) and #78500 (comment) it might also fix the original issue there but I can't reproduce it on my side and I also couldn't find anyone else who could apart from the OP and Calinou.

Details:
At this line:

view_pos.xy = (fog_unit_pos.xy * 2.0 - 1.0) * mix(params.fog_frustum_size_begin, params.fog_frustum_size_end, vec2(fog_unit_pos.z));
fog_unit_pos.z can be 0 which leads to the mix mix(params.fog_frustum_size_begin, params.fog_frustum_size_end, vec2(fog_unit_pos.z)) resolving into params.fog_frustum_size_begin which comes from this line:
fog_near_size = Vector2();

Here the fog_near_size = Vector2(); means it's zero which later on in every light calculation in the shader casues this: normalize(view_pos) to yield INF/-INF which is later used for calculations which are then stored in textures and result into NaN in textures. Due to temporal_reprojection's interpolation with previous frame these values are kept and spread more resulting in black artifacts like these in the video I captured here: https://youtu.be/Vz-fKhp5scQ

To reproduce this I needed to up the Volume Depth in Project Settings and increase Detail Spread in the volumetric fog otherwise it's difficult to reproduce since it can take longer time to happen, since all these result in similar visual glitches as I mentioned earlier this might also fix the original issue, I did manage to kind of reproduce it but had to do some jump arounds so I'm not 100% sure, hopefully @Calinou and @Jonne-G can test this and tell us if this helped the issue as well or if it only helps with the ones I mentioned, please?

As far as I can tell this is as far as the issue can be tracked, so far I've tested on Windows 10, GTX 1660TI and could not reproduce the issues after this fix while before I can reproduce it 100%.

I couldn't find a reason why Vector2() needs to be used for the start so if someone else knows better please feel free to let me know I'm not that far into udnerstanding the whole thing so maybe I'm missing some reasoning and this isn't a suitable fix?

The 0.001 is just a safety, currently frustum can't go below this but just in case it changes in the future for some reason.
(we could maybe also just set it as Vector2(0.01, 0.01); if the MAX(frustum_near_size) is an issue or do a proper calculation whatever that should be (no idea).

This is my first code PR, please have mercy if I'm making any mistakes here, thank you! ^.^


Side note:
Also I'm not sure about the use of detail_spread:

fog_unit_pos.z = pow(fog_unit_pos.z, params.detail_spread);
this power pretty much always results in 0 depending on settings - Volume Depth and Detail Spread - for example spread 2.0 -> 9.53674E-07, spread 6.0 -> 0.0000 (you can get vallues with exponents of E-13 and such), it's not directly related to this but it was part of the chain which made me wonder if these values are ok in general?

@viksl viksl requested a review from a team as a code owner August 25, 2023 11:01
@Calinou Calinou added bug topic:rendering topic:3d cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Aug 25, 2023
@Calinou Calinou added this to the 4.2 milestone Aug 25, 2023
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 change seems low-risk to me and makes sense conceptually.

@Calinou Can you test this locally and confirm it fixes the bug? I can't reproduce the original bug on my hardware

@viksl viksl force-pushed the volumetric-fog-NaN-issues branch from f239233 to 00c2fb4 Compare August 28, 2023 19:57
@Calinou
Copy link
Member

Calinou commented Aug 28, 2023

@Calinou Can you test this locally and confirm it fixes the bug? I can't reproduce the original bug on my hardware

This PR doesn't appear to resolve the issue on my end (Linux + NVIDIA):

image

Edit: This is fixed by #82546.

@viksl
Copy link
Contributor Author

viksl commented Aug 28, 2023

@Calinou I have no idea how to reproduce that. I even tried different machines with different systems (win 10, win 11, Linux) with different nvidia GPUs. Would you perhaps have an idea about something which could serve as a lead, please?

Could you just for a quick test change the lines in this PR to this please: fog_near_size = Vector2(1.0, 1.0); instead at both places just to see larger value?

@Calinou
Copy link
Member

Calinou commented Aug 28, 2023

Could you just for a quick test change the lines in this PR to this please: fog_near_size = Vector2(1.0, 1.0); instead at both places just to see larger value?

I get the same issue as before with the fog_near_size changed as you said.

@viksl
Copy link
Contributor Author

viksl commented Aug 28, 2023

Thank you, though it's unfortunate. I don't know how to reproduce the original issue, the issues thix fixes just result in similar artifacts. I wish I knew what was the difference for the reproduction but the original poster was on windows 10 (just like me) and you are on Linux. We all have nvidia GPUs, I tried with other people on win and linux and nvidia and amd GPUs and no one but you two could reproduce this so far. :/

Anyone has any ideas perhaps?

EDIT: Also just to be clear, it only happens with spotlights and only at these extreme settings correct?

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.

Looks good. This should be a low risk merge

@akien-mga akien-mga merged commit 7ee2eb5 into godotengine:master Oct 5, 2023
@akien-mga
Copy link
Member

Thanks!

@viksl viksl deleted the volumetric-fog-NaN-issues branch October 5, 2023 21:39
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.3.

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Oct 24, 2023
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.

6 participants