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

BatchedMesh: fix radix sort stability #28401

Merged
merged 3 commits into from
May 18, 2024
Merged

Conversation

gkjohnson
Copy link
Collaborator

Related issue: #27213

Description

I noticed that the batched mesh demo was flickering when using the radix sort from #27213. This was caused by the "z" values (used to sort) in the render list being negative or greater than camera.far at times, causing over / underflowing the Uint32 values used for radix.

This PR adjusts the z values to be the z distance along the camera forward vector (instead of just the distance between them). And the min and max values are calculated and used to map the values to the Uint32 range for sorting. The changes in the example add ~0.2-0.25 ms to the custom sort function (from ~0.8ms to ~1.0ms) when rendering 20,000 elements. The function could be sped up by using the boundingSphere if it exists to compute the relevant range to map but I'll leave that for another PR.

Here's an example of the flickering. You need to zoom the camera out a bit so the object is clipped by the camera far plane to see it:

batch-sort-flicker.mov

cc @sciecode

@gkjohnson gkjohnson added this to the r165 milestone May 17, 2024
Copy link

github-actions bot commented May 17, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
676.8 kB (167.9 kB) 676.9 kB (167.9 kB) +103 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
455.9 kB (110.1 kB) 455.9 kB (110.1 kB) +0 B

@gkjohnson
Copy link
Collaborator Author

An aside but is anyone else getting "CI Failed" emails when CI succeeded? Is this a new Github CI bug?

image

@linbingquan
Copy link
Contributor

An aside but is anyone else getting "CI Failed" emails when CI succeeded? Is this a new Github CI bug?

I got it also.

图片

@sciecode
Copy link
Contributor

sciecode commented May 17, 2024

PR addresses the problem correctly.

An alternative would be clamping the z distance to far planes, to prevent uint32 overflow wrap-around.

 const factor = 1 / camera.far; // 1 / max_depth 
 for ( let i = 0, l = list.length; i < l; i ++ ) { 
     list[ i ].z = Math.min( Math.max( list[ i ].z * factor, 0 ), 1 ) * ( 2 ** 32 - 1 );
 } 

However there's only one thing that has me scratching my head, why when the distance wrap-around happens the object disappears? Afaik, sorting happens after culling, so we wouldn't be removing objects from the list. Just sorting them to take advantage of early depth for opaque objects. In my head, the object would just be rendered out of order (which does need to be fixed eitherway), but I don't get why they disappear entirely. Do you happen to know what's happening there @gkjohnson ?

Aside from that, PR is good to go.

@sciecode
Copy link
Contributor

sciecode commented May 17, 2024

Ok, I figured out the problem with objects disappearing. There's a bug in radixSort where the comparison sort portion of the code doesn't treat z as signed, so it's duplicating entries and "deleting" others.

In theory we shouldn't be using signed ints, but I'll patch the sort shortly so it just converts values to uint32 instead of mishandling them. At least the bug served to show we were converting the distance incorrectly 😂

@gkjohnson
Copy link
Collaborator Author

Thanks for investigating and figuring it out! Going to merge, then.

@gkjohnson gkjohnson merged commit bac3cc7 into mrdoob:dev May 18, 2024
12 checks passed
@gkjohnson gkjohnson deleted the batch-sort-fix branch May 18, 2024 07:57
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.

3 participants