-
Notifications
You must be signed in to change notification settings - Fork 39
Feature/orthographic camera toggle #560
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Feature/orthographic camera toggle #560
Conversation
- Fix camera state leaking across circuit changes by only restoring state when explicitly switching camera types - Remove expensive JSON.stringify for viewerKey that caused UI lag - Optimize Vector3 cloning in useFrame to reduce GC pressure - Improve orthographic frustum size calculation using boardDimensions - Ensure stable defaultTarget reference to prevent unnecessary remounts - Combine duplicate useFrame hooks for better performance
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
src/hooks/useCameraController.ts
Outdated
| } | ||
|
|
||
| export const useCameraController = ({ | ||
| key, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove key and make this more meaningful? Key is a bad arg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i could rename it to something like isOrthographic to make it clear what its tracking
seveibar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems too complex, can we simplify this
|
In particular, avoid adding LoC to components like CadViewer or CadViewerContainer |
…the camera perset position preview not updating
| const toOrigin = new THREE.Vector3() | ||
| .sub(camera.position) | ||
| .normalize() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The vector calculation for toOrigin contains a logic error. When initializing an empty vector with new THREE.Vector3() (which creates a zero vector) and then subtracting camera.position from it, the result will be the negative of the camera position. To properly calculate a vector from the origin to the camera, use:
const toOrigin = new THREE.Vector3().subVectors(new THREE.Vector3(), camera.position).normalize()Or more simply:
const toOrigin = camera.position.clone().negate().normalize()This ensures the vector points from the origin toward the camera position.
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔
| }, [ | ||
| scene, | ||
| addFrameListener, | ||
| removeFrameListener, | ||
| cameraProps?.type, | ||
| cameraProps?.frustumSize, | ||
| ]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
}, [
scene,
addFrameListener,
removeFrameListener,
cameraProps?.type,
cameraProps?.frustumSize,
cameraProps?.position,
cameraProps?.up,
])
src/hooks/useCameraController.ts
Outdated
| onReady: handleControllerReady, | ||
| }), | ||
| [defaultTarget, handleControllerReady], | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't use eslint, so you shouldn't need these (make sure you're not using eslint on this project, because the default rules are not compatible with biome)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my bad!! will keep that in mind and also remove those
| ) | ||
| const width = mountRef.current.clientWidth || 1 | ||
| const height = mountRef.current.clientHeight || 1 | ||
| renderer.setSize(width, height) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is stuff like this changing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from what i remember, those specific lines aren't functionally changing, they're appearing in the diff because the entire useEffect was modified to add camera state preservation logic (refs on lines 45-58, state saving on lines 89-114, restoration on lines 142-227). The renderer sizing code itself is unchanged, just showing as context in the larger diff
|
i'm mostly ready to merge this BUT the sideviews are wrong for the camera position https://share.cleanshot.com/7kZbKfxZ also we need to fix the merge conflicts |
|
fixing the eslint thing after this |
| let target: THREE.Vector3 | ||
| if (stateToRestore.target) { | ||
| target = stateToRestore.target.clone() | ||
| } else { | ||
| const forward = new THREE.Vector3(0, 0, -1).applyQuaternion( | ||
| camera.quaternion, | ||
| ) | ||
| const estimatedDistance = camera.position.length() | ||
| const toOrigin = new THREE.Vector3() | ||
| .sub(camera.position) | ||
| .normalize() | ||
| const dotWithForward = forward.dot(toOrigin) | ||
| if (dotWithForward > 0.5) { | ||
| target = new THREE.Vector3(0, 0, 0) | ||
| } else { | ||
| target = camera.position | ||
| .clone() | ||
| .add(forward.multiplyScalar(estimatedDistance)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attempts to access stateToRestore.target but the saved state objects (lines 109-121) never include a target property. This check will always be false, causing the else branch to always execute and potentially calculate incorrect target positions.
// In the state saving logic (lines 109-121), add target:
if (wasOrthographic) {
lastOrthographicStateRef.current = {
position: existingCamera.position.clone(),
quaternion: existingCamera.quaternion.clone(),
up: existingCamera.up.clone(),
zoom,
target: contextState?.controls?.target?.clone() ?? new THREE.Vector3()
}
}Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
Removed eslint-disable comments for exhaustive-deps.
Ayushjhawar8
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @nishantkluhera
All the camera positions are not working / broken, and the view cube also seems to be broken.
|
Yeah I saw that, working on, the conflicts resolutions with the 3 files earlier caused these>
|
Feature: Orthographic Camera Toggle
Overview
Adds orthographic camera support to the 3D viewer with seamless switching between perspective and orthographic projection modes. Users can toggle between camera types while maintaining their current view position and orientation.
Functionality
Testing Scenarios
Camera Type Switching: