KittyCAD / modeling-app

The KittyCAD modeling app.
https://kittycad.io/modeling-app/download
MIT License
426 stars 37 forks source link

gizmo should allow snapping normal to #2445

Closed jessfraz closed 5 months ago

jessfraz commented 6 months ago

click to reset the view as well (zoom to fit)

from discord: https://discord.com/channels/915388055236509727/1138967638786179123/1242630645054967838

jessfraz commented 6 months ago

Same as https://github.com/KittyCAD/modeling-app/issues/1525

franknoirot commented 6 months ago

Zoom-to-fit we should put in a context menu that's available anywhere in the main view, Onshape has this convention and I appreciate it.

After we get the engine up-vector issue settled we should be able to extend @max-mrgrsk's client-side gizmo from #2354 to add hover and click events on each of the axes that call the appropriate engine commands.

I am a little nervous about my gizmo design's ability to support one-click isometric views. I believe this is why a lot of CAD applications have view cubes (Solidworks, Onshape). I can think of a stop-gap solution for isometric views and also design what a full view cube could look like for us.

I don't think these three things will be done in this week's push though unfortunately.

max-mrgrsk commented 6 months ago

I am a little nervous about my gizmo design's ability to support one-click isometric views. I believe this is why a lot of CAD applications have view cubes. I can think of a stop-gap solution for isometric views and also design what a full view cube could look like for us.

@franknoirot While view cubes offer more clicking options, axis gizmos provide a clearer sense of orientation. I like your current solution a lot.

anyway, swapping the gizmo with a cube is an easy fix. I'm happy to do it anytime :D

max-mrgrsk commented 6 months ago

Zoom-to-fit we should put in a context menu that's available anywhere in the main view, Onshape has this convention and I appreciate it.

Nice. Buttons F / N to fit the entire model / reset the view - are cool too. In blender you can do it with numpad buttons, and use num numbers for Top, Front, Right, etc.

Irev-Dev commented 6 months ago

I've assigned you @max-mrgrsk as well.

I want to give you more interesting issues too, but I think it makes sense as follow up with your gizmo.

I think the approach for this will be to use default_camera_look_at and default_camera_get_settings endpoints

await engineCommandManager.sendSceneCommand({
  type: 'modeling_cmd_req',
  cmd_id: uuidv4(),
  cmd: {
    type: 'default_camera_look_at',
    center: { x: 0, y: 0, z: 0 },
    vantage: { x: 5, y: 5, z: 5 },
    up: { x: 0, y: 0, z: 1 },
  },
})
await engineCommandManager.sendSceneCommand({
  type: 'modeling_cmd_req',
  cmd_id: uuidv4(),
  cmd: {
    type: 'default_camera_get_settings',
  },
})

The reason why both are needed is because the the first one moves the engine camera, and the second command we already have listeners for to update the the client side camera (what we've already been doing for the gizmo)

for

 cmd: {
    type: 'default_camera_look_at',
    center: { x: 0, y: 0, z: 0 },
    vantage: { x: 5, y: 5, z: 5 },
    up: { x: 0, y: 0, z: 1 },
  },

center is the camera's target and vantage is the camera's position, I think what we can do is keep the current camera's target for center (it shouldn't change) and just update vantage. I think it would make sense to get the current distance to the target with something like const distance = this.camera.position.distanceTo(this.target) that way clicking on the z axis in the gizmo could vantage: { x: 0, y: 0, z: distance }

I would probably implement most of this logic in a method in src/clientSideScene/CameraControls.ts and call the method with sceneInfra.camControls.yourMethod(...) within src/components/Gizmo.tsx but I'm probably being a little too prescriptive, don't feel the need to follow this exactly.

Irev-Dev commented 6 months ago

I don't have strong opinions on hotkeys or cube vs the current gizmo.

Irev-Dev commented 6 months ago

Oh I should mention that this will abruptly snap the camera, I think that's fine for now, we'd need the engine to give us a animated: bool param on default_camera_look_at to fix this.

max-mrgrsk commented 6 months ago

thanks @Irev-Dev , I would proceed with the following steps:

  1. Gizmo Interaction:
    • Click and Hover Events: Ensure the gizmo has click and hover events for all axes. ( visual feedback when hovering over or clicking on the gizmo ) and make camera part.

