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.

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

Draft: Fix fragment rendering on scaled canvases in bookview #3356

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 4 additions & 1 deletion __tests__/src/components/AnnotationsOverlay.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,10 @@ describe('AnnotationsOverlay', () => {
const context = wrapper.instance().osdCanvasOverlay.context2d;
expect(context.strokeStyle).toEqual('yellow');
expect(context.lineWidth).toEqual(20);
expect(strokeRect).toHaveBeenCalledWith(10, 10, 100, 200);
expect(strokeRect.mock.calls[0][0]).toBeCloseTo(24.561);
expect(strokeRect.mock.calls[0][1]).toBeCloseTo(24.561);
expect(strokeRect.mock.calls[0][2]).toBeCloseTo(245.61);
expect(strokeRect.mock.calls[0][3]).toBeCloseTo(491.22);
});
});

Expand Down
2 changes: 2 additions & 0 deletions src/components/AnnotationsOverlay.js
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,7 @@ export class AnnotationsOverlay extends Component {
annotation.resources.forEach((resource) => {
if (!canvasWorld.canvasIds.includes(resource.targetId)) return;
const offset = canvasWorld.offsetByCanvas(resource.targetId);
const scaleFactor = canvasWorld.scaleByCanvas(resource.targetId);
const canvasAnnotationDisplay = new CanvasAnnotationDisplay({
hovered: hoveredAnnotationIds.includes(resource.id),
offset,
Expand All @@ -356,6 +357,7 @@ export class AnnotationsOverlay extends Component {
},
},
resource,
scaleFactor,
selected: selectedAnnotationId === resource.id,
zoomRatio,
});
Expand Down
8 changes: 6 additions & 2 deletions src/lib/CanvasAnnotationDisplay.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,15 @@
export default class CanvasAnnotationDisplay {
/** */
constructor({
resource, palette, zoomRatio, offset, selected, hovered,
resource, palette, zoomRatio, offset, selected, hovered, scaleFactor,
}) {
this.resource = resource;
this.palette = palette;
this.zoomRatio = zoomRatio;
this.offset = offset;
this.selected = selected;
this.hovered = hovered;
this.scaleFactor = scaleFactor;
}

/** */
Expand Down Expand Up @@ -104,9 +105,12 @@ export default class CanvasAnnotationDisplay {

/** */
fragmentContext() {
const fragment = this.resource.fragmentSelector;
let fragment = this.resource.fragmentSelector;
fragment[0] += this.offset.x;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering now if perhaps we are calculating the offset incorrectly? Scaling the offsets before applying seem to be a useful transformation.

Copy link
Collaborator

@mejackreed mejackreed Dec 3, 2020

Choose a reason for hiding this comment

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

So I'm thinking we might need to do the following:

  1. Better calculate an offset based off of the scaling
  2. Resize the fragment/svg based off of scaling relationship to the canvas size

fragment[1] += this.offset.y;
if (this.scaleFactor && this.scaleFactor !== 1) {
fragment = fragment.map(x => x * this.scaleFactor);
}

let currentPalette;
if (this.selected) {
Expand Down
21 changes: 21 additions & 0 deletions src/lib/CanvasWorld.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,32 +28,44 @@ export default class CanvasWorld {
}

const [dirX, dirY] = this.canvasDirection;
// Smallest dimension in the given direction
const scale = dirY === 0
? Math.min(...this.canvases.map(c => c.getHeight()))
: Math.min(...this.canvases.map(c => c.getWidth()));
// Largest dimension in the given direction
const upScale = dirY === 0
? Math.max(...this.canvases.map(c => c.getHeight()))
: Math.max(...this.canvases.map(c => c.getWidth()));

let incX = 0;
let incY = 0;

const canvasDims = this.canvases.reduce((acc, canvas) => {
let canvasHeight = 0;
let canvasWidth = 0;

// Factor to scale elements on the canvas by to give the
// world position
let worldScale = 1;
if (!isNaN(canvas.aspectRatio)) {
if (dirY === 0) {
// constant height
canvasHeight = scale;
canvasWidth = Math.floor(scale * canvas.aspectRatio);
worldScale = upScale / canvas.getHeight();
} else {
// constant width
canvasWidth = scale;
canvasHeight = Math.floor(scale * (1 / canvas.aspectRatio));
worldScale = upScale / canvas.getWidth();
}
}

acc.push({
canvas,
height: canvasHeight,
width: canvasWidth,
worldScale,
x: incX,
y: incY,
});
Expand Down Expand Up @@ -198,6 +210,15 @@ export default class CanvasWorld {
};
}

/** scaleByCanvas - Get the scaling factor for the given canvas.
* For a given element on the canvas, multiplying its offset coordinates
* (i.e. [elemenX + canvasX, elemY + canvasY, elemWidth, elemHeight]) by
* this factor returns the coordinates of the element in the world
*/
scaleByCanvas(canvasTarget) {
return this.canvasDimensions.find(c => c.canvas.id === canvasTarget).worldScale;
}

/**
* worldBounds - calculates the "World" bounds. World in this case is canvases
* lined up horizontally starting from left to right.
Expand Down