FIX: Dynamic zoom speed with distance from camera broken

[Amoki]

Describe the bug
Zoom speed is constant and doesn't slow down when the focused object is near.
Can't it come from #1261?

To Reproduce
Steps to reproduce the behavior:

1. Go in a building
2. Zoom
3. Camera goes too fast

[xeolabs]

Probably. I didn't notice that when testing. Unfortunately I'm out of time to fix this until Sunday.
/ping @tmarti could you take a look?

[Amoki]

The object picking is also bugged viewer.cameraControl.on("picked") is not emited. I tried and both problems come from this PR

[tmarti]

That's unfortunate! Will fix tomorrow morning 🙂👌

[Amoki]

No worries!
I totally can wait

[tmarti]

@Amoki: also, do you have some sample HTML page for the case of viewer.cameraControl.on("picked")

[xeolabs]

@tmarti I can easily do a rollback on this no worries, no pressure man.

I just had a little look; basically the issue is that there are some handlers within CameraControl that expect a pick to happen synchronously when PickController.update() is called, where the handlers then check for results on PickController immediately after calling. So, when that is asynchronous, the results won't be there immediately after, and we'll get some missing pick events.

An idea: perhaps PickController.update() could cache its pick results until the next tick? So the first time it's called after the last tick, do an immediate pick, cache the results and return. Then if update() is called again, and a new tick hasn't happened yet, return the same results. Then as soon as a new tick happens, clear the results etc.

This would assume that the view does not update in any way between multiple calls to update() within the current render frame, which I believe is a safe assumption, since a render only happens once per tick.

The cache would be valid as long as the combination of values of these don't change since the last call to `update():

• PickController.pickCursorPos
• PickController.schedulePickEntity
• PickController.schedulePickSurface
• PickController.scheduleSnapOrPick

We could hash those. On each update() if the same values are on these since the last time, cached results would still be valid.

CameraControl.fireEvents() can remain the way it is, no problem there AFAIK.

Also a quick hack for now, if you run out of time, could be to make PickController.update() method call PickController.runUpdate().

[xeolabs]

@tmarti check it out, maybe this does the trick: #1264

I'm only seeing a small number of cache hits though. Maybe the hash can be improved.

[tmarti]

Yes, I see what's the problem now with my code 🙂.

It's this pattern:

pickController.update();

if (pickController.pickResult) {
    ...
}

This is, do something with pickResult immediately after calling the PickController.update() method!

Ok, then what do you think about this option @xeolabs?

For performance reasons, what I wanted to avoid is an avalanche of "picks" triggered by mousemove events, so at most we had one pick per tick due to those events, (plus fixing that race condition explained above).

What we could do, for input events that are triggered more than once per tick, is to "tick-debounce" them.

Following this idea, instead of the following pattern...

canvas.addEventListener(
    "mousemove",
    (e) => someCodeUsingPickController(e)
);

We could do wrap the someCodeUsingPickController() with that "tick-debouncer" function!

canvas.addEventListener(
    "mousemove",
    (e) => maxOncePerTick(
        () => someCodeUsingPickController(e)
    )
);

And, do the debouncing in maxOncePerTick instead of the PickController's double-scheme update+runUpdate.

This could be probably used also for mousewheel events?

Will do a quick experiment and tell 🙂

[xeolabs]

@tmarti yes that sounds good, although I dare say it might make the handlers a bit more complex in terms of state management. Let's see how your experimentation goes though, could be workable.

[tmarti]

Have created a new PR that should solve both the performance problem and the bug introduced in #1261 🙂.

I'm working out the PR description, will let you know when it's ready 😃

(spoiler: this is the new PR => #1265)

[tmarti]

@xeolabs @Amoki the description in #1265 is now complete 😃

[tmarti]

yes that sounds good, although I dare say it might make the handlers a bit more complex in terms of state management. Let's see how your experimentation goes though, could be workable

@xeolabs the new PR does not need any change in the state management part, just tickifying (and later untickyfy) the handlers 😊

[xeolabs]

Fixed by #1265