Fix node bound displacement #24
Conversation
|
This one looks good, I'll run some local tests to make sure it fixes the issues we were seeing, @adamocarolli could you also test this branch on your system? Seems to me that this bug could be OS/Browser specific. |
|
Unfortunately, this doesn't fix the issue pointed out by the bug. As you can see in the video below, using the example file uploaded by @adamocarolli, the mouse coordinates shift after scrolling the page. #9 tries to solve the issue by always using a fresh rect rather than caching it, that solution works fine for clicking but it breaks hovering which leads me to believe that there's a logic mistake somewhere. simplescreenrecorder-2021-01-11_11.10.27.mp4 |
|
Ok thanks for catching that. I think theres a bug in the logic of my code, lemme test something. |
|
Recent changes resolve picking issue for me when testing locally on MacOS/Chrome! |
| @@ -271,7 +262,7 @@ export class MouseHandler extends EventEmitter.mixin(UXModule) { | |||
|
|
|||
| this.newState.valid = Boolean( | |||
| canvas[0] >= rect.left && canvas[0] <= rect.right && | |||
There was a problem hiding this comment.
should this also be normalized to local coordinates?
canvas[0] >= 0 && canvas[0] <= rect.width
There was a problem hiding this comment.
From the tests I have run, it seems to work without also normalizing the x coordinates. (I only noticed the bug in the y direction) I get your point though, it seems weird that not normalizing the y coordinates would create a bug but not normalizing the x coordinates would be fine. I think its worth keeping an eye out for this.
There was a problem hiding this comment.
I assume you tested with horizontal scrolling?
|
|
||
| private handleMouseEnter(): void { | ||
| this.pollElement(); | ||
| this.poll = setInterval(this.pollElement.bind(this), POLLING_RATE); |
There was a problem hiding this comment.
I know you looked into this, but are 100% sure there's no other way? polling seems horrible!
There was a problem hiding this comment.
As far as my research has found there is no other way to reliably track changes to rect. I am not happy about polling either ya :(
There was a problem hiding this comment.
ok this is good for now, we should revisit at some point with #30
| this.elementTarget.addEventListener("mouseenter", this.handleMouseEnter.bind(this), false); | ||
| this.elementTarget.addEventListener("mouseleave", this.handleMouseLeave.bind(this), false); | ||
|
|
||
| this.rect = this.elementTarget.getBoundingClientRect(); |
There was a problem hiding this comment.
why are we updating the rect on mouse enter/leave? what's the use case?
There was a problem hiding this comment.
On second thought, there is no point in checking rect on mouseleave, so that can be removed with no effects.
As for mouse enter, I think its a good idea to check if rect needed to be updated when the cursor first enters the graph element to ensure that there are no visible errors as the user interacts with the graph. This is made necessary by the slow polling rate.
There was a problem hiding this comment.
cool, that makes sense, made a note on #30 to track this change
| } | ||
|
|
||
| private pollElement(): void { | ||
| const rect = this.elementTarget.getBoundingClientRect(); |
There was a problem hiding this comment.
We are updating the rect so often here that I wonder if it would be easier (and more performant) to calculate the rect in the mouse handler when the mouse moves, maybe have some other caching mechanism?
I feel like this approach is too "hot"
There was a problem hiding this comment.
So theres a few things I am doing in order to make this solution as low impact as possible. First, note that the polling period is 400 ms. This is as slow as I can possibly make it I feel before its possible for the user to notice any trigger inaccuracy. Second, the class maintains a record of the previous rect value, and it compares the new rect values with the old. It will only call the callback function if there is any difference. Therefore the performance impact when the location and size of the element is constant should be very small, as the most expensive function in this case is getBoundingClientRect. In the case where the element is actually moving, I think that its possible for this polling strategy to be even more performant compared to a naive observer strategy where the callback could be called every frame.
There was a problem hiding this comment.
makes sense to me, let's revisit at some point though
|
Opened #30 to keep track of unresolved comments |
Fixes #10
Replaces #9