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

Add annotation outlines to Largo thumbnails #119

Merged
merged 82 commits into from
Jan 26, 2024
Merged

Conversation

lehecht
Copy link
Contributor

@lehecht lehecht commented Nov 29, 2023

  • create svg annotations for images
  • create svg annotations for videos
  • display svgs on top of annotation background
  • use css classes to style svgs

Added by @mzur:

  • A switch to enable/disable the outlines.
  • An new flag for the largo:generate-missing console command to only generate missing SVGs. The command should also check the SVG existence and regenerate SVGs if they are missing by default.
  • Update the manual.
  • Run npm run prod and commit the files of src/public/.
  • Also handle whole frame annotations (no SVG needed).
  • Add/update tests.

@lehecht
Copy link
Contributor Author

lehecht commented Nov 29, 2023

I forgot to extend the tests. I'm working on it.

@mzur mzur linked an issue Nov 29, 2023 that may be closed by this pull request
Exclude whole frame video annotations,
because they are not displayed as a shape.
Due to the svg annotation extension, the shape id for
image video annotations is always needed.
@lehecht
Copy link
Contributor Author

lehecht commented Nov 30, 2023

I noticed that if you don't set at least the stroke color, lines and points are not drawn any more. So I need to add it before sending it to the frontend. Or do you have a better idea @mzur ?

@mzur
Copy link
Member

mzur commented Nov 30, 2023

Do you have an example file? So if you don't set the stroke in the SVG, you can't set/add it via CSS later?

@lehecht
Copy link
Contributor Author

lehecht commented Dec 1, 2023

Do you have an example file? So if you don't set the stroke in the SVG, you can't set/add it via CSS later?

Ah, yes. I no longer saw the annotation and thought that the data would also be gone somehow. But thanks for this reminder!

@lehecht
Copy link
Contributor Author

lehecht commented Dec 1, 2023

How should I access the svgs? Currently they are named like the jpg just with the svg ending and are saved in the same directory.
Quick and dirty would be to use the filesystem/storage. Another idea would be to extend the Image/VideoAnnotation class and database with a xml field for those svgs. Or creating an own class database for the svgs. But maybe there is a simpler solution, @mzur ?

@mzur
Copy link
Member

mzur commented Dec 1, 2023

Currently they are named like the jpg just with the svg ending and are saved in the same directory.

That's the way to go, IMO. You can load the SVGs in the same way than the thumbnails (via their public URL, since this storage disk is public).

@lehecht
Copy link
Contributor Author

lehecht commented Dec 1, 2023

Currently they are named like the jpg just with the svg ending and are saved in the same directory.

That's the way to go, IMO. You can load the SVGs in the same way than the thumbnails (via their public URL, since this storage disk is public).

Ah okay. That was my first idea, but it seemed to easy and good to be accepted later. Thanks! 😄

@mzur
Copy link
Member

mzur commented Dec 1, 2023

Sometimes easy is also good 😄

@lehecht lehecht force-pushed the annotation-svg-thumbnail branch from df0a5f6 to 267fac6 Compare December 8, 2023 10:22
Whole frame video annotation were not considered when requesting svgs.
Bugfix handles these cases separately.
Separate SVG patch generation test from normal patch generation tests
@lehecht lehecht requested a review from mzur January 25, 2024 07:40
@lehecht
Copy link
Contributor Author

lehecht commented Jan 25, 2024

I merged the master branch into this one, because I was not sure If I create merge conflicts when merging this branch into the master at the end. This branch was based on an older version of the master and I edited a file there, which was also edited on the newer master branch.

Copy link
Member

@mzur mzur left a comment

Choose a reason for hiding this comment

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

Please read this before you act on my comments below 😉

I'm now almost finished with #120 and I would like to wrap up this PR, too. Almost everything that I would ask you to do here, I would have to change or do again during the merge between #120 and this PR. So I'd like to leave most things as they are now.

So here is what I'd like you to do to finish this PR:

  • Fix checking for video SVG files in the GenerateMissing command (see comment below).
  • Clarify why the point radius was changed (see comment below).
  • Revert changes to the GenerateImage/VideoAnnotationPatch jobs and their tests (see comments below).
  • Remove the GenerateImage/VideoAnnotationSVGPatch jobs and their tests again. This will be handled differently after the merge (there is a new ProcessAnnotatedImage job which will have flags to ignore patches/feature vectors/SVGs).
  • Write a text in this PR that I can paste into the updated manual article.
  • Compile and commit the files of src/public/.

src/Console/Commands/GenerateMissing.php Outdated Show resolved Hide resolved
src/Jobs/GenerateAnnotationPatch.php Outdated Show resolved Hide resolved
src/Jobs/GenerateImageAnnotationPatch.php Outdated Show resolved Hide resolved
src/Jobs/GenerateVideoAnnotationPatch.php Outdated Show resolved Hide resolved
This reverts commit e2c97bc.
Keep SVG generation call in GenerateImage/VideoAnnotationPatch
due to changes in #120.
@lehecht
Copy link
Contributor Author

lehecht commented Jan 25, 2024

@mzur
Manual: The 'show annotation outlines'-button can be used to show/hide annotation outlines on the thumbnail.

@lehecht lehecht requested a review from mzur January 25, 2024 10:41
@mzur
Copy link
Member

mzur commented Jan 25, 2024

Thanks a lot! This is a huge new feature 👍

I will take it from here.

@mzur mzur self-assigned this Jan 25, 2024
@mzur
Copy link
Member

mzur commented Jan 26, 2024

I decided to backtrack a bit (f7236f4) and display the SVGs as <img> (preventing dynamic styling) because:

  • The relabel image grid image should show the outlines, too, and an outline color does not make sense there.
  • The browser can cache the <img> much better and there is no flickering when switching back and forth between the dismiss and relabel steps.
  • I found that a dynamically colored outline is not really useful in the dismiss step, too (all patches always have the same color).
  • I could position the SVG <img> much more precise over the patch <img> so the outline position is exact now.

Copy link
Member

@mzur mzur left a comment

Choose a reason for hiding this comment

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

Thanks again for all the work @lehecht!

@mzur mzur merged commit c322781 into master Jan 26, 2024
2 checks passed
@mzur mzur deleted the annotation-svg-thumbnail branch January 26, 2024 13:48
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.

Extend Largo to show the outline of the annotations
2 participants