Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix node bound displacement #24

Merged
merged 4 commits into from
Jan 19, 2021
Merged

Fix node bound displacement #24

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d92a981
Fix node bound displacement
Jan 8, 2021
0595edb
Finalized fix by replacing rect observer with polling and fixing inco…
Jan 12, 2021
cadb63e
Carry over fixes
Jan 12, 2021
ecb1296
Fix disconnect removeEventListener
Jan 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 7 additions & 16 deletions src/UX/mouse/MouseHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,29 +178,19 @@ export class MouseHandler extends EventEmitter.mixin(UXModule) {

private update(state: MouseState): void {
const events: EventEntry[] = [];
if (state.valid !== this.state.valid) {
this.state.valid = state.valid;
vec2.copy(this.state.clientCoords, state.clientCoords);
vec2.copy(this.state.canvasCoords, state.canvasCoords);
}
vec2.copy(this.state.deltaCoords, state.deltaCoords);
vec2.copy(this.state.clientCoords, state.clientCoords);
vec2.copy(this.state.canvasCoords, state.canvasCoords);
vec2.copy(this.state.glCoords, state.glCoords);

if (this.state.deltaCoords[0] !== 0 || this.state.deltaCoords[1] !== 0) {
if (this.state.valid) {
if (state.deltaCoords[0] !== 0 || state.deltaCoords[1] !== 0) {
if (state.valid) {
events.push({
event: kEvents.move,
args: [this.state.deltaCoords, this.state.canvasCoords],
args: [state.deltaCoords, state.canvasCoords],
});
}
}

const buttonKeys = Object.keys(state.buttons);
for (let i = 0, n = buttonKeys.length; i < n; ++i) {
const key = buttonKeys[i];
const pressed = this.state.valid && state.buttons[key];
const pressed = state.valid && state.buttons[key];
if (this.state.buttons[key] !== pressed) {
this.state.buttons[key] = pressed;
events.push({
Expand All @@ -209,7 +199,7 @@ export class MouseHandler extends EventEmitter.mixin(UXModule) {
});
}
}

this.setMouseState(state);
this.emitEvents(events);
}

Expand All @@ -223,6 +213,7 @@ export class MouseHandler extends EventEmitter.mixin(UXModule) {
this.state.valid = state.valid;
vec2.copy(this.state.clientCoords, state.clientCoords);
vec2.copy(this.state.canvasCoords, state.canvasCoords);
vec2.copy(this.state.glCoords, state.glCoords);
vec2.copy(this.state.deltaCoords, state.deltaCoords);
this.state.wheel = state.wheel;
Object.assign(this.state.buttons, state.buttons);
Expand Down Expand Up @@ -271,7 +262,7 @@ export class MouseHandler extends EventEmitter.mixin(UXModule) {

this.newState.valid = Boolean(
canvas[0] >= rect.left && canvas[0] <= rect.right &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this also be normalized to local coordinates?

canvas[0] >= 0 && canvas[0] <= rect.width

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you tested with horizontal scrolling?

canvas[1] >= rect.top && canvas[1] <= rect.bottom
canvas[1] >= 0 && canvas[1] <= rect.height
);

this.newState.buttons.primary = Boolean(e.buttons & 1);
Expand Down
54 changes: 54 additions & 0 deletions src/renderer/RectObserver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
export type RectObserverCallback = (rect: DOMRectReadOnly) => void;

const POLLING_RATE = 400; // ms

export default class RectObserver {
public elementTarget: HTMLElement;
public callback: RectObserverCallback;
public rect: DOMRectReadOnly;
private poll: ReturnType<typeof setInterval>;

constructor(callback: RectObserverCallback) {
this.callback = callback;
}

public observe(element: HTMLElement): void {
this.elementTarget = element;

this.elementTarget.addEventListener("mouseenter", this.handleMouseEnter.bind(this), false);
this.elementTarget.addEventListener("mouseleave", this.handleMouseLeave.bind(this), false);

this.rect = this.elementTarget.getBoundingClientRect();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we updating the rect on mouse enter/leave? what's the use case?

Copy link
Contributor Author

@mj3cheun mj3cheun Jan 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, that makes sense, made a note on #30 to track this change

}

public disconnect(): void {
clearInterval(this.poll);
this.elementTarget.removeEventListener("mouseenter", this.handleMouseEnter.bind(this), false);
this.elementTarget.removeEventListener("mouseleave", this.handleMouseLeave.bind(this), false);
}

private handleMouseEnter(): void {
this.pollElement();
this.poll = setInterval(this.pollElement.bind(this), POLLING_RATE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you looked into this, but are 100% sure there's no other way? polling seems horrible!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok this is good for now, we should revisit at some point with #30

}

private pollElement(): void {
const rect = this.elementTarget.getBoundingClientRect();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense to me, let's revisit at some point though

if(!this.rectEqual(this.rect, rect)) {
this.rect = rect;
this.callback(this.rect);
}
}

private handleMouseLeave(): void {
this.pollElement();
clearInterval(this.poll);
}

public rectEqual(prev: DOMRectReadOnly, curr: DOMRectReadOnly): boolean {
return prev.width === curr.width &&
prev.height === curr.height &&
prev.top === curr.top &&
prev.left === curr.left;
}
}
6 changes: 3 additions & 3 deletions src/renderer/Viewport.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {Camera} from './Camera';
import {Graph} from '../graph/Graph';
import {MouseHandler} from '../UX/mouse/MouseHandler';
import {ColorRegistry} from './ColorRegistry';
import RectObserver from './RectObserver';

export class Viewport {
public readonly element: HTMLElement;
Expand Down Expand Up @@ -69,9 +70,8 @@ export class Viewport {

this.camera = new Camera(this.size);

const resizeObserver = new ResizeObserver((entries: ResizeObserverEntry[]): void => {
const canvasEntry = entries[0];
this.rect = canvasEntry.contentRect;
const resizeObserver = new RectObserver((rect): void => {
this.rect = rect;
this.context.resize(this.rect.width * this.pixelRatio, this.rect.height * this.pixelRatio);
vec2.set(this.size, this.canvas.width, this.canvas.height);
this.camera.viewportSize = this.size;
Expand Down