Glyphmapper incorrect shift scale bug #3374
Open
Conversation
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
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
approved these changes
Dec 2, 2025
Member
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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
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