Gizmo Normal Snapping #2539
Gizmo Normal Snapping #2539
Conversation
|
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
having some playwright scrrenshot tests for this would be great! |
when the scene first loads, mouse position is 0,0, which renders the gizmo selected.
Maybe, I think there might be a way to test it without screenshots. But tests would be good ofc! |
| let vantage = { x: 0, y: 0, z: 0 } | ||
| let up = { x: 0, y: 0, z: 0 } | ||
|
|
||
| if (axis === 'x') { | ||
| vantage = { x: distance, y: 0, z: 0 } |
There was a problem hiding this comment.
Could we change this so to something like
const vantage = this.target.clone()
let up = { x: 0, y: 0, z: 0 }
if (axis === 'x') {
vantage.x += distance
up = { x: 0, y: 0, z: 1 }
// ...And then for the default camera look at keep the users's target
await this.engineCommandManager.sendSceneCommand({
type: 'modeling_cmd_req',
cmd_id: uuidv4(),
cmd: {
type: 'default_camera_look_at',
center: this.target,
vantage: vantage,
up: up,
},
})Sorry if if this is different from what I originally asked for, but think this will be better as it respects the user's current target, and just rotates the camera to a different perspective.
There was a problem hiding this comment.
now I can see that too ! so much better to code with viewport on :DDD
| async resetCameraPosition(): Promise<void> { | ||
| await this.engineCommandManager.sendSceneCommand({ | ||
| type: 'modeling_cmd_req', | ||
| cmd_id: uuidv4(), | ||
| cmd: { | ||
| type: 'default_camera_look_at', | ||
| center: { x: 0, y: 0, z: 0 }, | ||
| vantage: { x: 0, y: -128, z: 64 }, | ||
| up: { x: 0, y: 0, z: 1 }, | ||
| }, | ||
| }) | ||
| await this.engineCommandManager.sendSceneCommand({ | ||
| type: 'modeling_cmd_req', | ||
| cmd_id: uuidv4(), | ||
| cmd: { | ||
| type: 'zoom_to_fit', | ||
| object_ids: [], // leave empty to zoom to all objects | ||
| padding: 0.2, // padding around the objects | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Sorry I couldn't help but make a commit, just cause I wanted to see this working.
All I did is change the vantage to where the camera is on boot up vantage: { x: 0, y: -128, z: 64 }, The most important thing here is the ratio of y and z (double y).
But immediately followed up by a zoom to fit means it will look exactly the same as when you refresh the page.
Note: default_camera_get_settings is not needed because we already subscribe to the camera payload of zoom_to_fit
There was a problem hiding this comment.
since we fixed my viewport issue today, I could test this one...
In my case it does not work. The camera resets to weird position somewhere far away.
My findings:
1. Unexpected Camera Position: After executing both commands, the camera ended up in a strange position, far from where it should have been. The coordinates before resetting were something like {x: 1719.28, y: -1096.2126, z: 2252.1978} (zoomed and default rotation), but after resetting, they became {x: 1719.28, y: 4877.849, z: -696.26904}, instead of staying the same. Weird math...
2. Testing Commands Individually: When I tested each command (default_camera_look_at and zoom_to_fit) separately, they worked fine on their own. The issue only appeared when both commands were sent one after the other.
3. Pause Between Commands: I suspected that the rapid change from large coordinate values to small ones and back to large values might be causing the issue. I tried adding a short pause (1000ms) between the two commands to give the server time to process the changes, but it didn't help.
4. Eliminating the Coordinate Jump: Instead of using fixed small values for the default_camera_look_at command, I used the current target position for the center and adjusted the vantage point relative to this target. This way, there wasn't a large jump in coordinate values. For example:
center: this.target,
vantage: { x: this.target.x, y: this.target.y - 128, z: this.target.z + 64 }
instead of:
center: { x: 0, y: 0, z: 0 },
vantage: { x: 0, y: -128, z: 64 },
This approach worked and the camera reset correctly without any issues.
but in case of moving camera super far away you might need to click reset 2 times :DD
5. Further Investigation: I found it strange that using the this.target position worked, but hardcoded values did not. It turns out that the vantage value can be any value; the angle may not be default, but the zoom-in will work as expected. The problem lies with the center value. Any value other than this.target causes an offset in the zoom_to_fit command.
There was a problem hiding this comment.
Okay this sounds like a good work around, but maybe the zoom to fit should be robust against this kind of thing?
Maybe I'll mess with it and raise something with the engine team because I'm not sure it should require us to finagle like this.
There was a problem hiding this comment.
Looks great.
One small tweak of https://github.com/KittyCAD/modeling-app/pull/2539/files#r1616427476
And now that it's interactive it would be good to get some e2e tests in I'll follow up with a few more details.
-- edit: never mind, realised this is essentially what I mentioned in #2539 (comment), it just didn't click sorry.
|
Okay so here's my idea for tests, the stub in fc37031, there should be some notes in the readme about playwright (but feel free to updates them if anything is missing or stale) And the following explanation Screen.Recording.2024-05-28.at.11.51.19.AM.mp4 |
|
Also from your description, I don't get
Could you expand on what you mean here? |
|
@Irev-Dev, thank you so much! The video was super helpful! ^^ Awesome update: I couldn't test it because I don't see any model to zoom in on. Do you have any updates on fixing my issue (probably related to tokens)? I could better explain and test orientation ideas if I had something on screen. Anyway, if it works for you, then I can remove the "reset view" handle. This matches what I was proposing:
Thanks for implementing it!
Awesome, thanks! Nest steps: |
|
Oh right, yeah it's going to be hard for you to finish the test if you can't get the stream/token working. |
|
@franknoirot, thank you for the tips, they helped a lot! @Irev-Dev here are my findings regarding the recent issues we've faced with our Playwright tests, both locally and in our CI environment. Local Testing: CI Testing: This adjustment allowed the tests to pass on Mac, but they continued to be flaky on Ubuntu. Upon reviewing the trace screenshots from the Ubuntu tests, I noticed that after clicking on the gizmo, the camera appears to rotate, but the values in the debug output remain unchanged, and the gizmo itself does not update. This suggests that the gizmo correctly sends new coordinates to the server, and the server updates the server-side camera, but fails to notify the client-side camera.
it explains this error message: This should not happen because in the For some reason, this call does not always execute properly on Ubuntu. Temporary Solution: This modification helped the tests pass, but we need to investigate why this issue occurs and double-check the behavior of the |
| await u.sendCustomCmd({ | ||
| type: 'modeling_cmd_req', | ||
| cmd_id: uuidv4(), | ||
| cmd: { | ||
| type: 'default_camera_get_settings', | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Making a not of this for as issue #2603
There was a problem hiding this comment.
I got rid of some of the longer timeouts in a3313e2
🤞 the tests are okay, if not we can change it back. As a rule of thumb I'm okay with timeouts of 100, as a way to wait for things to tick along, but if they're more than 100 it's normally a sign that we're not waiting on the right thing, (having said that there are still bigger timeout in the tests, we'll use them where needed). So what I did instead of the timeout is waiting for responses to the websocket commands.
The problem with timeouts when used to wait for something async to happen is the timeout is either too short and you have flakey tests (a real PITA) or you make them conservatively long and most of the time you wouldn't have needed to wait that long, but for the odd test that's running slow, it's slow enough for that case, but now you've slowed down that test every-time it runs to the slowest possible case. And the time it takes for tests to run can really ad up.
Your solution with the camera not updating is a good one, that's a bit of a mystery as to what's happening there. I'd rather not forget about it so I made #2603
|
Sorry I'm getting unrelated failures now, I'll get this fixed up and merged soon. |
Summary:
This pull request addresses two main features:
Gizmo Normal Snapping (gizmo should allow snapping normal to #2445):
Point-and-Click Improvements (Point and Click improvements #2035):
Key Updates:
Gizmo Interaction Enhancements:
updateCameraToAxismethod incameraControlsto adjust camera vantage and up vector based on the selected axis.View Reset:
default_camera_look_atanddefault_camera_get_settingsendpoints to manage camera settings.Next Steps:
Implementation Details:
src/clientSideScene/CameraControls.ts.src/components/Gizmo.tsx.References:
Please review the changes and provide any additional comments or suggestions.