feat: implement camera positioning for 3D snapshots and add related tests#2050
feat: implement camera positioning for 3D snapshots and add related tests#2050rushabhcodes wants to merge 3 commits intotscircuit:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request introduces a deterministic camera positioning system for 3D PCB snapshot rendering. The implementation adds a new utility function that calculates optimal camera positions based on board or panel dimensions, replacing hardcoded values to ensure consistent rendering across different environments.
Changes:
- Added
getBestCameraPositionutility function for deterministic camera positioning based on board/panel size and center - Updated 3D snapshot rendering to use the new camera positioning function instead of hardcoded values
- Added integration test for larger board 3D snapshot generation
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/shared/get-best-camera-position.ts | New utility function calculating optimal camera positions with deterministic integer values for consistency |
| lib/shared/snapshot-project.ts | Updated to use getBestCameraPosition instead of hardcoded camera values [10, 10, 10] |
| tests/cli/snapshot/snapshot.test.ts | Added integration test verifying 3D snapshot creation for larger boards (40.64mm x 24.13mm) |
| tests/cli/snapshot/snapshots/large-pcb-3d.snap.png | Binary snapshot file for visual regression testing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
imrishabh18
left a comment
imrishabh18
left a comment
There was a problem hiding this comment.
Wait for @seveibar's review
|
|
||
| // Use completely deterministic integer values to ensure identical rendering | ||
| // across all environments (local, CI, different Node versions, etc.) | ||
| const baseDistance = Math.round(maxDimension * 0.8) |
There was a problem hiding this comment.
Maybe these numbers like 0.8 should be assigned a variable
There was a problem hiding this comment.
i think more examples are needed- to me it seems like it may be zoomed out too far, and the correct calculation would perfectly place the board inside the image. This can be done with some complex math
It is an improvement, but we might as well go for the most reliable long-term solution imo- especially because this is a breaking change (snapshots will be updated downstream)
|
also this method should be exported from circuit-json-to-gltf, it should not be in the CLI!!!! |
3 participants
This pull request improves the way 3D PCB snapshots are rendered by introducing a deterministic, board-size-aware camera positioning function. This ensures consistent and optimal camera angles for different board sizes across all environments. Additionally, a new test verifies correct snapshot creation for larger boards.
Camera positioning improvements:
getBestCameraPositioninlib/shared/get-best-camera-position.tsto calculate optimal, deterministic camera positions based on the board or panel size and center, ensuring consistent 3D rendering results.snapshotProjectinlib/shared/snapshot-project.tsto usegetBestCameraPositioninstead of hardcoded camera values, resulting in improved and consistent 3D snapshot images for all board sizes. [1] [2]Testing enhancements:
tests/cli/snapshot/snapshot.test.tsto verify that the snapshot command correctly creates a 3D snapshot for a larger board, improving test coverage for the new camera logic.Before
After