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 sorting for objects with positive view z #28474

Merged
merged 1 commit into from
Jun 6, 2024

Conversation

eiriklegernaes
Copy link
Contributor

Description

Currently, if an object with positive zview (i.e. center "behind" the camera, but bounds still intersect frustum) is rendered using perspective projection, the computed zndc will be positive, and sorting will not work as intended. This means it'll likely be drawn at the very end, leading to sub-optimal early depth testing and occlusion queries.

This PR changes the sorting input from zndc to -zview, which I think will work without breaking existing default and (hopefully) custom sorting.

Thoughts?

sorting_issue

Copy link

github-actions bot commented May 23, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
677.8 kB (168 kB) 677.8 kB (168 kB) +41 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
455.8 kB (110.1 kB) 455.9 kB (110.1 kB) +41 B

@gkjohnson
Copy link
Collaborator

Interesting find. I think if this is going to be adjusted then the best way to do it is to multiply the position vector by the projection matrix without dividing by the w component which is what's causing the asymptote and sign flip. If we just sort based the z value in the local camera coordinate frame then the sort order will not be correct if the projection matrix / frustum is rotated - either from use manually setting it or some WebXR cases.

It occurs to me that my recent fix to BatchedMesh sorting in #28401 will have the same issue so we'll want to fix that, as well.

@eiriklegernaes
Copy link
Contributor Author

eiriklegernaes commented May 24, 2024

Makes sense! Thanks for the input, @gkjohnson. I've pushed a change that attempts to implement this. Do you see any obvious problems?

edit: Well.. e2e tests say I goofed up.. I'm guessing skipping the w-divide for the combined transform is not the same as just skipping it for the perspective transform.. (?).

@mrdoob
Copy link
Owner

mrdoob commented May 24, 2024

Seems like the examples that broke all use OrthographicCamera and/or Sprite.

@eiriklegernaes
Copy link
Contributor Author

eiriklegernaes commented May 25, 2024

Okey. I had to make some silly mistakes, but we should be pretty close now. Both the transform from object to world space, and from world to view space are affine transforms, so there's no need to divide by w after doing those.

Comment on lines 1310 to 1311
_vector3.setFromMatrixPosition( object.matrixWorld );
_vector4.copy( _vector3 )
Copy link
Collaborator

@gkjohnson gkjohnson May 26, 2024

Choose a reason for hiding this comment

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

I'm wondering if we should add Vector4.setMatrixFromPosition or Vector4.setFromMatrixColumn so we can facilitate this kind of use more cleanly.

cc @mrdoob

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's focus on getting this PR correct, first. If we sort in camera space, this is a non-issue.

Copy link
Collaborator

@gkjohnson gkjohnson May 28, 2024

Choose a reason for hiding this comment

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

Can you explain how the previous commits were not correct? It was sorting in the camera clip space (before w divide) which is arguably more correct since this is where the triangle projection and rasterization is actually happening. The fundamental issue is that dividing by w can flip the sign of the object origin used to sort.

This is what was done before the commits were overwritten:

_vec3.setFromMatrixPosition( object.matrixWorld );

// applying matrix 4 to vec 3 implicitly divides by w
_vec4.copy( _vec3 ).applyMatrix4( _projScreenMatrix ); 

// sort on _vec4.z ...

Copy link
Collaborator

@gkjohnson gkjohnson Jun 3, 2024

Choose a reason for hiding this comment

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

One comment on the code here is that if we don't want to add a new function for vector4 to get the translation column is that this can still be written in one line like so, though I'll defer to others preferences on style here:

_vec4.set( 0, 0, 0, 1 )
  .applyMatrix4( object.matrixWorld )
  .applyMatrix4( _projScreenMatrix ); 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having a Vector4.setFromMatrixPosition makes sense in my mind, at least. If we don't want to add that, I'd say we should avoid the extra vector-matrix multiplication (which due to formatting still adds a line..), and just use the copy function.

Copy link
Owner

Choose a reason for hiding this comment

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

Added Vector4.setMatrixFromPosition sounds good to me 👍

@WestLangley
Copy link
Collaborator

If we just sort based the z value in the local camera coordinate frame then the sort order will not be correct if the projection matrix / frustum is rotated - either from use manually setting it or some WebXR cases.

I'd suggest following convention and sorting in camera space. If someone can demonstrate doing so is a problem, we can fix it.

@gkjohnson
Copy link
Collaborator

gkjohnson commented May 28, 2024

I'd suggest following convention and sorting in camera space. If someone can demonstrate doing so is a problem, we can fix it.

