Fix camera model transform bounds - #3586
Open
sankhesh wants to merge 2 commits into
Open
Conversation
The camera pose is consumed after modelTransformMatrix, so resetCamera and resetCameraClippingRange must transform world-space prop bounds before deriving the pose and clipping range from them. Ports vtkRenderer::ExpandBounds.
modelTransformMatrix is supplied column-major like every other user-facing matrix in vtk-js, but getViewMatrix composed it against a row-major view matrix, yielding view * transpose(M) instead of view * M. Symmetric matrices such as a pure scale were unaffected, so vertical exaggeration hid the bug; rotations and translations were applied transposed. The previous test asserted the composition in the same mixed convention as the code, so it passed either way. It now checks where world points land in eye coordinates. BREAKING CHANGE: `setModelTransformMatrix` now interprets its argument as gl-matrix column-major, consistent with `vtkTransform`, `vtkMatrixBuilder` and `userMatrix` on `vtkProp3D` — and with what `index.d.ts` already declared (`mat4`). This is a silent change for anyone who passed a rotation or translation and pre-transposed to compensate; pure scales are unaffected.
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.
Context
#3538 fixed the composition order so the transform is applied in world space. But the camera pose is consumed after the transform:
getViewMatrix()buildslookAt(position, focalPoint, viewUp) · M, soposition/focalPoint/clippingRangeare interpreted in transformed space, while prop bounds are in world space. Nothing bridged the two.This is exactly how VTK C++ behaves —
vtkCamera.cxxcomposesModelView = ViewTransform · ModelTransformMatrix, andvtkCamera.hdocuments that "Clipping distance is measured in world coordinate unless a scale factor exists in camera's ModelTransformMatrix." The difference is that C++ hasvtkRenderer::ExpandBounds(vtkRenderer.cxx), which pushes bounds through the transform before deriving the camera pose from them, called fromResetCamera,ResetCameraClippingRangeandResetCameraScreenSpace. vtk-js had no equivalent.While adding it, I found a second, unrelated bug: the model transform was being applied transposed.
Results
resetCamera/resetCameraClippingRange. Before, both derived the camera pose and clipping range from untransformed bounds. With a 10x Z exaggeration on bounds[-1000, 1000, -1000, 1000, 90, 110],resetCameraaimed the camera at world Z 100 while the rendered scene center was at 1000 — the scene was mis-framed and clipped. The error is proportional to(factor − 1)and vanishes at 1x, which is why it went unnoticed. After, the camera is aimed at the transformed center and the clipping range covers the transformed scene.Transposed model transform.
getViewMatrixcomposedview · Mᵀinstead ofview · M. Probing a camera at(0,0,10)looking at the origin, withM = translate(1, 0, 0)fromvtkTransform:A pure scale is symmetric, so
M = Mᵀand vertical exaggeration worked either way — but rotations and translations were applied transposed.Changes
Behavior change:
setModelTransformMatrixnow interprets its argument as gl-matrix column-major, consistent withvtkTransform,vtkMatrixBuilderanduserMatrixonvtkProp3D— and with whatindex.d.tsalready declared (mat4). This is a silent change for anyone who passed a rotation or translation and pre-transposed to compensate; pure scales are unaffected.PR and Code Checklist
semantic-release commit messages
Run
npm run reformatto have correctly formatted codeThis change adds or fixes unit tests
Tested environment: