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 34 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
8a2889c
Add initial files for single image viewer with visx zoom
mcbouslog Oct 15, 2024
b274f8b
Add rotation to single image viewer
mcbouslog Oct 15, 2024
58d39be
Initial refactor single image viewer with VisXZoom
mcbouslog Oct 15, 2024
878edd9
Refactor single image viewer with VisXZoom mouse drag and zoom
mcbouslog Oct 15, 2024
09d2d92
Rename SingleImageViewer
mcbouslog Oct 24, 2024
0d203b0
Add PlaceholderSVG component
mcbouslog Oct 25, 2024
d598a4c
Fix drawing mark dragEnd
mcbouslog Oct 25, 2024
c93f219
Refactor viewers that use SingleImageViewer
mcbouslog Oct 30, 2024
39ad1a6
Add resetView to MultiFrameViewer
mcbouslog Nov 5, 2024
7b9da7a
Edit SeparateFrame SingleImageCanvas subjectId
mcbouslog Nov 6, 2024
3929816
Remove SVGPanZoom
mcbouslog Nov 6, 2024
b0ef4c6
Add PlaceholderSVG maxHeight
mcbouslog Nov 6, 2024
f6b8415
Refactor SeparateFrame with transform matrix
mcbouslog Nov 7, 2024
f0d4086
Refactor SeparateFrame with SingleImageContainer
mcbouslog Nov 7, 2024
75a568a
Refactor ImageAndTextViewer properties
mcbouslog Nov 7, 2024
260d87b
Refactor MultiFrameViewer with SingleImageViewer
mcbouslog Nov 7, 2024
467973b
Refactor FlipbookViewer and SeparateFrameViewer with SingleImage comp…
mcbouslog Nov 7, 2024
066619a
Revert FlipbookViewer handleFrameChange changes
mcbouslog Nov 7, 2024
49d6ca3
Add default props to SingleImageCanvas
mcbouslog Nov 7, 2024
ec4651c
Refactor FlipbookViewer with SingleImageContainer
mcbouslog Nov 7, 2024
0c27c95
Revert height and width edit in VisXZoom
mcbouslog Nov 7, 2024
c1a434e
Remove previous SingleImageViewer tests
mcbouslog Nov 12, 2024
f2f2a56
Add PlaceholderSVG stories and tests
mcbouslog Nov 12, 2024
8809d9f
Delete SingleImageConnector
mcbouslog Nov 13, 2024
f4bf4b0
Refactor ImageAndTextViewer and MultiFrameViewer to pass loadingState…
mcbouslog Nov 13, 2024
88c669e
Refactor FlipbookViewer with SingleImageViewer and useSubjectImage
mcbouslog Nov 13, 2024
9c0a872
Refactor SeparateFrame with SingleImageViewer and useSubjectImage
mcbouslog Nov 13, 2024
a52f9cd
Remove title default prop
mcbouslog Nov 13, 2024
39f75e4
Fix tests, cleanup
mcbouslog Nov 14, 2024
c0937af
Fix FlipbookViewer handleSpaceBar
mcbouslog Nov 14, 2024
9c81f38
Add initial SingleImageViewer tests
mcbouslog Nov 14, 2024
128c5b5
Merge branch 'master' into lib-classifier_mouse-scroll-zoom
mcbouslog Nov 14, 2024
e8a4c45
Update READMEs, remove SVGPanZoom references
mcbouslog Nov 14, 2024
c58b452
Refactor FlipbookViewer onReady exclusively per initial default frame…
mcbouslog Nov 20, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ describe('Components > Classifier', function () {
}

describe('while the subject is loading', function () {
let subjectImage, tabPanel, taskAnswers, taskTab, tutorialTab, workflow
let subjectImagePlaceholder, tabPanel, taskAnswers, taskTab, tutorialTab, workflow

before(function () {
sinon.replace(window, 'Image', MockSlowImage)
Expand All @@ -82,7 +82,7 @@ describe('Components > Classifier', function () {
)
taskTab = screen.getByRole('tab', { name: 'TaskArea.task'})
tutorialTab = screen.getByRole('tab', { name: 'TaskArea.tutorial'})
subjectImage = screen.getByRole('img', { name: `Subject ${subject.id}` })
subjectImagePlaceholder = screen.getByTestId('placeholder-svg')
tabPanel = screen.getByRole('tabpanel', { name: '1 Tab Contents'})
const task = workflowSnapshot.tasks.T0
const getAnswerInput = answer => within(tabPanel).getByRole('radio', { name: answer.label })
Expand All @@ -101,8 +101,8 @@ describe('Components > Classifier', function () {
expect(tutorialTab).to.be.ok()
})

it('should have a subject image', function () {
expect(subjectImage).to.be.ok()
it('should have a placeholder', function () {
expect(subjectImagePlaceholder).to.be.ok()
})

describe('task answers', function () {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,17 @@
import { render, screen } from '@testing-library/react'
import { render, screen, waitFor } from '@testing-library/react'
import { composeStory } from '@storybook/react'

import Meta, { Default, mockTasks } from './CenteredLayout.stories.js'

describe('Component > Layouts > Centered', function () {
it('should render a subject and a task', function () {
it('should render a subject and a task', async function () {
const DefaultStory = composeStory(Default, Meta)
render(<DefaultStory />)
expect(screen.getByLabelText('Subject 1')).exists() // img aria-label from SVGImage
expect(screen.getByText(mockTasks.init.strings.question)).exists() // task question paragraph

// Mock the loading state transition
Default.store.subjectViewer.onSubjectReady()

await waitFor(() => expect(screen.getByLabelText('Subject 1')).exists()) // img aria-label from SVGImage
await waitFor(() => expect(screen.getByText(mockTasks.init.strings.question)).exists()) // task question paragraph
})
})
Original file line number Diff line number Diff line change
@@ -1,13 +1,18 @@
import { render, screen } from '@testing-library/react'
import { render, screen, waitFor } from '@testing-library/react'
import { composeStory } from '@storybook/react'

import Meta, { Default, mockTasks } from './MaxWidth.stories.js'

describe('Component > Layouts > MaxWidth', function () {

it('should render a subject and a task', function () {
it('should render a subject and a task', async function () {
const DefaultStory = composeStory(Default, Meta)
render(<DefaultStory />)
expect(screen.getByLabelText('Subject 1')).exists() // img aria-label from SVGImage
expect(screen.getByText(mockTasks.init.strings.question)).exists() // task question paragraph

// Mock the loading state transition
Default.store.subjectViewer.onSubjectReady()

await waitFor(() => expect(screen.getByLabelText('Subject 1')).exists()) // img aria-label from SVGImage
await waitFor(() => expect(screen.getByText(mockTasks.init.strings.question)).exists()) // task question paragraph
})
})
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
import { render, screen } from '@testing-library/react'
import { render, screen, waitFor } from '@testing-library/react'
import { composeStory } from '@storybook/react'
import Meta, { Default, mockTasks } from './NoMaxWidth.stories.js'

describe('Component > Layouts > NoMaxWidth', function () {
it('should render a subject and a task', function () {
it('should render a subject and a task', async function () {
const DefaultStory = composeStory(Default, Meta)
render(<DefaultStory />)
expect(screen.getByLabelText('Subject 1')).exists() // img aria-label from SVGImage
expect(screen.getByText(mockTasks.init.strings.question)).exists() // task question paragraph

// Mock the loading state transition
Default.store.subjectViewer.onSubjectReady()

await waitFor(() => expect(screen.getByLabelText('Subject 1')).exists()) // img aria-label from SVGImage
await waitFor(() => expect(screen.getByText(mockTasks.init.strings.question)).exists()) // task question paragraph
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,11 @@ A subject viewer is a component designed to render the media of a subject and an

### Pan and Zoom

Currently, we have four implementations of pan and zoom functionality. An open [discussion](https://github.com/zooniverse/front-end-monorepo/discussions/2427) is on Github about how we support this overall and suggests consolidating where we can to use the VisX implementation so we can better support configuration and zoom to point since the VisX implementation already does. The documentation here will just be summarizing the types and where they are used.
Currently, we have three implementations of pan and zoom functionality. An open [discussion](https://github.com/zooniverse/front-end-monorepo/discussions/2427) is on Github about how we support this overall. The documentation here will just be summarizing the types and where they are used.

- [`VisXZoom`](components/SVGComponents/VisXZoom) - An extension of the VisX library's pan and zoom functional component. Uses a transformation matrix, boolean control for zoom and pan individual to turn the functions on and off as needed, has configurability for zoom direction, minimum zoom, maximum zoom, zoom in value, and zoom out value, and supports zoom to point. Currently implemented with the `ScatterPlotViewer`, the scatter plots used by the `VariableStarViewer`, and the scatter plot used by `DataImageViewer`. It uses the [`ZoomEventLayer`](components/SVGComponents/ZoomEventLayer) which is a transparent SVG rectangle that has event listeners for double click, on wheel, on key down, on mouse down, on mouse up, on mouse enter, and on mouse leave. Subject viewers using this should set as props the width, height, left and top points of the SVG area to render, as well as, the child subject viewer component to render, a zoom configuration object, and optionally a custom [constrain](https://airbnb.io/visx/docs/zoom#Zoom_constrain) function enabling the ability to constrain pan and zoom directionality and/or pan dimensions. An example of this implementation is used with the [`ZoomingScatterPlot`](components/ScatterPlotViewer/ZoomingScatterPlot). The zoom configuration can be set in the workflow configuration [`subject_viewer_config`](https://github.com/zooniverse/front-end-monorepo/blob/master/docs/arch/adr-27.md) object or in the JSON of a JSON subject. (NOTE: TODO is to finish building out support for configuration being set on the workflow configuration object).
- [`SVGPanZoom`](components/SVGComponents/SVGPanZoom) - A port of the PFE pan and zoom functionality being used with the `SingleImageViewer` and subsequently the `MultiFrameViewer` and anywhere else the `SingleImageViewer` may be used. It does scale transformations on the SVG view box.
- [`VisXZoom`](components/SVGComponents/VisXZoom) - An extension of the VisX library's pan and zoom functional component. Uses a transformation matrix, boolean control for zoom and pan individual to turn the functions on and off as needed, has configurability for zoom direction, minimum zoom, maximum zoom, zoom in value, and zoom out value, and supports zoom to point. Currently implemented with the `SingleImageViewer`, the single image viewer is used by `DataImageViewer`, `FlipbookViewer`, `ImageAndTextViewer`, `MultiFrameViewer`, and `SeparateFramesViewer`, as well as the `ScatterPlotViewer`, the scatter plots used by the `VariableStarViewer`, and the scatter plot used by `DataImageViewer`. It uses the [`ZoomEventLayer`](components/SVGComponents/ZoomEventLayer) which is a transparent SVG rectangle that has event listeners for double click, on wheel, on key down, on mouse down, on mouse up, on mouse enter, and on mouse leave. Subject viewers using this should set as props the width, height, left and top points of the SVG area to render, as well as, the child subject viewer component to render, a zoom configuration object, and optionally a custom [constrain](https://airbnb.io/visx/docs/zoom#Zoom_constrain) function enabling the ability to constrain pan and zoom directionality and/or pan dimensions. An example of this implementation is used with the [`ZoomingScatterPlot`](components/ScatterPlotViewer/ZoomingScatterPlot). The zoom configuration can be set in the workflow configuration [`subject_viewer_config`](https://github.com/zooniverse/front-end-monorepo/blob/master/docs/arch/adr-27.md) object or in the JSON of a JSON subject. (NOTE: TODO is to finish building out support for configuration being set on the workflow configuration object).
Copy link
Contributor

Choose a reason for hiding this comment

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

Just out of curiosity is the 'todo' at the end in a Github Issue or referenced somewhere else in the code?

Copy link
Contributor Author

@mcbouslog mcbouslog Nov 19, 2024

Choose a reason for hiding this comment

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

I think #2110 is the related issue. I'm not sure the NOTE belongs in the README though. I could remove the NOTE or update it to include the specific issue?

- [`D3`](components/LightCurveViewer) - Since the TESS light curve viewer's render and functionality is transferred to d3, the pan and zoom functionality is directly implemented using d3 and is coupled to the project requirements. This cannot be reused with other subject viewers.
- [`SubjectGroupViewer`](components/SubjectGroupViewer) - This uses a modified version of `SVGPanZoom`, but applies the scale transformation of the view box simultaneously to all subjects in the grid.
- [`SubjectGroupViewer`](components/SubjectGroupViewer) - It does scale transformations on the SVG view box simultaneously to all subjects in the grid.

### Drawing Interaction

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,14 +125,10 @@ const DataImageViewer = forwardRef(function DataImageViewer({
<SingleImageViewerContainer
enableInteractionLayer={false}
enableRotation={enableRotation}
imageLocation={imageLocation}
loadingState={loadingState}
move={move}
rotation={rotation}
subject={{
locations: [
imageLocation
]
}}
setOnPan={setOnPan}
setOnZoom={setOnZoom}
zoomControlFn={(zoomEnabled.image) ? () => disableImageZoom() : () => setAllowPanZoom('image')}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
import { useEffect, useState } from 'react';
import { Box } from 'grommet'
import PropTypes from 'prop-types'
import { useEffect, useState } from 'react'

import locationValidator from '../../helpers/locationValidator'
import { useSubjectImage } from '@hooks'

import SingleImageViewer from '../SingleImageViewer/SingleImageViewer.js'
import SVGImage from '../SVGComponents/SVGImage'
import SVGPanZoom from '../SVGComponents/SVGPanZoom'
import locationValidator from '../../helpers/locationValidator'

import SingleImageViewer from '../SingleImageViewer/SingleImageViewer'
import FlipbookControls from './components'

const DEFAULT_HANDLER = () => true
Expand All @@ -19,38 +18,17 @@ const FlipbookViewer = ({
flipbookAutoplay = false,
invert = false,
limitSubjectHeight = false,
move,
move = false,
onError = DEFAULT_HANDLER,
onKeyDown = DEFAULT_HANDLER,
onReady = DEFAULT_HANDLER,
playIterations,
rotation,
rotation = 0,
setOnPan = DEFAULT_HANDLER,
setOnZoom = DEFAULT_HANDLER,
subject
}) => {
const [currentFrame, setCurrentFrame] = useState(defaultFrame)
const [playing, setPlaying] = useState(false)
const [dragMove, setDragMove] = useState()
/** This initializes an image element from the subject's defaultFrame src url.
* We do this so the SVGPanZoom has dimensions of the subject image.
* We're assuming all frames in one subject have the same dimensions. */
const defaultFrameLocation = subject ? subject.locations[defaultFrame] : null
const { img, error, loading, subjectImage } = useSubjectImage({
src: defaultFrameLocation.url,
onReady,
onError
})
const {
naturalHeight = 600,
naturalWidth = 800
} = img

const viewerLocation = subject?.locations ? subject.locations[currentFrame] : ''

useEffect(() => {
enableRotation()
}, [img.src])

useEffect(() => {
const reducedMotionQuery = window.matchMedia('(prefers-reduced-motion: reduce)')
Expand All @@ -59,6 +37,18 @@ const FlipbookViewer = ({
}
}, [])

const imageLocationUrl = subject?.locations[currentFrame]?.url

const { img, error, loading, subjectImage } = useSubjectImage({
src: imageLocationUrl,
onError,
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing this from defaultFrameLocation to imageLocationUrl means useSubjectImage() hook is called on every frame because imageLocationUrl is determined by currentFrame rather than defaultFrame. This is undesired behavior because it directly calls onSubjectReady() every time a volunteer flips to a new frame. onSubjectReady() resets the rotation between frames and pushes new metadata to the classification.

Regarding rotation, we want volunteers to be able to rotate the subject and 'play' the flipbook with a subject rotated. rotation should not be reset in between every frame.

I noticed the metadata bug because earlier this year it was introduced by #5754 and fixed by #5952. On this branch, when I used my flipbook test project (listed in the FEM active projects for reference), this is what a submitted classification looks like when I flipped through 12 frames. Huge metadata causes classification exports to hang.
Screenshot 2024-11-18 at 3 32 35 PM


Questions - Is the change from defaultFrameLocation to imageLocationUrl intentional and required for the new SingleImageViewer with VisXZoom? How can each viewer ensure onSubjectReady() is called only once per subject, and not per frame? I didn't look into ImageAndTextViewer, MultiFrameViewer, and SeparateFramesViewer in detail yet, but I want to include those in this discussion too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Questions - Is the change from defaultFrameLocation to imageLocationUrl intentional and required for the new SingleImageViewer with VisXZoom?

I don't think it's required. I'll try refactoring so onSubjectReady (onReady passed from SubjectViewer) is only called once per subject and report back.

onReady
})
const {
naturalHeight = 600,
naturalWidth = 800
} = img

const onPlayPause = () => {
setPlaying(!playing)
}
Expand All @@ -67,57 +57,28 @@ const FlipbookViewer = ({
if (event.key === ' ') {
event.preventDefault()
onPlayPause()
} else {
onKeyDown(event)
}
}

const setOnDrag = (callback) => {
setDragMove(() => callback)
}

const onDrag = (event, difference) => {
dragMove?.(event, difference)
}

return (
<Box>
<SVGPanZoom
key={`${naturalWidth}-${naturalHeight}`}
<SingleImageViewer
enableInteractionLayer={enableInteractionLayer}
enableRotation={enableRotation}
frame={currentFrame}
imgRef={subjectImage}
invert={invert}
limitSubjectHeight={limitSubjectHeight}
maxZoom={5}
minZoom={0.1}
move={move}
naturalHeight={naturalHeight}
naturalWidth={naturalWidth}
setOnDrag={setOnDrag}
onKeyDown={handleSpaceBar}
rotation={rotation}
setOnPan={setOnPan}
setOnZoom={setOnZoom}
src={img.src}
>
<SingleImageViewer
enableInteractionLayer={enableInteractionLayer}
frame={currentFrame}
height={naturalHeight}
limitSubjectHeight={limitSubjectHeight}
onKeyDown={handleSpaceBar}
rotate={rotation}
width={naturalWidth}
>
<g role='tabpanel' id='flipbook-tab-panel'>
<SVGImage
ref={subjectImage}
invert={invert}
move={move}
naturalHeight={naturalHeight}
naturalWidth={naturalWidth}
onDrag={onDrag}
src={viewerLocation.url}
subjectID={subject.id}
/>
</g>
</SingleImageViewer>
</SVGPanZoom>
subject={subject}
/>
<FlipbookControls
currentFrame={currentFrame}
locations={subject.locations}
Expand All @@ -142,11 +103,13 @@ FlipbookViewer.propTypes = {
/** Passed from Subject Viewer Store */
invert: PropTypes.bool,
/** Passed from Subject Viewer Store */
limit_subject_height: PropTypes.bool,
limitSubjectHeight: PropTypes.bool,
/** Passed from Subject Viewer Store */
move: PropTypes.bool,
/** withKeyZoom() is for using keyboard pan and zoom controls while focused on the subject image */
onKeyDown: PropTypes.func,
/** Passed from Subject Viewer Store */
onError: PropTypes.func,
/** Passed from Subject Viewer Store */
onFrameChange: PropTypes.func,
/** Passed from Subject Viewer Store and called when default frame's src is loaded */
onReady: PropTypes.func,
/** Fetched from workflow configuration. Number preference for how many loops to play */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,10 @@ describe('Component > FlipbookViewer', function () {
it('should play or pause via keyboard when image is focused', async function () {
const user = userEvent.setup({ delay: null })

const { container, getByLabelText } = render(<DefaultStory />)
const imageSVG = container.querySelector('svg')
const { container, getByLabelText, getByTestId } = render(<DefaultStory />)
const imageSVGZoomLayer = getByTestId('zoom-layer')

imageSVG.focus()
imageSVGZoomLayer.focus()
await user.keyboard(' ')

const pauseButton = getByLabelText('SubjectViewer.VideoController.pause')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import asyncStates from '@zooniverse/async-states'
import PropTypes from 'prop-types'
import { observer } from 'mobx-react'

import { useKeyZoom, useStores } from '@hooks'
import { useStores } from '@hooks'
import locationValidator from '../../helpers/locationValidator'
import FlipbookViewer from './FlipbookViewer'
import SeparateFramesViewer from '../SeparateFramesViewer/SeparateFramesViewer'
Expand Down Expand Up @@ -64,20 +64,15 @@ function FlipbookViewerContainer({
setOnZoom
} = useStores(storeMapper)

const { onKeyZoom } = useKeyZoom(rotation)

useEffect(
function preloadImages() {
subject?.locations?.forEach(({ url }) => {
if (url) {
const { Image } = window
const img = new Image()
img.src = url
}
})
},
[subject?.locations]
)
useEffect(function preloadImages() {
subject?.locations?.forEach(({ url }) => {
if (url) {
const { Image } = window
const img = new Image()
img.src = url
}
})
}, [subject?.locations])

if (loadingState === asyncStates.error || !subject?.locations) {
return <div>Something went wrong.</div>
Expand All @@ -88,6 +83,7 @@ function FlipbookViewerContainer({
{separateFramesView ? (
<SeparateFramesViewer
enableInteractionLayer={enableInteractionLayer}
enableRotation={enableRotation}
onError={onError}
onReady={onReady}
subject={subject}
Expand All @@ -102,7 +98,6 @@ function FlipbookViewerContainer({
limitSubjectHeight={limitSubjectHeight}
move={move}
onError={onError}
onKeyDown={onKeyZoom}
onReady={onReady}
playIterations={playIterations}
rotation={rotation}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,10 @@ FlipbookControls handles the following features:
- `onReady`: (function) Function is passed from SubjectViewer and dimensions are added to classification metadata.
- `playIterations`: (string) Can be '', meaning infinite, or a number represented as a string. Set in the project builder and determines how many times to loop the flipbook.
- `rotation`: (number) Passed from the subject viewer store. Needed in SingleImageViewer to handle transforming (rotating) the image.
- `setOnPan`: (function) Passed from subject viewer store and used in SVGPanZoom.
- `setOnZoom`: (function) Passed from subject viewer store and used in SVGPanZoom.
- `setOnPan`: (function) Passed from subject viewer store and used in VisXZoom.
- `setOnZoom`: (function) Passed from subject viewer store and used in VisXZoom.
- `subject`: (object) Passed from mobx store via SubjectViewer.

### State Variables
- `currentFrame`: (number) Frame index that determines `viewerSrc`.
- `playing`: (boolean) Whether or not the looping feature is playing.
- `dragMove`: Used in SVGPanZoom.
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ function storeMapper (store) {
},
subjectViewer: {
dimensions,
enableRotation,
frame,
loadingState,
setFrame
Expand All @@ -17,7 +16,6 @@ function storeMapper (store) {

return {
dimensions,
enableRotation,
frame,
loadingState,
setFrame,
Expand Down
Loading
Loading