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

lib-classifier: refactor single image viewer with VisXZoom #6390

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

Conversation

mcbouslog
Copy link
Contributor

@mcbouslog mcbouslog commented Oct 16, 2024

Package

  • lib-classifier

Linked Issue and/or Talk Post

Describe your changes

  • create new single image subject viewer
  • create storybook for new single image subject viewer
  • rotate
  • invert
  • add VisXZoom for pan and zoom (includes mouse wheel zoom)
  • drag
  • drawing / InteractionLayer
  • placeholder image
  • check new single image subject viewer in test project
  • check new single image subject viewer with other viewers that use single image viewer
  • fix broken tests
  • add new tests
  • update related READMEs (i.e. Subject Viewer, likely others)
  • review mobile/pinch handling new functionality will be investigated and potentially a separate PR

Notes

How to Review

Helpful explanations that will make your reviewer happy:

Checklist

PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.

General

  • Tests are passing locally and on Github
  • Documentation is up to date and changelog has been updated if appropriate
  • You can yarn panic && yarn bootstrap or docker-compose up --build and FEM works as expected
  • FEM works in all major desktop browsers: Firefox, Chrome, Edge, Safari (Use Browserstack account as needed)
  • FEM works in a mobile browser

General UX

Example Staging Project: i-fancy-cats

  • All pages of a FEM project load: Home Page, Classify Page, and About Pages
  • Can submit a classification
  • Can sign-in and sign-out
  • The component is accessible

New Feature

  • The PR creator has listed user actions to use when testing the new feature
  • Unit tests are included for the new feature
  • A storybook story has been created or updated
    • Example of SlideTutorial component with docgen comments, and its story

Refactoring

  • The PR creator has described the reason for refactoring
  • The refactored component(s) continue to work as expected

@coveralls
Copy link

coveralls commented Oct 16, 2024

Coverage Status

coverage: 78.5% (+2.9%) from 75.594%
when pulling a3f27a0 on lib-classifier_mouse-scroll-zoom
into 3d3aefc on master.

@eatyourgreens
Copy link
Contributor

eatyourgreens commented Nov 23, 2024

I just tried a few different browsers. Scaling and dragging seems to be ok in Chrome or Safari, but broken in Firefox.

@eatyourgreens
Copy link
Contributor

eatyourgreens commented Nov 23, 2024

This might be relevant. Dragging interactions get the current transformation matrix from a svg element stored in SVGContext here:

Creating a mark here gets the CTM from the DrawingCanvas component in the InteractionLayer component, which is an SVG rect that handles all the pointer events that create marks.

These refs don’t point to the same element, so that could account for differences in behaviour between creating a mark and dragging a mark. They probably should be using the same element and transformation matrix. I don't think I've noticed this before. At least, I can't find an issue for it, and I'm sure I would have opened an issue for something like this.

@eatyourgreens
Copy link
Contributor

eatyourgreens commented Nov 23, 2024

Update: dragging a zoomed-in mark is broken in Firefox on the production release. I'll open an issue. It's not a bug that's been introduced on this branch. The reason is that the draggable decorator is getting the screen CTM from the wrong DOM node, so not transforming pointer coordinates correctly.

Update to the update:

@mcbouslog
Copy link
Contributor Author

mcbouslog commented Jan 29, 2025

A brief update on a few issues previously noted:

  1. classification.metadata.subject_dimensions broken, reversion of previous bug - I think fixed per c58b452 and 7eb30ef and 0a2d54f
  2. marks with drag and move handles aren't scaled and working as expected - fixed with fix(lib-classifier): canvas context for SVG drawing tools #6491
  3. keyboard pan and zoom should be enabled in annotation mode - fixed with 0193a44
  4. marks aren't properly scaled zoomed in or out - fixed with refactor(classifier): use native browser SVG scaling for drawing tools #6066

@mcbouslog
Copy link
Contributor Author

I think this is ready for another review 😵‍💫 !

I've been using https://local.zooniverse.org:3000/projects/markb-panoptes/test-project-1-mb-fem-lab?env=staging&demo=true to test various workflows, image sizes, and drawing tools (see workflow descriptions).

@goplayoutside3
Copy link
Contributor

I did a first-pass review using Brave Browser on a Macbook Pro and want to jot down three things for now. I plan to test on another macbook and my phone later on!

  1. If the speed of the mouse-wheel zoom is adjustable, I think it should be a little slower. Especially because there's no limit on zooming out, the reaction time to the input from my track pad is jarring. I'll try to find a mouse with an actual wheel to test later too.
  2. Looking at metadata per classification, a flipbook subject ends up with more objects than expected in the metadata.subject_dimensions specifically when I switch back and forth between separate frames and flipbook.
    • When a subject is loaded in the separate frames view, for instance https://local.zooniverse.org:3000/projects/markb-panoptes/test-project-1-mb-fem-lab/classify/workflow/3429?env=staging&demo=true, there are 2 records of subject dimensions.
    • When a subject is loaded in the flipbook, and a classification submitted without switching to separate frames, there is 1 record of dimensions.
    • By switching back and forth between separate frames and flipbook 6 times, there are 11 records of subject dimensions in the metadata upon classification.
    • Based on this behavior, I think each individual SeparateFrame is somehow signaling onSubjectReady. Perhaps there should be a check for a set of dimensions existence in the metadata and don’t add more if it’s already there?
  3. We chatted about this in person, but I noticed that when I draw a point on a subject (regardless of viewer) and then rotate the subject 90deg, then zoom out, the size of the point increases. I'm not sure if this is related to scale or the stroke width of svg, but I don't believe it affects classification data, just visual differences.

Will continue testing on more devices by end of the week!

@eatyourgreens
Copy link
Contributor

#6697 should remove the image scaling calculation from this PR. It removes the scale prop (and subject image refs) throughout the subject viewers.

@eatyourgreens
Copy link
Contributor

eatyourgreens commented Feb 17, 2025

Looking at metadata per classification, a flipbook subject ends up with more objects than expected in the metadata.subject_dimensions specifically when I switch back and forth between separate frames and flipbook.

If it's any help, I'm seeing this bug in #6697 too. It appears to be a bug in the current production-release. I can also reproduce it by going to Daily Minor Planet in production, switching to separate frames mode and loading a subject, then switching back and forth between separate frames and flipbook. Separate frames pushes one dimensions object per frame to the array, flipbook pushes a single dimensions object to the array, and this happens each time that you switch between them.

onSubjectReady here will run multiple times, even if the loading state is success. I don't know if that's intentional.

onSubjectReady (event) {
const { target = {} } = event || {}
const {
clientHeight = 0,
clientWidth = 0,
naturalHeight = 0,
naturalWidth = 0
} = target || {}
self.dimensions.push({ clientHeight, clientWidth, naturalHeight, naturalWidth })
self.rotation = 0
self.loadingState = asyncStates.success
},

I did a quick check of #6581, which refactors the image handlers to use standard DOM interfaces, and that branch has the same bug too.

EDIT: I've updated #6581 to include a fix for this, by adding the image frame index to onSubjectReady so that it mirrors the corresponding PFE code.
https://github.com/zooniverse/Panoptes-Front-End/blob/326f657631ffc29e60ff93503ec0908c81802273/app/classifier/classifier.jsx#L202

@eatyourgreens
Copy link
Contributor

eatyourgreens commented Feb 17, 2025

Based on this behavior, I think each individual SeparateFrame is somehow signaling onSubjectReady. Perhaps there should be a check for a set of dimensions existence in the metadata and don’t add more if it’s already there?

This works when every frame has the same dimensions, but Squirrel Mapper (which has odd subjects) has subjects where each frame has different dimensions, including subjects where the frames are a mixture of portrait and landscape orientations. metadata.subject_dimensions has one entry per subject frame in PFE, to account for this.

There's also a potential bug when panoptes-uploads.zooniverse.org returns errors and subject frames don't load. In that case, it's still possible to submit a Daily Minor Planet classification, even if only one frame loaded. I think the onSubjectReady code only requires loading status to be success for one frame, not the whole subject.

Should subject loading success be 'one frame has loaded' or 'all frames have loaded'?

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

Successfully merging this pull request may close these issues.

Add mouse wheel zoom to image subject viewers
4 participants