asny / three-d

2D/3D renderer - makes it simple to draw stuff across platforms (including web)
MIT License
1.24k stars 105 forks source link

OrbitControl zoom-in always zooms to max for wasm target #403

Open trumank opened 8 months ago

trumank commented 8 months ago

Zoom-in always zooms to max zoom, but zoom out steps in normal increments. This seems to only affect wasm builds and is visible in the live examples.

trumank commented 8 months ago

Seems like zoom-in just doesn't like high delta values, but zoom-out is fine. The events received by the wasm version have vertical deltas of -120 and 120 and when I send events with those same deltas to the desktop version I get the same behavior.

asny commented 8 months ago

It works just fine for me, both on mobile touch and desktop mouse and desktop touchpad 🤷 a delta of 120 also sounds crazy high so maybe something with your mouse or whatever you use? Otherwise, it might be a bug in the winit crate? At least, I'd like you to pinpoint exactly where you found a delta of 120 in winit_window.rs?

huand commented 8 months ago

had the same issue and was about to post this. also works fine on non-wasm with the same mouse, but on web only zoom-in will go to max. I remember also a 120 delta for me. (but still weird that it doesnt go to the previous place. 1 zoom out + 1 zoom in should be the identity but somehow zoom in is bugged?). So just to say i have same issue. Works fine when i use the touchpad, only with plugged mouse this appears, and only on wasm. Also maybe 120 is crazy high but zoom out still looks ok per increment.

huand commented 8 months ago

i just checked and i have +-120 also for delta both ways. Also i tried with clamping the delta value and it was working just fine then. It is just a mouse scroll increment problem. Maybe clamping in the library code would help as such high values make little sense and it's probably unintended from the user?

but about the the fact that zoomin->zoomout is not the identity for orbitcontrol, this could be solved by something similar to this:

new camera distance to target = current_distance 2^(a delta)

(minus delta works as the opposite division) so a zoom out/zoom in with same +-delta:

d02^(a delta) 2^(-a delta)=d0 <-initial distance

if making the zoom have this "identity behavior" beneficial, i can make a PR. (but this really is just a noticeable problem in this already problematic case of 120 ydelta scroll) also don't know why the scroll delta is ok in non-wasm

huand commented 8 months ago

for ref my delta scroll in non-wasm is +-24

huand commented 8 months ago

Also for @trumank if you want to "fix it" in your user code I quickly tried this and works as expected

        frame_input.events.iter_mut().for_each(|e| {
            if let three_d::renderer::control::Event::MouseWheel { delta: (_, b), .. } = e {
                let clamp = 24.;
                *b = b.clamp(-clamp, clamp);
            }
        });
trumank commented 8 months ago

@huand Ha! I have a similar hack:

        for event in &mut frame_input.events {
            if let Event::MouseWheel {
                ref mut delta,
                handled,
                ..
            } = event
            {
                if !*handled {
                    // artificially decrease zoom delta
                    // https://github.com/asny/three-d/issues/403
                    delta.1 /= 5.;
                }
            }
        }
huand commented 8 months ago

cool! be careful that it still works as intended most of the cases (probably some special config somewhere about mouse scroll stuff?) for example for me it works well with the touchpad of the same pc. with my .clamp() method it just affects the speed of the "faulty scroll" and yours makes everyone else slow to zoom(even if you dont realize it because of always using the same gear :smile:

asny commented 7 months ago

Sorry for the late reply 😬

if making the zoom have this "identity behavior" beneficial, i can make a PR.

@huand That would be super great 👍 The controls are really really simple right now, they could definitely be improved and this would make zoom much, better I think.

If you look at the OrbitControl code, it's really simple. It's actually constructed so you can easily make your own control by wrapping a CameraControl in a struct in a similar manner as the OrbitControl. That means, you can make your own control that behaves exactly as you want without having to write a lot of code.

I would still like a fix for this special case in the OrbitControl. It's probably just a matter of updating the speed in a different way than now:

if let CameraAction::Zoom { speed, target, .. } = &mut self.control.scroll_vertical {
    let x = target.distance(*camera.position());
    *speed = 0.01 * x + 0.001;
}

Or maybe we need some changes to the Zoom case in CameraControl?