Skip to content
This repository was archived by the owner on Apr 18, 2024. It is now read-only.

fix: LSDV-4864: LEAP-148: Magic Wand doesn't work with MIG #1327

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

nick-skriabin
Copy link
Member

@nick-skriabin nick-skriabin commented Apr 19, 2023

Description

When both MIG and image preloading are enabled, there was no canvas overlay for the Magic Wand which caused the Magic Wand to fail.

Another problem was the data export. Magic Wand uses dataURL to operate. The MIG introduces a new RLE export mechanism that does not require access to the Konva's Stage object, instead it renders existing RLE/Touches directly onto an offscreen canvas. This mechanism did not involve rendering with data URL. With this PR we're adding support for the dataURL, yet it introduces a side effect – the serializeAnnotation method must be async. This is due to inability to draw dataURL images directly on the canvas. Instead, you must render it via Image, wait for it to load and then use ctx.drawImage to render an image, and waiting for image is an async process.

PR fulfills these requirements

  • Commit message(s) and PR title follows the format [fix|feat|ci|chore|doc]: TICKET-ID: Short description of change made ex. fix: DEV-XXXX: Removed inconsistent code usage causing intermittent errors
  • Tests for the changes have been added/updated (for bug fixes/features)
  • Docs have been added/updated (for bug fixes/features)
  • Best efforts were made to ensure docs/code are concise and coherent (checked for spelling/grammatical errors, commented out code, debug logs etc.)
  • Self-reviewed and ran all changes on a local instance (for bug fixes/features)

Change has impacts in these area(s)

(check all that apply)

  • Product design
  • Frontend

Describe the reason for change

LSDV-4864

What does this fix?

Fixes inability to use the Magic Wand when image preloading is enabled

What is the new behavior?

Magic Wand works with image preloading enabled

What is the current behavior?

Magic Wand doesn't work when the image preloading is enabled

What libraries were added/updated?

None

Does this change affect performance?

No

Does this change affect security?

No

What alternative approaches were there?

None

What feature flags were used to cover this change?

fflag_feat_front_dev_4081_magic_wand_tool
fflag_feat_front_lsdv_4583_6_images_preloading_short

Does this PR introduce a breaking change?

(check only one)

  • Yes, and covered entirely by feature flag(s)
  • Yes, and covered partially by feature flag(s)
  • No
  • Not sure (briefly explain the situation below)

What level of testing was included in the change?

(check all that apply)

  • e2e
  • integration
  • unit

Which logical domain(s) does this change affect?

Labeling Interface, Image Segmentation, Magic Wand

@codecov-commenter
Copy link

codecov-commenter commented Apr 19, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (a3e4f43) 68.69% compared to head (ea97df2) 68.70%.

Files Patch % Lines
src/components/Debug.js 16.66% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1327      +/-   ##
==========================================
+ Coverage   68.69%   68.70%   +0.01%     
==========================================
  Files         443      443              
  Lines       28721    28731      +10     
  Branches     7636     7633       -3     
==========================================
+ Hits        19729    19739      +10     
  Misses       7748     7748              
  Partials     1244     1244              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nick-skriabin nick-skriabin changed the title fix: LSDV-4864: Magic Wand doesn't work with MIG fix: LSDV-4864: LEAP-148: Magic Wand doesn't work with MIG Jan 11, 2024
Copy link
Contributor

@yyassi-heartex yyassi-heartex left a comment

Choose a reason for hiding this comment

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

there are a couple of very minor things that i wouldn't consider show stoppers but would be great to at least acknowledge if not accept suggestion

as.clearHistory();

// always check that history is for correct and submitted annotation
if (!history.length || !as.selected?.pk) return;
if (Number(as.selected.pk) !== Number(history[0].annotation_id)) return;
if (Array.isArray(history) && !history.length || !as.selected?.pk) return console.warn('Invalid history record', history);
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need check for array here? could it be something else and why?
we use it as array 2 lines down with no checks

Copy link
Member Author

Choose a reason for hiding this comment

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

it's js, it could be anything :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I assume at some point we were getting "cannot read length of undefined" or something like that. Honestly, I don't remember already why I did that. Shouldn't be a problem though. At least this code shows that we expect an array and nothing else.

nick-skriabin and others added 2 commits January 12, 2024 15:02
Co-authored-by: yyassi-heartex <[email protected]>
Co-authored-by: yyassi-heartex <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants