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] Sort based on camera position when using perspective camera #60381

Merged
merged 1 commit into from
Apr 26, 2022

Conversation

clayjohn
Copy link
Member

@clayjohn clayjohn commented Apr 19, 2022

Fixes a sorting bug reported on RocketChat and likely other reported sorting issues.

The core of the problem here is that sorting appeared to be dependant on view angle. This is because Godot is calculating distance based on the distance to the near plane instead of distance to the camera origin. When using a perspective camera, sorting should take place based on distance to the camera origin rather than distance to the camera plane.

This is a small change with big consequences and needs a lot of testing before being merged.

A similar change needs to be made in 4.0 as well (likely in multiple places see: #60253)

Here is how it worked in Godot 2

if (camera_ortho) {
e->depth=camera_plane.distance_to(p_instance->transform.origin);
} else {
e->depth=camera_transform.origin.distance_to(p_instance->transform.origin);
}

Here is how it works in Ogre:
https://github.com/OGRECave/ogre/blob/d302dee15b389cb916dfa13b1e4e55cd8cffaa55/OgreMain/src/OgreNode.cpp#L663-L670

The Unity Docs also suggest using the near plane for sorting when using an orthogonal camera and the origin for perspective https://docs.unity3d.com/ScriptReference/Camera-transparencySortMode.html

Drawing of the problem with distance to plane

Quick sheets

@clayjohn clayjohn added this to the 3.x milestone Apr 19, 2022
@clayjohn clayjohn requested a review from a team as a code owner April 19, 2022 22:06
@clayjohn clayjohn changed the title Sort based on camera position when using perspective camera [3.x] Sort based on camera position when using perspective camera Apr 19, 2022
@lawnjelly
Copy link
Member

lawnjelly commented Apr 20, 2022

Makes sense. 👍

Out of interest, what were the arguments pro / against using distance rather than distance squared, I noticed Ogre didn't do this? Greater precision, or does it interact better with the rest of the sorting which expects linear distance? Not that I think it will make a great deal of difference performance wise for the number of objects typically involved, even if doing it per light etc.

I suppose it could lead to some incorrect swapping of e.g. windows that are close and parallel to the viewer. But this kind of sorting is always very rough anyway and subject to all kinds of issues.

@clayjohn
Copy link
Member Author

Out of interest, what were the arguments pro / against using distance rather than distance squared, I noticed Ogre didn't do this? Greater precision, or does it interact better with the rest of the sorting which expects linear distance? Not that I think it will make a great deal of difference performance wise for the number of objects typically involved, even if doing it per light etc.

I suppose it could lead to some incorrect swapping of e.g. windows that are close and parallel to the viewer. But this kind of sorting is always very rough anyway and subject to all kinds of issues.

My guess is that regular distance is the naive approach and squared distance is the optimized approach. Godot has always just used regular distance, I'm not sure there has ever been arguments in either direction. The only thing to consider is consistency with other areas that rely on depth. In Godot 3.x it wouldn't be very hard to switch to squared distance if we wanted, in Godot 4.0 a lot of different systems rely on the distance (Lod, Occlusion, Geometry Fade, Decals, Clusters) so we would have to be very careful.

@clayjohn
Copy link
Member Author

cc @tinmanjuggernaut This PR may be of interest to you. I'm not sure if you continue to have depth sorting issues, but if you do, this may help.

@Calinou
Copy link
Member

Calinou commented Apr 20, 2022

My guess is that regular distance is the naive approach and squared distance is the optimized approach. Godot has always just used regular distance, I'm not sure there has ever been arguments in either direction.

From my testing, we can use squared distance easily for "hard" cutoffs, but smooth fading should still use standard distance to prevent skewing the fading curve.

@TokisanGames
Copy link
Contributor

cc @tinmanjuggernaut This PR may be of interest to you. I'm not sure if you continue to have depth sorting issues, but if you do, this may help.

Yes, this was an issue if you were standing among two trees with overlapping branches. Turn the camera one way and one set of branches overlaps. Rotate the camera and the other tree pops forward. Does this fix that?

What about depth sorting with transparent objects with MMI? Which is broken/backward last I checked. Does this help that?

@clayjohn
Copy link
Member Author

Yes, this was an issue if you were standing among two trees with overlapping branches. Turn the camera one way and one set of branches overlaps. Rotate the camera and the other tree pops forward. Does this fix that?

Potentially yes. Sorting is no longer camera-orientation dependant so sorting should be stable on camera rotation.

What about depth sorting with transparent objects with MMI? Which is broken/backward last I checked. Does this help that?

MultiMeshInstance doesn't do any sorting of objects within the MultiMeshInstance, so this may still be an issue. This PR won't help sorting within a MultiMeshInstance, but may help sorting between multiple MeshInstances.

@lawnjelly
Copy link
Member

Have approved this as we have to test at some point. One additional possibility is we could make this a project setting to swap between the two modes, even if temporarily? But maybe this would be overkill.

It may cause some slight differences in sorting with e.g. parallel alpha blended polys with a sufficiently different AABB. But maybe the benefits outweigh this.

@akien-mga akien-mga merged commit d063bc4 into godotengine:3.x Apr 26, 2022
@akien-mga
Copy link
Member

Thanks!

@atirut-w
Copy link
Contributor

Does this PR effects transparent object sorting by any chance?

@lawnjelly
Copy link
Member

Does this PR effects transparent object sorting by any chance?

Yes, have you seen any regressions?

@atirut-w
Copy link
Contributor

I don't test alpha/beta and PR versions so idk but I'm happy if it will make transparency sorting more accurate

@clayjohn clayjohn deleted the 3.x-sorting branch April 26, 2022 14:37
@clayjohn
Copy link
Member Author

I don't test alpha/beta and PR versions so idk but I'm happy if it will make transparency sorting more accurate

This should make transparency sorting more accurate :) You may or may not notice a difference in your project though.

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

6 participants