fix(Stretch): Image aspect ratio resets to fit viewport when changing layout after calling setAspectRatio with isFitViewportAfterStretch disabled - #2832
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe camera model now stores and exposes ChangesViewport stretch-fit state
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/RenderingEngine/Viewport.ts`:
- Line 1797: Update the camera update flow around setAspectRatioForVTKCamera and
vtkCamera.setIsFitViewportAfterStretch to resolve isFitViewportAfterStretch once
from cameraInterface, previousCamera, and the default before any aspect-ratio
handling. Apply that resolved value independently of aspectRatio, pass it to
setAspectRatioForVTKCamera, use it in the VTK setter, and persist the resolved
value in updatedCamera.
- Line 2269: Extend the ViewPresentation type to include
isFitViewportAfterStretch, persist the value from getViewPresentation(), and
update setViewPresentation() to pass the stored flag to setAspectRatio(). Use
the target viewport’s current getIsFitViewportAfterStretch() value only when
applying presentations created without the new property.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 68505726-8373-43b7-b1d9-df1514129974
📒 Files selected for processing (3)
packages/core/src/RenderingEngine/Viewport.tspackages/core/src/RenderingEngine/vtkClasses/extendedVtkCamera.tspackages/core/src/RenderingEngine/vtkClasses/vtkSlabCamera.ts
…fter calling setAspectRatio with isFitViewportAfterStretch disabled
a17207e to
abc7362
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@wayfarer3130 Could you please take a look at this PR? |
Context
Viewport.setAspectRatio(aspectRatio, isFitViewportAfterStretch)stretches the camera to a custom aspect ratio. WhenisFitViewportAfterStretchis false, it should keep that stretched ratio instead of fitting the image back to the viewport borders. But earlier, theisFitViewportAfterStretchflag was never saved anywhere. It was used only once insidesetAspectRatioForVTKCamerato calculate the current math and then discarded. The VTK camera object had no memory of this flag.Because of this, while run
resetCamera(), did not know that fit-after-stretch was disabled.resetCamera()hardcodedisFitViewportAfterStretchasfalsewhen recreating the camera. Also,setViewPresentation()(used while restoring layout state) calledsetAspectRatio()without giving the second argument, which set it back to fit-to-viewport by default. So, your custom aspect ratio visually jumped back to fit-viewport behavior every time changed the layout.The PR is incorporated by FlyWheel.io.
Fixes: #2830
Changes & Results
File changed:
extendedVtkCamera.tsgetIsFitViewportAfterStretch()andsetIsFitViewportAfterStretch()to the custom VTK camera interface and implementation. Now the flag is saved directly on the camera model itself asmodel.isFitViewportAfterStretch.vtkSlabCamera.tsgetIsFitViewportAfterStretch()andsetIsFitViewportAfterStretch()type declarations to thevtkSlabCamerainterface.RenderingEngine/Viewport.tsgetIsFitViewportAfterStretch()onViewport, which reads the flag fromgetCamera()(defaults tofalseif not set).getCameraNoRotation()now includesisFitViewportAfterStretch: vtkCamera.getIsFitViewportAfterStretch()in the returned camera state, so it stays available throughgetCamera().setCamera()now callsvtkCamera.setIsFitViewportAfterStretch(isFitViewportAfterStretch)whenever aspect ratio is applied, saving the flag on the VTK camera.resetCamera()now usespreviousCamera.isFitViewportAfterStretch(instead of hardcodedfalse) when recreating the camera, keeping whichever stretch mode was active before.setViewPresentation()now callsthis.setAspectRatio(aspectRatio, this.getIsFitViewportAfterStretch())instead of skipping the second argument, so layout restore also respects the saved flag.Result:
The
isFitViewportAfterStretchflag is now properly saved on the camera instead of being a one-time setting. When you set a viewport aspect ratio usingsetAspectRatio(aspectRatio, false), that stretched ratio stays saved acrossresetCamera()calls and layout restores, even when switching between common, custom, or MPR layouts. The image will no longer automatically snap back to fit-viewport behavior when you change the layout.Before
2830-before.mp4
After
2830-after.mp4
Testing
viewport.setAspectRatio([x, y], false)Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit
Bug Fixes
Enhancements