Glyphmapper incorrect shift scale bug #3374
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in Glyph3DMapper where the shift-scale transformation was being incorrectly applied to glyph transformation matrices. The fix consolidates the shift-scale matrix calculation into a single source function and uses proper matrix multiplication instead of component-wise operations.
Key Changes:
- Replaced incorrect component-wise shift-scale application with proper matrix multiplication in
Glyph3DMapper - Refactored
computeInverseShiftAndScaleMatrixinto a shared helper function - Fixed GUI controller reference in ResliceCursorWidget example to use lil-gui API
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
Sources/Rendering/OpenGL/Glyph3DMapper/index.js |
Removed incorrect applyShiftScaleToMat function and replaced with proper matrix multiplication using computeInverseShiftAndScaleMatrix helper |
Sources/Rendering/OpenGL/CellArrayBufferObject/helpers.js |
Added computeInverseShiftAndScaleMatrix as an exported helper function with necessary gl-matrix imports |
Sources/Rendering/OpenGL/CellArrayBufferObject/index.js |
Refactored to import computeInverseShiftAndScaleMatrix from helpers module, removing duplicate implementation |
Sources/Widgets/Widgets3D/ResliceCursorWidget/example/index.js |
Updated GUI controller reference to store and use lil-gui controller object instead of DOM element |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
finetjul
left a comment
finetjul
left a comment
There was a problem hiding this comment.
LGTM
It would have been good to explain in git commit message why the transform was previously incorrect.
jadh4v
force-pushed
the
3330-bug-glyphmapper-shift-scale
branch
from
09049c5 to
fd57b90
Compare
Previous function to apply shift and scale transform to the existing glyph matrix was incorrect because it did not account for scaling of rotation components. The glyph matrices can contain rotation components, which when combined with a shift-scale operation will get affected by scaling. It is therefore, better to use proper matrix multiplications rather than trying to recreate the effect through manipulation of individual components of a matrix.
jadh4v
force-pushed
the
3330-bug-glyphmapper-shift-scale
branch
from
fd57b90 to
7a0fea0
Compare
|
I have updated the relevant commit message explaining what was previously wrong with the matrix calculations. No other changes were made since the last review. I will merge after the pipeline passes. |
|
🎉 This PR is included in version 34.16.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
Context
The transformation matrices of each glyph should undergo the same shift-scale transform as the input points array of the
Glyph3DMapper. However, this transform was being incorrectly calculated inGlyph3DMapper. This issue was fixed by ensuring that there is a single source (function) for the calculation of the shift-scale matrix (or its inversion).Results
The glyphs are now again visible in the
ResliceCursorWidgetexample.Changes
PR and Code Checklist
npm run reformatto have correctly formatted codeTesting