Unless the projection matrix is "facing" exactly along -Z (which there is no restriction for) then this will result in an incorrect sort order.

At JPL our camera models treat +X as boresight which we used to preview a camera view. Even then there is no guarantee that the derived camera models are not rotated or off axis. The three.js integration with mapbox gl also includes translation and rotation in the projection matrix, as well, which this will break transparent rendering for.

I'll have to check some of my other WebXR camera frustums but my point is that there are real use cases that sorting in camera local space rather than clip space will break. And I'm not sure if I see any value added in requiring -Z to be the center direction for custom projection matrices.

@eiriklegernaes
Copy link
Contributor Author

Would it be possible to say that these non-conventional use cases just require custom sorting functions? This will of course break (a tiny portion of?) existing code.

Let me know which way you want to go on this, and I'll adjust the PR accordingly.

@gkjohnson
Copy link
Collaborator

gkjohnson commented May 28, 2024

Would it be possible to say that these non-conventional use cases just require custom sorting functions?

There's a reliable, generalized solution to the problem so I'm not sure why custom sorting should be needed - and this is a case that already works currently. The solution that was implemented previously (referenced in #28474 (comment)) should produce correct results, unless I'm missing something, and had passing tests.

Let me know which way you want to go on this, and I'll adjust the PR accordingly.

It's worth letting the discussion play out before making more changes to the PR. Thank you for the changes!

@WestLangley
Copy link
Collaborator

tldr;

Sorting in ndc-space was introduced in three.js in 2011. There have been no complaints, but the OP is correct, the sort function is discontinuous, which can lead to incorrect sort order in certain edge cases.

Sorting in camera space is the convention. I am not aware of any engines sorting in clip-space. Does someone know of one?

@gkjohnson You may be correct that sorting in clip-space is better, but I do not like your example. In the mapbox example, the camera projection matrix is a function of the object's transform. That seems to me to be a very strange edge case. Also, I am aware of the concept of skewing a projection matrix, but not of the concept of rotating one.

Also to consider, I would expect we want to ensure that the left and right eye render with the same sort order.

@gkjohnson
Copy link
Collaborator

gkjohnson commented Jun 2, 2024

I'm really surprised there's so much resistance to just doing the math here in the correct space. If we just rely on mathematical consistency we don't have to worry about arbitrarily imposing restrictions. The point is you do not always have control over the construction of the projection matrix - particularly when it comes from another system. The solution I've suggested is correct and the one proposed only serves to break existing use cases because we don't think a user is using it in a way we want. There hasn't really been any other reason given.

I and a lot of other users have likely written a lot of code that this project wouldn't deem the "correct" way of doing things and the fact that it works is great. The flexibility of three.js is why I and many others use it in the first place - the fact that these things work and that users can rely on the mathematics being applied in consistent ways if they know what's going on is a feature, not a bug.

Also to consider, I would expect we want to ensure that the left and right eye render with the same sort order.

If we were sorting objects twice for VR rendering that would be a reason enough to address this separately for the sake of performance but this isn't happening. Objects from ArrayCameras are sorted once before they each camera is rendered, so this is not an issue (I know I originally suggested this might apply to VR, my mistake).

@eiriklegernaes
Copy link
Contributor Author

While sorting in view space may be the convention, I find it hard to argue against @gkjohnson's points. As far as I can tell, sorting in view and clip space have similar runtime cost and code complexity. But one imposes more restrictions for no apparent benefit.

I was a little quick to force push, but here's the diff for clip space sorting:
eiriklegernaes/three.js@6c8b877...fix-sorting-clip-z

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 3, 2024

While sorting in view space may be the convention, I find it hard to argue against @gkjohnson's points. As far as I can tell, sorting in view and clip space have similar runtime cost and code complexity. But one imposes more restrictions for no apparent benefit.

Indeed. I vote for clip space.

@mrdoob
Copy link
Owner

mrdoob commented Jun 6, 2024

I think we can give this a go.

@mrdoob mrdoob added this to the r166 milestone Jun 6, 2024
@mrdoob mrdoob merged commit b1e08ab into mrdoob:dev Jun 6, 2024
22 checks passed
@eiriklegernaes
Copy link
Contributor Author

@mrdoob The PR currently contains the view-space sorting. It seems most people now lean towards the clip-space sorting, so I'll have to update this PR prior to merging

@mrdoob
Copy link
Owner

mrdoob commented Jun 6, 2024

Reverted!

@eiriklegernaes
Copy link
Contributor Author

Doesn't seem to be a way to repoen this PR, so I opened #28571, with clip space sorting and Vector4.setFromMatrixPosition

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants