Simplify the unified CPU sort params after the non-uniform scale fix - #9270
Merged
Merged
Conversation
The radial sort path never reads `transformedDirection`, so the ternary introduced in #9268 had a dead arm. Always post the exact per-axis weights and branch only on the scale, which the radial path still needs to convert local distances to world units. Also rename `uniformScale` to `scale`, since it is 1 on the linear path, and pass the already-cached `radialSort` to `setSortParams` rather than re-reading the scene flag.
Build size reportThis PR changes the size of the minified bundles.
|
Contributor
There was a problem hiding this comment.
Pull request overview
Cleans up the unified CPU gsplat sort parameter construction after the non-uniform scale fix, aiming to simplify semantics and reduce redundant state reads while keeping sort behavior unchanged.
Changes:
- Makes
transformedDirectionconsistently represent the per-axis weights (and renamesuniformScale→scale). - Passes the cached
radialSortflag through tosetSortParamsto avoid mismatched reads. - Removes the now-dead radial/linear meaning split in the
transformedDirectionconstruction (though there’s still avoidable work in radial mode; see comments).
Suppressed comments (2)
src/scene/gsplat-unified/gsplat-manager.js:837
transformedDirectionis still computed and a Vec3 allocated even whenradialSortis enabled, but the worker’s radial path never readstransformedDirection(it only usestransformedPositionandscale). This adds unnecessary dot products and allocation in radial mode.
const transformedDirection = new Vec3(
modelMat.getX(splatAxis).dot(cameraDirection),
modelMat.getY(splatAxis).dot(cameraDirection),
modelMat.getZ(splatAxis).dot(cameraDirection)
);
src/scene/gsplat-unified/gsplat-manager.js:857
modelMat: modelMat.data.slice()is never read by the sort worker (nomodelMatusage ingsplat-unified-sort-worker.js), but it allocates a new array and increases per-frame message payload for every splat placement. Removing it should reduce GC and postMessage overhead without changing behavior.
scale,
modelMat: modelMat.data.slice(),
aabbMin: [aabbMin.x, aabbMin.y, aabbMin.z],
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ); | ||
| // the radial path multiplies local-space distances by this to get world units; the | ||
| // linear path has the scale folded into the per-axis weights below, so it needs none | ||
| const scale = radialSort ? modelMat.getScale().x : 1; |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up cleanup to #9268. No behavioural change to either sort path.
Changes:
transformedDirectionternary. The radial path dispatches toevaluateSortKeysRadial/computeEffectiveDistanceRangeRadial, neither of which readstransformedDirection— onlytransformedPositionandscale— so theinvModelMat.transformVector(...).normalize()result was never consumed.transformedDirectionnow always carries the exact per-axis weights, and onlyscalebranches on the sort mode. This drops the two-convention coupling where the meaning of a posted field depended on a flag interpreted in another file.uniformScaletoscale, since it is1on the linear path now that the scale is folded into the per-axis weights.setSortParamsis passed the already-cachedradialSortinstead of re-readingthis.scene.gsplat.radialSorting, so the params and the mode flag cannot be built from different reads.Performance:
lastState.splatscounts placements, not gaussians.Note on #9268: worth recording that the CPU worker's per-splat AABB range and the GPU path's
computeDistanceRangenow compute the same quantity. Before #9268 they disagreed under non-uniform scale, since the worker projected the local AABB with the erroneous1/sᵢweights while the GPU path already transformed corners with the full matrix.