Skip to content

Conversation

@nishantkluhera
Copy link

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

  • Camera Support: Full implementation of orthographic camera
  • Seamless Switchin: Toggle between camera types without losing view context
  • Smart State Preservation: Automatically saves and restores camera position, orientation, and zoom when switching modes
  • Zoom Conversion: Converts perspective FOV to equivalent orthographic zoom for consistent visual scale
  • Dynamic Frustum Sizing: Automatically calculates optimal orthographic frustum based on board dimensions

Testing Scenarios

Camera Type Switching:

  • Perspective → Orthographic: Preserves view, adjusts zoom
  • Orthographic → Perspective: Preserves position and orientation
  • Camera positioning presets work on both camera

seveibar and others added 7 commits October 31, 2025 10:12
- 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
@vercel
Copy link

vercel bot commented Nov 9, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
3d-viewer Ready Ready Preview Comment Nov 11, 2025 9:49pm

}

export const useCameraController = ({
key,
Copy link
Contributor

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

Copy link
Author

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

Copy link
Contributor

@seveibar seveibar left a 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

@seveibar
Copy link
Contributor

seveibar commented Nov 9, 2025

In particular, avoid adding LoC to components like CadViewer or CadViewerContainer

…the camera perset position preview not updating
Comment on lines +174 to +176
const toOrigin = new THREE.Vector3()
.sub(camera.position)
.normalize()
Copy link
Contributor

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

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔

Comment on lines +288 to +294
}, [
scene,
addFrameListener,
removeFrameListener,
cameraProps?.type,
cameraProps?.frustumSize,
])
Copy link
Contributor

@nailoo nailoo Nov 10, 2025

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,
])

onReady: handleControllerReady,
}),
[defaultTarget, handleControllerReady],
// eslint-disable-next-line react-hooks/exhaustive-deps
Copy link
Contributor

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)

Copy link
Author

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)
Copy link
Contributor

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?

Copy link
Author

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

@seveibar
Copy link
Contributor

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

@nishantkluhera
Copy link
Author

fixing the eslint thing after this

Comment on lines +174 to +193
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))
}
}
Copy link
Contributor

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

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

Removed eslint-disable comments for exhaustive-deps.
Copy link
Contributor

@Ayushjhawar8 Ayushjhawar8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image Hey @nishantkluhera All the camera positions are not working / broken, and the view cube also seems to be broken.

@nishantkluhera
Copy link
Author

Yeah I saw that, working on, the conflicts resolutions with the 3 files earlier caused these> Image

Hey @nishantkluhera
All the camera positions are not working / broken, and the view cube also seems to be broken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants