Skip to content

perf(Prop3D): only recompute the matrix when the transform changes - #3685

Open
cpruijsen wants to merge 1 commit into
Kitware:masterfrom
cpruijsen:fix/issue-3378
Open

cpruijsen wants to merge 1 commit into
Kitware:masterfrom
cpruijsen:fix/issue-3378

Conversation

@cpruijsen

Copy link
Copy Markdown

Context

vtkActor.onModified triggers computeMatrix() on every modified() event, even when only the visibility or pickability changes. The matrix is already recomputed lazily on access, so the eager recompute is wasted work.

fix #3378

Results

Before: setVisibility(), setPickable() or a vtkProperty change each rebuilt the actor's 4x4 matrix, once eagerly inside onModified and again on the next getMatrix()/computeBounds()/per-frame computeMatrix() call from the OpenGL/WebGPU view nodes, because the staleness guard compared the prop's global mtime.

After: the matrix is rebuilt only when a transform input (origin, position, scale, orientation/rotation, userMatrix) changes. isIdentity is still refreshed on transform changes via the existing onModified subscription, which now early-returns after a single timestamp comparison for unrelated modifications.

Verified by a new regression test in Sources/Rendering/Core/Prop3D/test/testComputeMatrix.js: after setVisibility(false), setPickable(false) and a property opacity change, the matrix's mtime is unchanged across getMatrix()/computeMatrix(); after setPosition/setOrigin/setScale/rotateZ/setOrientation it advances and the matrix content is correct.

Note on approach: the staleness check in computeMatrix() was narrowed to a dedicated transform-inputs mtime rather than only removing the onModified(computeMatrix) subscription. Removing the subscription alone would leave the lazy path recomputing after unrelated modified() calls (the guard keyed on the global mtime), and getIsIdentity() would go stale between a transform change and the next access. The subscription can still be removed if reviewers prefer.

Changes

  • In Sources/Rendering/Core/Prop3D/index.js, vtkProp3D.computeMatrix() compares a new model.transformsMTime against model.matrixMTime instead of the prop's global getMTime().
  • transformsMTime is bumped by setOrigin/setPosition/setScale (via _onXXXChanged model hooks, the same mechanism vtkImageData uses) and by addPosition, rotateX/Y/Z, rotateWXYZ, rotateQuaternion, setOrientation, setOrientationFromQuaternion, setUserMatrix.
  • No public API added, deleted, or changed. vtkFollower.computeMatrix() is unchanged: its matrix depends on the camera, which the transform mtime deliberately does not cover.

PR Checklist

  • GitHub Actions CI passed: semantic-release commit messages, lint, and tests
  • Test coverage added: Sources/Rendering/Core/Prop3D/test/testComputeMatrix.js (fails without the fix: AssertionError: expected 10 to be 2)
  • Documentation and TypeScript definitions are updated to match these changes (no public API change; no .d.ts update needed)

computeMatrix() used the prop's global mtime to decide whether to rebuild
the matrix, so any modified() call - for example setVisibility() - made
it stale and forced a recompute, both eagerly through the onModified
subscription and lazily on the next access.

Track the modification time of the transform inputs (origin, position,
scale, rotation, userMatrix) in a dedicated transformsMTime and use it
as the staleness check. The onModified subscription is kept so
isIdentity stays up to date when a transform actually changes.

fix Kitware#3378

This branch has not been deployed

No deployments
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.

[Perf] Changing actor visibility should not compute actor matrix

1 participant