KittyCAD / modeling-app

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

Rework zooming #2798

Open lf94 opened 5 days ago

qa-wolf[bot] commented 5 days ago

QA Wolf here! As you write new code it's important that your test coverage is keeping up. Click here to request test coverage for this PR!

vercel[bot] commented 5 days ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
modeling-app ✅ Ready (Inspect) Visit Preview Jun 28, 2024 11:26pm
adamchalmers commented 5 days ago

A before/after video would really hit the spot just saying :)

Also could you explain what the overall problem/solution was? I'm not very familiar with this part of the code.

lf94 commented 5 days ago

I was telling @jessfraz this is hard to get a video because you wont be able to see when I scroll and when it fires

I'll copy paste my explanation I sent to jess in a PM:

So a big issue is the fact this is all over network
I think we already knew this, but we were not using the data channel at all
For zoom
Another thing is we were "debouncing" but what we really needed was some sort of buffer per slice of time that only sends a portion of commands
So that the behavior is the same from either a mouse or a trackpad
I need to add similar logic to "sketch mode zooming" because that is completely client side, and the user can tell the difference immediately
Another issue was we were not properly dividing by devicePixelRatio
(Since the deltas are in pixels)
Irev-Dev commented 4 days ago

You need to use the camera on your phone lol, record your hands as well.

Irev-Dev commented 4 days ago

I'm worried we're over engineering this so wanted to pop in this bit of context and thought.

Maybe we over-enigeering on the frontend when there's a simple API change

Changing magnitude in from something relative to something absolute. (not 100% sure of the specifics)

{
  magnitude: number;
  type: 'default_camera_zoom';
}

Here's the full thought.

https://github.com/KittyCAD/modeling-app/assets/29681384/9700b0ce-8298-4121-b481-eb2662a9bbdd

lf94 commented 4 days ago

For what it's worth, I believe the solution is pretty simple as-is and could actually be more complicated if we introduce heuristics and predictions.

I don't understand how relative vs absolute changes in the zoom would help. We should still see symptoms of network latency / congestion since that's the core of the issue. All my "solution" does is reduce the out going commands to something reasonable but also still visually acceptable / not really noticable.

jessfraz commented 4 days ago

Merge it thank you Lee!

jessfraz commented 4 days ago

Bye bye TCP I cannot even lol mine was always so backed up on tcp

lf94 commented 4 days ago

Oh yeah I've got @adamchalmers working on another aspect of this issue which should also drastically help.

lf94 commented 2 days ago

I've actually made this even better.

lf94 commented 2 days ago

I did away with the buffering and instead, we have a "virtual fps", which is set to 60. We can lower this number as needed. It's actually simpler than the original solution.

lf94 commented 2 days ago

@jessfraz could you try this on mac again