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

Fix occlusion rounding bug #3400

Merged
merged 4 commits into from
Sep 20, 2024
Merged

Conversation

taylorobyen
Copy link
Contributor

Fixes #3370

@dae
Copy link
Member

dae commented Sep 10, 2024

If you're aware, could I trouble you to provide a brief explanation of why this change fixes the problem? And could it be more succinctly expressed with Math.floor()?

@taylorobyen
Copy link
Contributor Author

I looked into this a bit more to get you an explanation, but actually found a better fix. The root cause for this issue is that the canvas size is determined inconsistently, 706 & 707 in this example. I found this in the calls to position.ts:

xFromNormalized() -> Math.round(x * width) = result
width:707 x:0.619 result:438
width:707 x:0.6204 result:439
width:707 x:0.6218 result:440
width:707 x:0.6232 result:441

xToNormalized() -> (x / width) = result
width:706 x:438 result:0.6203966005665722
width:706 x:439 result:0.6218130311614731
width:706 x:440 result:0.62322946175637

706 is the true size of the canvas, this is calculated when moving a shape on the canvas to save the data to the database. Below is the stack trace:

JS info /_anki/js/editor.js:100032 Error: Stack trace
    at xToNormalized (http://127.0.0.1:40000/_anki/js/editor.js:100030:13)
    at _Rectangle.toNormal (http://127.0.0.1:40000/_anki/js/editor.js:100230:16)
    at fabricObjectToBaseShapeOrShapes (http://127.0.0.1:40000/_anki/js/editor.js:104870:19)      
    at http://127.0.0.1:40000/_anki/js/editor.js:104820:14
    at Array.map (<anonymous>)
    at baseShapesFromFabric (http://127.0.0.1:40000/_anki/js/editor.js:104815:20)
    at exportShapesToClozeDeletions (http://127.0.0.1:40000/_anki/js/editor.js:104754:20)
    at MaskEditor.updateOcclusionsField (http://127.0.0.1:40000/_anki/js/editor.js:129943:32)     
    at http://127.0.0.1:40000/_anki/js/editor.js:78161:44
    at Array.forEach (<anonymous>)

This is determined in lib.ts from a function that grabs the bounds of the canvas:

export const getBoundingBox = () => {
    const canvas = globalThis.canvas;
    return canvas.getObjects().find((obj) => obj.fill === "transparent")
};

However, when the occlusion editor initially loads it determines the canvas size is 707. See below stack trace:

JS info /_anki/js/editor.js:100044 Error: Stack trace
    at xFromNormalized (http://127.0.0.1:40000/_anki/js/editor.js:100042:13)
    at _Rectangle.toAbsolute (http://127.0.0.1:40000/_anki/js/editor.js:100238:16)
    at _Rectangle.toFabric (http://127.0.0.1:40000/_anki/js/editor.js:100223:29)
    at addShape (http://127.0.0.1:40000/_anki/js/editor.js:100379:31)
    at addShapesToCanvasFromCloze (http://127.0.0.1:40000/_anki/js/editor.js:100402:9)
    at image.onload (http://127.0.0.1:40000/_anki/js/editor.js:105469:7)

This is determined in from-shapes.ts in addShape(). boundingBox is a invisible fabric.Rect that is the size of the canvas. The getBoundingRect method increases the absolute size by 1 pixel on both the X and Y axis.

const fabricShape = shape.toFabric(boundingBox.getBoundingRect(true));

My solution to this is to update the getBoundingBox function in lib.ts to return the size in line with what is calculated in from-shapes.ts.

export const getBoundingBox = () => {
    const canvas = globalThis.canvas;
    return canvas.getObjects().find((obj) => obj.fill === "transparent").getBoundingRect(true);
};

My alternate approach is below was to use the actual size of the canvas as the bounds in from-shapes.ts, but this felt less elegant:

const fabricShape = shape.toFabric({"width": boundingBox.width ?? 1, "height": boundingBox.height ?? 1});

@dae
Copy link
Member

dae commented Sep 20, 2024

Thanks Taylor, this looks like an elegant fix.

@dae dae merged commit 96ff4f1 into ankitects:main Sep 20, 2024
1 check passed
twwn added a commit to twwn/anki that referenced this pull request Sep 22, 2024
* master:
  Update translations
  Add an option to show image from editor in folder (ankitects#3412)
  Call the profile_did_open() hook earlier (ankitects#3421)
  Fix FSRS progress update issues (ankitects#3420)
  Update tooltip text (ankitects#3418)
  Fix pasting from the primary selection (ankitects#3413)
  Fix occlusion rounding bug (ankitects#3400)
  Add comment about the usage of the input field in the statistics page (ankitects#3394) (ankitects#3398)
  Possible to show “last” subdeck name in Browser? (ankitects#3387)
twwn pushed a commit to twwn/anki that referenced this pull request Sep 22, 2024
* Fix occlusion rounding bug

* Fix contributors
dae added a commit that referenced this pull request Sep 25, 2024
This reverts commit 96ff4f1.

This change broke adding of new occlusions on desktop:

JS error /_anki/js/editor.js:100016 Uncaught TypeError: Cannot read properties of undefined (reading 'getBoundingRect')
@dae
Copy link
Member

dae commented Sep 25, 2024

A heads up: unfortunately this broke adding of occlusions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image occlusion masks don't stay in the same place
2 participants