feat: camera sync canvas relative #167
Conversation
| * to return [0,0] and 1 for the initial values. | ||
| * @param resetOffsets can be passed to skip resetting, ie a no-op on this call | ||
| */ | ||
| protected resetInitialOffsets(resetOffsets = true) { |
There was a problem hiding this comment.
The whole notion of resetInitialOffset does not make sense to me, this function just get the camera and set it as initialCamera. Shouldn't this function be like setInitialCamera? and then resetOffsets be like storeInitialCamera or something else? I don't understand what is being reset here?
There was a problem hiding this comment.
The idea is that it stores the initial camera, so I'm going to call it just that. Will push an update momentarily.
| * Sets the canvas pan value relative to the initial view position of 0,0 | ||
| * Modifies the camera to perform the pan. | ||
| */ | ||
| public setPan(pan: Point2, resetOffsets = false) { |
There was a problem hiding this comment.
again resetOffsets is confusing to me, I don't understand what it does. + function documentation miss arguments
There was a problem hiding this comment.
Renamed to storeInitialCamera
packages/tools/src/synchronizers/callbacks/zoomPanSyncCallback.ts
Outdated
Show resolved
Hide resolved
packages/tools/src/synchronizers/synchronizers/createCameraPositionSynchronizer.ts
Outdated
Show resolved
Hide resolved
packages/tools/src/synchronizers/synchronizers/createZoomPanSynchronizer.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
See my comments, main one is that resetOffsets is vauge and should be renamed to represent what it does which is setting initial camera. initial camera is also something that should be documented more of what it is. If you fix these minor comments we can merge. Thanks
sedghi
changed the title
✅ Deploy Preview for cornerstone-3d-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
sedghi
force-pushed
the
feat/cameraSync-canvas-relative
branch
from
e68a99a to
014fff2
Compare
This feature adds a new zoom/pan sync option, with either setting being able to be turned off.