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

Upgrade OpenLayers to v9 #868

Merged
merged 28 commits into from
Jul 16, 2024
Merged

Upgrade OpenLayers to v9 #868

merged 28 commits into from
Jul 16, 2024

Conversation

mzur
Copy link
Member

@mzur mzur commented Jun 27, 2024

This is an attempt to upgrade OpenLayers from v5 to the current v9. It may offer better performance and new features.

Plan:

Known issues:

  • Capturing a screenshot does not produce a high-DPI image. Fix?
  • Drawing and modifying with a polygon brush is slower. Maybe newer/slower turfjs? Fix?
  • Selecting a complicated (magic wand) annotation is extremely slow. Fix?
  • Zooming and panning of images is sometimes (?) more janky. Fix?
    • The map object was accidentally made reactive through the minimap, which made everything slow.
  • Static image display is flickering sometimes during zoom/pan. Fix?
    • It's only the image/canvas layer that's flickering. The annotation layer remains visible!
    • No idea. Only observed this sometimes in Firefox when the dev console is open. Never in Chrome.
  • Video playback is faster with an updated implementation (that works with the minimap)!
  • Seems to fix Zooming causes video to disappear in firefox #626.
  • Zoom (to extent) buttons are smaller. Fix?
  • Update magic wand snapshot only when tool is active and using throttle().
  • Update video minimap more efficiently with source.change() instead of layer.change() so no render listener is required any more.
  • Tiled image minimap sometimes flickers.
  • Zoom to full image no longer has a padding. Fix?
  • Selecting multiple overlapping annotations seems different. Fix?
  • The video annotation tool shows a strange artifact at the bottom of the video.
    • Seems to be a Firefox thing. The video element is fine but once the video is drawn onto a canvas, the artifacts are there.
  • Look for possible bottlenecks (e.g. event listeners) in image/video drawing.
    • Double render through drawn video frame and then refreshed annotations?
    • Is the selectedAnnotations watcher necessary or does it double the work?

@mzur mzur self-assigned this Jun 27, 2024
@mzur mzur mentioned this pull request Jun 27, 2024
@mzur mzur linked an issue Jun 28, 2024 that may be closed by this pull request
mzur added 5 commits July 8, 2024 16:07
The old code removed and added all annotations again at all times.
A change is only required if annotations are de-/selected in the
sidebar.
This is no longer required with the new method of video rendering.
@mzur
Copy link
Member Author

mzur commented Jul 10, 2024

@dlangenk @lehecht Could you please check out this branch and try to break the image/video annotation tools when you have time? After checking out the branch you have to run npm install and npm run dev once. This PR upgrades the most important third-party library in BIIGLE four major versions and I don't want to publish it before adequate eyeballing.

@mzur mzur marked this pull request as ready for review July 10, 2024 13:55
@dlangenk
Copy link
Member

dlangenk commented Jul 10, 2024

I found a bug, which wasn't there before (at least I can reproduce it at biigle.de) [Should we post them here?]

  1. Activate the brush tool and draw an annotation.
  2. Deactivate it and click on the newly created annotation
  3. Activate the modify brush (T) and paint a new annotation to the top left of the current one, without intersection. (Edit: Anywhere to the left still leads to the bug)
  4. The previous annotation is now gone and the new one is there.

@dlangenk
Copy link
Member

I also get a warning in the console: No map visible because the map container's width or height are 0.

@dlangenk
Copy link
Member

That is everything I found. So great work🎉

@mzur mzur linked an issue Jul 11, 2024 that may be closed by this pull request
@mzur
Copy link
Member Author

mzur commented Jul 11, 2024

@dlangenk Thanks, this should be fixed now. It also fixed #873.

@lehecht Anything to add?

@mzur mzur changed the title Upgrade OpenLayers Upgrade OpenLayers to v9 Jul 16, 2024
@mzur mzur merged commit 2d0b462 into master Jul 16, 2024
6 checks passed
@mzur mzur deleted the ol-v9 branch July 16, 2024 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants