Fix ScreenSpaceEventHandler when Cesium does not take up the entire browser window

[kring]

A bug was introduced in #2408 that breaks mouse events completely when the Cesium canvas is not in the upper-left of the browser window.

The problem is that clientX and clientY are relative to the client area of the overall browser window, not the DOM element, even for listeners attached to a specific element.
https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent.clientX

[pjcozzi]

@bagnell can you review please?

[kring]

I found a related bug in CameraEventAggregator. Fix coming soon.

[pjcozzi]

Additional discussion: https://groups.google.com/forum/#!topic/cesium-dev/Emt-CrGYulY

[kring]

The related bug I mentioned above is separate from the issue mentioned on the forum, and is now fixed in this pull request. CameraEventAggregator was receiving events from two different ScreenSpaceEventHandlers, and not accounting for the fact that they provide coordinates in two different coordinate systems (document versus canvas).

[mramato]

I know very little about the mouse handling code, but isn't canvas.getBoundingClientRect(); a performance bottleneck? I'm pretty sure some browsers reflow/recalculate it every time it's called. I think we cache results like this in other places and only compute it once at the beginning of the frame. In this case it might not be an issue, but I just wanted to bring it up.

@shunter any thoughts here?

[pjcozzi]

Any update? We'll want to merge this this week so it makes 1.6.

[bagnell]

This looks good to me. Just waiting on a response to @mramato's comment.

[emackey]

I found various references here and there that suggest a full layout is triggered by getBoundingClientRect. We're already getting the size in viewer.resize() every frame, is it reasonable to pass that size down into the handlers somehow?

[kring]

We're already getting the size in viewer.resize() every frame

Size, yes, but we're not calling getBoundingClientRect every frame. That's good because we only need it on input, which happens less often than rendering.

is it reasonable to pass that size down into the handlers somehow?

Not really. The top of the call stack is an input event, so we can't pass it down from there. The best we could do is compute and cache the rectangle if it's undefined, and then reset it to undefined each frame. I started trying to do this but it's ugly and likely to lead to bugs. In particular, ScreenSpaceEventHandler has no notion of render frames and no dependence on Scene. The scene could potentially reset its ScreenSpaceEventHandler(s) each frame, but what if the user creates their own? I think that's somewhat common because a single ScreenSpaceEventHandler can only have a single handler per input event. The user would need to remember to reset it each frame (or at least anytime the size/location of the canvas changes). If they don't, they get weird bugs.

I just committed a change to reduce the number of calls to getBoundingClientRect a bit, so maybe that makes it more acceptable to everyone.

By the way, this pull request brings Cesium's input handling from "horribly broken" to "working but possibly not quite as optimal as it could be" so merge it already! 6 days, sheesh. :)

[emackey]

Agreed we should merge this before the release.

input, which happens less often than rendering.

With one important caveat: When you're moving the mouse, dozens of mousemove messages come in per frame. That's the reason for the CameraEventAggregator, is to gather up those messages and limit activity to once per frame. In theory we could make less calls by moving the logic there, but in practice I don't know if additional re-layouts are actually caused by multiple calls in the same frame.

Anyway, I did a little performance testing of this vs master, and I couldn't spot the difference. So @bagnell I think you should merge.

[mramato]

As @emackey stated, profiling doesn't show any performance issues (I tried IE, Firefox, and Chrome).