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

Fix zoom centering #605

Draft
wants to merge 8 commits into
base: dev
Choose a base branch
from
Draft

Fix zoom centering #605

Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1699732
Document setZoomLevel, change the distance from center check to use t…
guyguy2001 Feb 9, 2022
11a2fe1
Merge branch 'dev' into fix-zoom-centering
guyguy2001 Feb 9, 2022
2b8f24c
Refactor zooming to respect the mouse position, add world<->screen ut…
guyguy2001 Feb 10, 2022
1465e91
Refactor Pose2D to use Point for offsets
guyguy2001 Feb 10, 2022
16c235a
Rename offsetTranslating to animatingZoom, make stuff call setOffset
guyguy2001 Feb 14, 2022
2239eb2
Remove extranious getter
luxaritas Mar 9, 2022
549561a
Split setZoomLevel into helper zoomOntoMouse and focusOnScreenPoint
guyguy2001 May 6, 2022
6febe8e
Fix centering when changing to PIP Mode
guyguy2001 May 6, 2022
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
186 changes: 121 additions & 65 deletions src/eterna/pose2D/Pose2D.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,38 @@ export default class Pose2D extends ContainerObject implements Updatable {
}));
}

public get mousePosition() {
return Pose2D.P;
}

public worldToScreenPosition(
worldPosition: Point,
offset: Point | null = null,
zoomLevel: number | null = null
): Point {
zoomLevel ??= this.zoomLevel;
offset ??= this._offset;
const spacing = Pose2D.ZOOM_SPACINGS[zoomLevel];
return new Point(
offset.x + worldPosition.x * spacing,
offset.y + worldPosition.y * spacing
);
}

public screenToWorldPosition(
screenPosition: Point,
offset: Point | null = null,
zoomLevel: number | null = null
): Point {
zoomLevel ??= this.zoomLevel;
offset ??= this._offset;
const spacing = Pose2D.ZOOM_SPACINGS[zoomLevel];
return new Point(
(screenPosition.x - offset.x) / spacing,
(screenPosition.y - offset.y) / spacing
);
}

