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

VV - Implement Classifier Annotation Model #6568

Merged
merged 2 commits into from
Dec 19, 2024
Merged

VV - Implement Classifier Annotation Model #6568

merged 2 commits into from
Dec 19, 2024

Conversation

kieftrav
Copy link
Contributor

@kieftrav kieftrav commented Dec 16, 2024

Package

  • lib-classifier

Linked Issue and/or Talk Post

Describe your changes

  • Refactor SubjectViewer to no longer need to handle async components.. That work was moved into the viewer itself (VolumetricViewerWrapper)
  • Create Wrapper for VolumetricViewer in the Classifier that follows other Viewer patterns and has related tests
  • Moved detection of isVolumetricViewer into the Subject viewer detection function alongside all other viewer detection code
  • Add onAnnotation callback in the lib-subject-viewers VolumetricViewer component to communicate back to MobX the updated annotations

How to Review

  • All specs should pass
  • Classifier Storybook: Subject Viewers > VolumetricViewer
  • Classifier Storybook: Tasks > Volumetric
  • Load Staging VV Project in local Classifier and ensure that clicking on the cube / plane creates an annotation and the TaskArea tool updates to {n} drawn
  • Submit classification and inspect that the classification has annotation data

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

@coveralls
Copy link

coveralls commented Dec 16, 2024

Coverage Status

coverage: 77.369% (-0.02%) from 77.385%
when pulling e0cfcf7 on vv-annotations
into 2150c33 on master.

@goplayoutside3 goplayoutside3 self-assigned this Dec 17, 2024
@goplayoutside3
Copy link
Contributor

I haven't looked too deeply in to the code changes, but I started to review by running your staging project at:

In all three cases, I can trigger a crashing bug by clicking one or two annotations, and then scrolling my touchpad over the volumetric viewer. In Storybook, the error shows up hundreds of times in the console, but when the apps are run the page crashes. The error message is: TypeError: Cannot read properties of undefined (reading 'data') thrown by SortedSet.js in lib-subject-viewers.

Could you see if you can replicate this crashing bug? Am I using the correct test staging project?

@goplayoutside3
Copy link
Contributor

Here's a video of the bug with lib-classifier run locally.

bug.mov

@kieftrav
Copy link
Contributor Author

kieftrav commented Dec 17, 2024

@goplayoutside3 - I apologize for not clarifying that I did not do any work inside of the lib-subject-viewers to fix any of the bugs/issues that are caused by scrolling. If you were to pull and run my draft Styles PR, you'll see that this bug is fixed.

If I could constrain the focus of the PR to 1) does the annotation show up in the Task Area and 2) if an annotation is created, does it get submitted, that would be appreciated. All work fixing interactions with the VV are fixed in the Styles PR. Here's the changelog for the Styles PR:

  • Add Histogram range slider
  • Add params to overall Viewer
  • Fix scroll bug
  • Fix OrbitControls import
  • Incorporate Figma Styles
  • Inline OrbitControls

@goplayoutside3
Copy link
Contributor

Ah okay understood about further scroll behavior changes in the other styling PR.

Focusing on the annotations flow, I can click on the viewer and I see areas of the 3D subject become highlighted. The task area says "1 drawn". However, further clicks on the subject do not increment the task area beyond "1 drawn", even though new areas are highlighted in the subject viewer. This behavior is true when your test staging project is run locally with app-project or lib-classifier.

Is the annotation prop you've grabbed in VolumetricTask.js setup to count the number of annotations correctly? (I'm assuming I should be able to draw more than 1 annotation per subject)

@kieftrav
Copy link
Contributor Author

kieftrav commented Dec 18, 2024

Yup - also didn't clarify the testing action flow for creating multiple annotations. Thank you :)

So the interaction flow is:

  1. Left click will create a new annotation if no annotations exist. Further left-clicks add to the existing annotation
  2. Holding the "shift" button and then clicking will start a new annotation instead of adding to the existing annotation. The color of the next annotation will be different from the previous annotation. Green => Yellow => Orange => Red, for example. It's this interaction that increases the annotation count!

Copy link
Contributor

@goplayoutside3 goplayoutside3 left a comment

Choose a reason for hiding this comment

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

Gotcha. Using Shift + Click I can make multiple annotations of different colors and the task area label increments as expected. I assume the interaction instructions will be in tutorial and field guide.

I can also see the expected annotations inside a classification object when I hit Submit.

Just a note about classification data - The universal onSubjectReady() function is called as expected when the subject is ready to be interacted with. This function defined on the SubjectViewerStore also pushes metadata about clientHeight, clientWidth, naturalHeight, and naturalWidth into the classification object, and those values will always be zero for volumetric subjects. If the researchers ever ask:
Screenshot 2024-12-18 at 3 47 30 PM

@@ -0,0 +1,48 @@
import asyncStates from '@zooniverse/async-states'
import { lazy, Suspense } from 'react'
import { withStores } from '@helpers'
Copy link
Contributor

Choose a reason for hiding this comment

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

A non-blocking suggestion to use the useStores() hook instead. I know some of the oldest code like SubjectViewer.js still contains withStores(), but component wrappers and the "with" syntax are designed for Class components. useStores() hook is designed for functional components.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I appreciate that recommendation - cleaned up the code a nice little bit!

@github-actions github-actions bot added the approved This PR is approved for merging label Dec 18, 2024
@kieftrav kieftrav merged commit c9ecdc9 into master Dec 19, 2024
9 checks passed
@kieftrav kieftrav deleted the vv-annotations branch December 19, 2024 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved This PR is approved for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants