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 distance to camera in light and decal distance fade calculations #60253

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Apr 14, 2022

This also uses a smoother fading formula for Decal distance fade (same as OmniLight and SpotLight), superseding #58298 in the process.

I found this fix while working on a 3.x backport of #58512.

This closes #58513.

Testing project: test_light_decal_distance_fade.zip

@Calinou Calinou requested a review from a team as a code owner April 14, 2022 22:42
@Calinou Calinou added this to the 4.0 milestone Apr 14, 2022
@Calinou Calinou force-pushed the distance-fade-use-distance-to-camera branch 2 times, most recently from 46666aa to 449bf91 Compare April 14, 2022 22:58
}
}

cluster.omni_light_sort[cluster.omni_light_count].instance = li;
cluster.omni_light_sort[cluster.omni_light_count].depth = distance;
cluster.omni_light_sort[cluster.omni_light_count].depth = depth;
Copy link
Member

Choose a reason for hiding this comment

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

Is there a compelling reason to use camera depth here instead of just using distance everywhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know if the clustering code expects the distance to be "flat" (relative to the camera frustum). I kept it this way to avoid regressions.

@clayjohn
Copy link
Member

clayjohn commented Apr 26, 2022

One problem we are going to face here is that we don't propogate camera information all the way through the rendering pipeline. We don't even know if the camera is perspective or orthogonal. We may have to pass more information down to the functions to make sure that we properly compute distance in both cases

Edit: actually it wont be too bad, we can just propogate more information, everything we need is in the RenderData struct

This also uses a smoother fading formula for Decal distance fade
(same as OmniLight and SpotLight).
@Calinou Calinou force-pushed the distance-fade-use-distance-to-camera branch from 449bf91 to abba682 Compare August 23, 2022 14:33
@Calinou
Copy link
Member Author

Calinou commented Aug 23, 2022

Rebased and tested again, it works as expected. I added a testing project to OP as well for testing all kinds of distance fade (including GeometryInstance3D visibility range).

We don't even know if the camera is perspective or orthogonal.

Distance fade currently appears broken when the camera is orthogonal, but this is also an issue with GeometryInstance3D visibility range in master. We could try fixing all three in this PR, or do it in a separate PR.

@clayjohn
Copy link
Member

As discussed here and in my comment above, we need to maintain the old behaviour for orthogonal cameras. The old depth calculation is correct for orthogonal cameras. My guess is that even the clustering code should use distance to camera instead of plane, even when using a perspective camera, but that theory will need to be tested

In 3.x the code is:

if (p_cam_orthogonal) {
	ins->depth = near_plane.distance_to(aabb_center);
} else {
	ins->depth = p_cam_transform.origin.distance_to(aabb_center);
}

// Out of range, don't draw this light to improve performance.
continue;
}
if (distance > Math::pow(fade_begin + fade_length, 2)) {
Copy link
Member

Choose a reason for hiding this comment

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

Its probably better to do a multiply instead of a pow() call. Looking around online it sounds like GCC with -O3 optimizes pow(x, 2) to about the same as x*x but many other combinations of compiler/settings don't

@clayjohn
Copy link
Member

I really want to get this in before release, so are you okay if I make a PR to your branch to fix up my comments and make a few other little changes?

@clayjohn clayjohn self-assigned this Jan 13, 2023
@Calinou
Copy link
Member Author

Calinou commented Jan 16, 2023

I really want to get this in before release, so are you okay if I make a PR to your branch to fix up my comments and make a few other little changes?

Sure 🙂

@clayjohn
Copy link
Member

Superseded by #71709

@clayjohn clayjohn closed this Jan 20, 2023
@clayjohn clayjohn removed this from the 4.0 milestone Jan 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants