Fix picking perf when dragging #775
Conversation
src/utils/events/event-manager.js
Outdated
| * Process the event deregistration for a single event + handler. | ||
| */ | ||
| _removeEventHandler(event, handler) { | ||
| let success = false; |
There was a problem hiding this comment.
nit: call this eventHandlerRemoved to be a bit more explicit about what succeeded
| }; | ||
|
|
||
| // Calculate center relative to the root element | ||
| // TODO/xiaoji - avoid using getBoundingClientRect for perf? |
There was a problem hiding this comment.
Did you decide this is not a perf concern?
There was a problem hiding this comment.
I don't find it prominent during profiling.
There was a problem hiding this comment.
May not be conclusive. It reportedly depends on the surrounding DOM, how deep we are in the DOM tree, styling etc.
Whether it is worth the trouble to avoid I don't know...
| if (alias) { | ||
| // fire all events aliased to srcEvent.type | ||
| const emitEvent = Object.assign({}, event, {type: alias}); | ||
| const emitEvent = Object.assign({}, event, {isDown: true, type: alias}); |
There was a problem hiding this comment.
Looks like you're always setting isDown: true here, should the third Object.assign() param be the second?
There was a problem hiding this comment.
we're overriding event.type.
|
Worth noting that this PR fixes the more obvious perf problem during dragging (perf slows down the screen update while dragging), but not the more subtle perf problem that happens when running picking on hover (mousemove without mousedown). The latter is for a future PR. |
| }; | ||
|
|
||
| // Calculate center relative to the root element | ||
| // TODO/xiaoji - avoid using getBoundingClientRect for perf? |
There was a problem hiding this comment.
May not be conclusive. It reportedly depends on the surrounding DOM, how deep we are in the DOM tree, styling etc.
Whether it is worth the trouble to avoid I don't know...
src/utils/events/event-manager.js
Outdated
| import WheelInput from './wheel-input'; | ||
| import MoveInput from './move-input'; | ||
| import {isBrowser} from '../globals'; | ||
| import {isBrowser} from '../../lib/utils/globals'; |
There was a problem hiding this comment.
Some of these utils should be moved up a level if they are no longer used only inside lib.
src/lib/layer-manager.js
Outdated
| */ | ||
| _onPointerMove(event) { | ||
| if (event.isDown) { | ||
| // Is dragging |
There was a problem hiding this comment.
Can we add a little more to the comment i.e. why does dragging mean that we don't need move events?
There was a problem hiding this comment.
this comment means we should not fire pointermove event when mouse is down because pointermove leads to pickLayer getting called. Yes, we probably want to explain a bit more in the comments, or considering put a link to this PR
There was a problem hiding this comment.
Well, more specifically _onPointerMove feeds into our hover handlers (onHover and onLayerHover), and dragging is not supported by the framework itself, it's the app's responsibility. But yeah, fleshing out the comment will help future detective work.
| @@ -1,15 +0,0 @@ | |||
| // Purpose: include this in your module to avoids adding dependencies on | |||
- Update EventManager - Add `isDown` property to pointer events - Prevent picking when dragging
4 participants
isDownproperty to pointer events