public setSize(width: number, height: number): void {
this._width = width;
this._height = height;
Expand Down Expand Up @@ -317,39 +349,61 @@ export default class Pose2D extends ContainerObject implements Updatable {
return this._zoomLevel;
}

/**
* @param animate should the zoom happen in an animation over time, instead of instantaniously
* @param center should the camera move to the center of the screen
*/
public setZoomLevel(zoomLevel: number, animate: boolean = true, center: boolean = false): void {
if ((this._zoomLevel !== zoomLevel || center) && animate) {
if (this._zoomLevel === zoomLevel && center) {
if (Math.abs(this._width / 2 - this._offX) + Math.abs(this._height / 2 - this._offY) < 50) {
const needsToZoom = this._zoomLevel !== zoomLevel;
if ((needsToZoom || center) && animate) {
if (!needsToZoom && center) {
// TODO Guy: hypot is expensive and I don't even need the sqrt
const distanceFromCenter = Math.hypot(
this._width / 2 - this._offset.x,
this._height / 2 - this._offset.y
);
if (distanceFromCenter < 50) {
return;
}
}

this._startOffsetX = this._offX;
this._startOffsetY = this._offY;

let scaler = 1;
if (zoomLevel > this._zoomLevel) {
scaler = Pose2D.ZOOM_SPACINGS[zoomLevel] / Pose2D.ZOOM_SPACINGS[this._zoomLevel];
}
this._startOffset = this._offset.clone();

if (!this._offsetTranslating && !center) {
this._endOffsetX = scaler * (this._offX - this._width / 2) + this._width / 2;
this._endOffsetY = scaler * (this._offY - this._height / 2) + this._height / 2;
} else if (this._offsetTranslating) {
this._endOffsetX = scaler * (this._endOffsetX - this._width / 2) + this._width / 2;
this._endOffsetY = scaler * (this._endOffsetY - this._height / 2) + this._height / 2;
if (center) {
this._endOffset = new Point(this._width / 2, this._height / 2);
} else {
this._endOffsetX = this._width / 2;
this._endOffsetY = this._height / 2;
// TODO: should this be different? I need to respect if the mosue moved
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about this - I feel like something should break if the user moves their mouse while zooming, but I couldn't really find an issue when testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I finally found it:

chrome_sKjIFNZXIw.mp4

What happens is that if I zoom in, and while the animation is playing, move my mouse and then zoom in again, the mouse will not stay in the same world position (in this case it started outside the RNA, just to its right, but finished on the RNA)

I couldn't find a trivial and clean way to do this (which would happen by giving the screen/world conversion functions the current spacing in the middle of the animation, which isn't part of the ZOOM_SPACING array, but rather a linear value between the two).
I could calculate the value there, but that would mean that we'd calculate the progression in 3 different places, which I think is overkill.

Should I work on fixing that? Or should I document it as a known issue?

Copy link
Contributor Author

@guyguy2001 guyguy2001 Feb 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, I'm not sure if that would be enough to fix it, since the animation has to finish before we can zoom in again (or before the next zoom-in applies, IDK), so the mouse would have to move in world space either way

EDIT: No, that's probably wrong. I think the new animation starts immidiately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll open a new issue and resolve this thread before merging

// also what if the user presses the button?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to add a parameter to allow it to zoom into the center of the RNA, for when the user presses the button.
This would also be useful if we'd want to allow users to choose in the settings whether they want the zoom to zoom to the mouse or to the center of the RNA.

Copy link
Contributor Author

@guyguy2001 guyguy2001 May 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

A problem with that is that the +/- hotkeys just simulate clicking the button, and so it isn't possible to (easily) make them behave differently.
I think that it's very out of scope for now, and it only really affects users who do use the mouse but don't use the scroll wheel
EDIT: This means that I want to make the +/- keys scroll to the center of the screen

// also what if the user drags in the middle of the zoom?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dragging in the middle of the zoom is broken in live, and I don't see myself fixing it.

/** The offset after the current animation finishes */
const effectiveOffset = this._offsetTranslating ? this._endOffset : this._offset;

const oldMouseScreenPosition = this.mousePosition;
const mouseWorldPosition = this.screenToWorldPosition(
oldMouseScreenPosition,
effectiveOffset,
this.zoomLevel
);
const newMouseScreenPosition = this.worldToScreenPosition(
mouseWorldPosition,
effectiveOffset,
zoomLevel
);

// The mouse should stay at the same point in space before and after zooming,
// so move the offset to cancel the mouse movement.
this._endOffset = new Point(
effectiveOffset.x + oldMouseScreenPosition.x - newMouseScreenPosition.x,
effectiveOffset.y + oldMouseScreenPosition.y - newMouseScreenPosition.y
);
}

this._offsetTranslating = true;

this._zoomLevel = zoomLevel;
this.computeLayout(true);
this._redraw = true;
} else if (this._zoomLevel !== zoomLevel) {
} else if (needsToZoom) {
this._zoomLevel = zoomLevel;
this.computeLayout(true);
this._redraw = true;
Expand Down Expand Up @@ -522,8 +576,8 @@ export default class Pose2D extends ContainerObject implements Updatable {
if (out == null) {
out = new Point();
}
out.x = this._bases[seq].x + this._offX;
out.y = this._bases[seq].y + this._offY;
out.x = this._bases[seq].x + this._offset.x;
out.y = this._bases[seq].y + this._offset.y;
return out;
}

Expand All @@ -542,8 +596,8 @@ export default class Pose2D extends ContainerObject implements Updatable {

public getBaseOutXY(seq: number, out: Point | null = null): Point {
out = this._bases[seq].getOutXY(out);
out.x += this._offX;
out.y += this._offY;
out.x += this._offset.x;
out.y += this._offset.y;
return out;
}

Expand Down Expand Up @@ -1173,16 +1227,16 @@ export default class Pose2D extends ContainerObject implements Updatable {
if (!this._targetPairs.isPaired(startIdx)) {
// Update individual base coordinates.
this._bases[startIdx].setXY(
(mouseX - this._offX),
(mouseY - this._offY)
(mouseX - this._offset.x),
(mouseY - this._offset.y)
);

// Update the customLayout in the same way.
// Actually, after writing this, I no longer know why it works.
for (let ii = 0; ii < localCustomLayout.length; ++ii) {
localCustomLayout[startIdx] = [
localCustomLayout[ii][0] as number + (mouseX - this._offX) - this._bases[ii].x,
localCustomLayout[ii][1] as number + (mouseY - this._offY) - this._bases[ii].y
localCustomLayout[ii][0] as number + (mouseX - this._offset.x) - this._bases[ii].x,
localCustomLayout[ii][1] as number + (mouseY - this._offset.y) - this._bases[ii].y
];
}

Expand All @@ -1195,8 +1249,8 @@ export default class Pose2D extends ContainerObject implements Updatable {
for (const bp of stem) {
for (const idx of bp) {
this._bases[idx].setXY(
mouseX + this._bases[idx].x - origX - this._offX,
mouseY + this._bases[idx].y - origY - this._offY
mouseX + this._bases[idx].x - origX - this._offset.x,
mouseY + this._bases[idx].y - origY - this._offset.y
);
this._bases[idx].setDirty();

Expand Down Expand Up @@ -1226,7 +1280,7 @@ export default class Pose2D extends ContainerObject implements Updatable {
const fullSeqLen = this.fullSequenceLength;
for (let ii = 0; ii < fullSeqLen; ii++) {
const mouseDist: number = this._bases[ii].isClicked(
mouseX - this._offX, mouseY - this._offY, this._zoomLevel, this._coloring
mouseX - this._offset.x, mouseY - this._offset.y, this._zoomLevel, this._coloring
);
if (mouseDist >= 0) {
if (closestIndex < 0 || mouseDist < closestDist) {
Expand Down Expand Up @@ -1273,8 +1327,8 @@ export default class Pose2D extends ContainerObject implements Updatable {
if (!this._coloring) {
this.clearMouse();
}
const mouseX = this._bases[closestIndex].x + this._offX;
const mouseY = this._bases[closestIndex].y + this._offY;
const mouseX = this._bases[closestIndex].x + this._offset.x;
const mouseY = this._bases[closestIndex].y + this._offset.y;

this._paintCursor.display.x = mouseX;
this._paintCursor.display.y = mouseY;
Expand Down Expand Up @@ -1344,11 +1398,11 @@ export default class Pose2D extends ContainerObject implements Updatable {
}

public get xOffset(): number {
return this._offX;
return this._offset.x;
}

public get yOffset(): number {
return this._offY;
return this._offset.y;
}

public setOffset(offX: number, offY: number): void {
Expand All @@ -1359,8 +1413,8 @@ export default class Pose2D extends ContainerObject implements Updatable {
this._annotationDialog.display.x -= (this.xOffset - offX);
this._annotationDialog.display.y -= (this.yOffset - offY);
}
this._offX = offX;
this._offY = offY;
this._offset.x = offX;
this._offset.y = offY;
this._redraw = true;
}

Expand Down Expand Up @@ -1766,8 +1820,7 @@ export default class Pose2D extends ContainerObject implements Updatable {
}

public updateHighlightsAndScores(): void {
this._prevOffsetX = -1;
this._prevOffsetY = -1;
this._prevOffset = new Point(-1, -1);
this.generateScoreNodes();
}

Expand Down Expand Up @@ -2112,7 +2165,7 @@ export default class Pose2D extends ContainerObject implements Updatable {
const fullSeqLen = this.fullSequenceLength;
for (let ii = 0; ii < fullSeqLen; ii++) {
const mouseDist: number = this._bases[ii].isClicked(
mouseX - this._offX, mouseY - this._offY, this._zoomLevel, false
mouseX - this._offset.x, mouseY - this._offset.y, this._zoomLevel, false
);
if (mouseDist >= 0) {
if (closestIndex < 0 || mouseDist < closestDist) {
Expand Down Expand Up @@ -2748,7 +2801,7 @@ export default class Pose2D extends ContainerObject implements Updatable {
}

this._bases[ii].setDrawParams(
this._zoomLevel, this._offX, this._offY, currentTime, drawFlags, numberBitmap, hlState
this._zoomLevel, this._offset.x, this._offset.y, currentTime, drawFlags, numberBitmap, hlState
);
}
}
Expand Down Expand Up @@ -2784,6 +2837,17 @@ export default class Pose2D extends ContainerObject implements Updatable {

if (prog >= 1) {
prog = 1;
}

if (this._offsetTranslating) {
this._redraw = true;
this._offset = new Point(
prog * this._endOffset.x + (1 - prog) * this._startOffset.x,
prog * this._endOffset.y + (1 - prog) * this._startOffset.y
);
}

if (prog >= 1) {
this._offsetTranslating = false;

this.redrawAnnotations();
Expand All @@ -2793,12 +2857,6 @@ export default class Pose2D extends ContainerObject implements Updatable {
this.clearAnnotationCanvas();
}

if (this._offsetTranslating) {
this._redraw = true;
this._offX = prog * this._endOffsetX + (1 - prog) * this._startOffsetX;
this._offY = prog * this._endOffsetY + (1 - prog) * this._startOffsetY;
}

this.setAnimationProgress(prog);
} else if (currentTime - this.lastSampledTime > 2 && !this._isExploding) {
this.lastSampledTime = currentTime;
Expand All @@ -2811,7 +2869,8 @@ export default class Pose2D extends ContainerObject implements Updatable {
}

// Update score node
this.updateScoreNodeVisualization(this._offX !== this._prevOffsetX || this._offY !== this._prevOffsetY);
// TODO Guy: test
this.updateScoreNodeVisualization(!this._offset.equals(this._prevOffset));

// / Bitblt rendering
const needRedraw = this._bases.some(
Expand Down Expand Up @@ -2967,12 +3026,14 @@ export default class Pose2D extends ContainerObject implements Updatable {
if (this._isExploding && !this._offsetTranslating && this._baseToX == null) {
if (this._explosionStartTime < 0) {
this._explosionStartTime = currentTime;
this._origOffsetX = this._offX;
this._origOffsetY = this._offY;
this._origOffset = this._offset.clone();
}

this._offX = this._origOffsetX + (Math.random() * 2 - 1) * 5;
this._offY = this._origOffsetY + (Math.random() * 2 - 1) * 5;
// TODO Guy: Should we call setOffset here?
this._offset = new Point(
this._origOffset.x + (Math.random() * 2 - 1) * 5,
this._origOffset.y + (Math.random() * 2 - 1) * 5
);
this._redraw = true;

const prog = (currentTime - this._explosionStartTime) * 5;
Expand Down Expand Up @@ -3021,8 +3082,7 @@ export default class Pose2D extends ContainerObject implements Updatable {
}
}

this._prevOffsetX = this._offX;
this._prevOffsetY = this._offY;
this._prevOffset = this._offset.clone();
}

public lateUpdate(_dt: number): void {
Expand Down Expand Up @@ -3055,8 +3115,8 @@ export default class Pose2D extends ContainerObject implements Updatable {
const vx: number = this._baseToX[ii] - this._baseFromX[ii];
const vy: number = this._baseToY[ii] - this._baseFromY[ii];

const currentX: number = this._baseFromX[ii] + ((vx + (vx * progress)) / 2) * progress;
const currentY: number = this._baseFromY[ii] + ((vy + (vy * progress)) / 2) * progress;
const currentX = this._baseFromX[ii] + vx * progress;
const currentY = this._baseFromY[ii] + vy * progress;

this._bases[ii].setXY(currentX, currentY);
}
Expand Down Expand Up @@ -4300,15 +4360,12 @@ export default class Pose2D extends ContainerObject implements Updatable {
private lastSampledTime: number = -1;

// Pose position offset
private _offX: number = 0;
private _offY: number = 0;
private _prevOffsetX: number = 0;
private _prevOffsetY: number = 0;
private _offset: Point = new Point(0, 0);
private _prevOffset: Point = new Point(0, 0);
/** Are we currently animating a movement */
private _offsetTranslating: boolean;
guyguy2001 marked this conversation as resolved.
Show resolved Hide resolved
private _startOffsetX: number;
private _startOffsetY: number;
private _endOffsetX: number;
private _endOffsetY: number;
private _startOffset: Point;
private _endOffset: Point;

// For base moving animation
private _baseFromX: number[] | null;
Expand All @@ -4327,8 +4384,7 @@ export default class Pose2D extends ContainerObject implements Updatable {
private _isExploding: boolean = false;
private _explosionStartTime: number = -1;
private _explosionRays: LightRay[];
private _origOffsetX: number;
private _origOffsetY: number;
private _origOffset: Point;

private _onExplosionComplete: (() => void) | null;

Expand Down