For animation, we could consider using an animated: value parameter instead of a boolean, allowing us to control the length of the animation in ms.

  1. Zoom-to-Fit and View Reset:
    • View Reset: I'd start with it, @Irev-Dev logic, it looks super nice.
    • Zoom-to-Fit: We need to know the size of the model to understand the camera's distance to it, the center of the model, especially if it is not in the middle of the scene.
    • Context Menu and Hotkeys: Implement the “Zoom-to-fit” and “Reset view” options in both the context menu and through hotkeys. I'd need some help from @franknoirot here :D

Extending this further, it would be beneficial to know what part / face / edge is selected to zoom into it, or if nothing is selected, zoom into the entire assembly.

  1. Future View Cube Implementation:
    • We can revisit the view cube implementation once we have completed these interaction improvements. Can we conduct a poll in the community to gather opinions and preferences? Or just do both :D

btw, in Onshape, the whole gizmo can be moved with drag.

franknoirot commented 6 months ago

Zoom-to-Fit: We need to know the size of the model to understand the camera's distance to it, the center of the model, especially if it is not in the middle of the scene.

We actually just got a zoom_to_fit engine command landed so you don't have to do any of that math client-side!

Extending this further, it would be beneficial to know what part / face / edge is selected to zoom into it, or if nothing is selected, zoom into the entire assembly.

Ayy at least zoom_to_fit takes an array of object to fit, so that part I think we'd just have to plug the user's selection into that command.

Context Menu and Hotkeys: Implement the “Zoom-to-fit” and “Reset view” options in both the context menu and through hotkeys. I'd need some help from @franknoirot here :D

Yeah man I'll get this feature in a separate issue assigned to me, don't worry about context menus it's a whole other can of worms

btw, in Onshape, the whole gizmo can be moved with drag.

Yeah truth be told I have no idea why they implemented that, and I'm not keen to replicate it. In future I could maybe get behind letting the user snap it to different corners, but I just don't see the utility of the draggable view cube.

max-mrgrsk commented 6 months ago

I have finished the gizmo update; it is now interactive: da56ced71a845d3b3b2bf36228be4a90d5839785

Key updates:

Gizmo

  1. The Raycaster is enabled only when the mouse is over the gizmo canvas for better performance.
  2. If the mouse is over any gizmo handle, the Raycaster scales up this handle and sends its information, including the AxisName, to handleClick.
  3. If a click happens while the mouse is over a gizmo handle, it calls the updateCameraToAxis method in cameraControls, as suggested by @Irev-Dev, and passes the AxisName to it.
  4. The method in cameraControls sets not only vantage but also up, because when looking up or down along the Z axis, the camera's up vector is different: -1, 0, 0.

resetCameraPosition

I've temporarily added a handle in the middle of the gizmo to test the resetCameraPosition method in cameraControls. If I knew the assembly size, I could pass it to vantage. Otherwise, the scene might appear much smaller or larger than the hardcoded value.

Next steps I will have another look at the gizmo this week. If everyone is happy with how it works, I will remove the reset handle and call it a day. Please review the updates and let me know your thoughts.

@franknoirot, the gizmo handle scales up 1.5 times on mouse over. If you have any other ideas, I'm happy to implement them :)

The server-side camera sends a 0,0,0,0 rotation for the first few seconds until the stream loads, causing the Gizmo to start aligned to the X axis and then snap to the default rotation. This effect is not nice. I could temporarily swap the initial rotation values with fake ones that match the default camera rotation until we get the correct values from the server. This would be a temporary solution until the engine team fixes it. What do you think?

_@franknoirot, is there any help needed from me regarding zoom_to_fit?_

franknoirot commented 6 months ago

@franknoirot, the gizmo handle scales up 1.5 times on mouse over. If you have any other ideas, I'm happy to implement them :)

That works for me for now @max-mrgrsk, thanks for adding a change sorry I didn't give any direction there!

In future we may opt for a brightness change instead of a scale change, but I'm not 100% certain so let's move forward with this, it looks great.

My concern I'm gonna watch for as the UI matures is ensuring that it feels increasingly "solid" to users, and while things that move on interaction are predictable and therefore shouldn't contribute to a "shaky" or "unreliable" feeling, it's one thing I'm keeping my eye on.