Change Flipbook and SeparateFrames viewers behavior to not clone drawing marks

[kieftrav]

Package

• lib-classifier

Linked Issue and/or Talk Post

Issue #5493
Issue #5833 (unresolved in this PR)

Describe your changes

• Pull the frame into and through the FlipbookViewer, SeparateFrameViewer, and SingleImageViewer components.
• Enable Interaction Layer in FlipbookViewer
• Use the 'setCurrentFramehook from theSubjectViewerStore` for all frame changes
• Update FlipbookViewer spec given serial vs async test running
• Moved mark process storage into the individual Subject store instead of SubjectStore
• Frame selection happens at the Viewer level
• All new marks get the actual frame value from where they were created (editing on a different frame won’t change the mark frame value)
• Because SeparateFrameViewer displays multiple frames, we isolate the subjectViewerStore.frame to only those that display 1 frame at a time (FlipbookViewer, MultiFrameViewer)
• PreviousMarks now gets passed the Frame reference because of SeparateFramesViewer

How to Review

• Pull down the branch and visit the classifier for the "Freehand Line Multiframe" project. There are two workflows, one for cloning marks and one for not.
• Each drawing task is now 3 Steps: Drawing Task 1, Yes/No question, Drawing Task 2
• Caesar now has 1 mark for 1 image in each drawing task. This means there will be 1 green mark pre-loaded for the first drawing task, and 1 blue mark loaded for the second drawing task.
• Each mark on a Subject from Caesar is unique to that Subject (before different subjects would have the same mark so it wasn’t easy to disambiguate between subjects and ensure clean state as we change subjects)

When deployed to project branch.

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

[kieftrav]

This refactor took quite a bit of thinking through state and how it updates + flows in the downstream components. I at first was trying to create local state and pass through + update the local state of the frame within the components before realizing I could piggyback on the way the frames are rendered to pass state. This made the code changes relatively minor (first couple tries weren't!). This currently does not have the "clone marks" functionality from issue #5493 - that's next.

[coveralls]

coverage: 81.186% (-0.01%) from 81.196%
when pulling c93f21f on flipbook-muliFrame-drawing-tools
into 0da2d53 on master.

[goplayoutside3]

This looks like a good starting point because it's essentially changing FlipbookViewer's frame handling into MulitFrameViewer's frame handling. MultiFrameViewer pulls its current frame from the store because transcription subject frames are always viewed one at a time. This is fine for flipbook when a project owner doesn't want to clone marks across frames.

I tried out the deployed test project and have some questions:

1. Is this drawn mark intentionally pre-populated on one subject? I saw it sometimes, but sometimes not... and I did not draw it myself.

---

2. When I draw a new line on a SeparateFrame, and then hit Done, the drawn mark is missing data in the completed classification. Here's a screenshot of a completed classification in the console, which has the correct frame, but it does not have the vital pathX and pathY data.

---

3. When I draw on a SeparateFrame component, the drawing tools controls component is very large and covers most of the subject image. Something with scaling is incorrect here...

---

4. For a test project with multiframe subjects, I suggest images of the same dimensions (like Daily Minor Planet). While building the "clone or not to clone" config, we'll need to test the play button for FlipbookViewer and it's a jarring UX when images are different dimensions from each other.

[goplayoutside3]

Just to double check since this PR is still marked as a draft - Is it ready for review or do some of the review points above still need to be addressed?

[kieftrav]

@goplayoutside3 - this is ready for review

[goplayoutside3]

Looking better! I tested with your project locally by running app-project. Existing flipbook projects like Daily Minor Planet also work as expected. See notes below about Correct a Cell.

The PR description is missing some relevant review things so I also tested:

• 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 PR creator has described the reason for refactoring
• The PR creator has listed user actions to use when testing the new feature
• Unit tests are included for the new feature
• The refactored component(s) continue to work as expected

Blocking Things

1. Please double check your changes to useFreeHandLineReductions against live projects that use the feature. This is my first experience reading the code that handles reductions, so I need a bit more guidance on how to test, but I think I already found a bug. I looked at the Correct a Cell workflow on production, and each subject loads with reductions on top of its image. However, when I run Correct a Cell locally on this branch, and submit a classification, all of the subsequent subjects loaded are showing 0 reductions. Perhaps I got a few subjects in a row that don't actually have reductions, but there needs to be some manual testing and documentation in this PR about how changes affect or don't affect live freehand line projects.

2. I recommend changing the PR title to "Change Flipbook and SeparateFrames viewers behavior to not clone drawing marks" to communicate the changes here.

[kieftrav]

@goplayoutside3 - ready for re-review. I saw the bug you found regarding not being able to submit multiple classifications after changing subjects. I moved all this code into the SubjectStore instead of the Workflow store (makes more sense to tie subject state in the subject store). I modified the test project to have 4 separate subjects so you can test the issue resolution within the current project.

I also tracked down the source of the LineControls zoom size issue.. It's not specific to this project but a more contained, separate bug. I created Issue #5833 to document this. I couldn't quite figure out how to fix it so I'd like to brainstorm with the team about ideas when it makes sense.

[goplayoutside3]

Thanks for the update! I'll take a look at this today and linked the corresponding PFE PR zooniverse/Panoptes-Front-End#6996 to be tested together.

Regarding #5833, I don't know of a solution of the top of my head, but after reviewing this PR I'll see if I can add any ideas to that Issue.

[goplayoutside3]

The recent changes fix the issue with loading reductions as I classify multiple subjects 👍 However, now we've got a problem with the frame prop being too rigid.

While I think it's fine to force frame to 0 in useFreehandLineReductions() based on whether a workflow clones marks or not, forcing the frame prop passed to viewer components has unintended consequences in the flipbook because the navigation controls and functionality of the viewer rely directly on that prop.

The Flipbook Viewer and Separate Frames Viewer should be free to react to the frame prop passed to them from the store. Instead of writing a conditional in FlipbookViewerContainer, I think a good strategy would be to modify frame in useFreehandLineReductions() hook or FreehandLineReductions.js store or a new helper function as needed.

Maybe there needs to be separation of the prop that determines "currently viewed frame" and the way frame is recorded per mark. For instance, on production Flipbook Viewer has local state (useState) as a strategy for "currently viewed frame" especially because projects like Daily Minor Planet autoplay the carousel, and rather than update the mobx-state-tree store on each render, only local state changes. Building on the existing architecture, is there a way to load freehand line reductions, put them in the store, and then each time the frame changes in FlipbookViewer use a helper function to grab the corresponding line reductions from the store? The store would act as source of truth for marks, and each mark object has a frame (i.e tool.createMark). In this scenario the same helper function would be called in a SeparateFrame component who's frame prop is passed as the index of the locations array for the subject.

[goplayoutside3]

This line introduces two problems:

1. When multiImageCloneMarkers is configured to true, frame is forced to 0 here, and the flipbook controls no longer work because frame is always 0. I tested this by loading your staging project via the PFE lab (https://www.zooniverse.org/lab/1993/workflows/3760?env=staging&pfeLab=true), I enabled "Clone markers in all frames", and then ran app-project locally. While the drawn marks are cloned correctly across frames in the separate frames view, I can't play or navigate between frames in the flipbook viewer.
2. By forcing frame to 0, it also overwrites default frame configuration that project teams can introduce per subject. See the example in FlipbookViewer's storybook here. The SubjectViewerStore detects if there's metadata.default_frame and sets frame accordingly, but should not be overwritten here in FlipbookViewerContainer. I suggest either creating stories or subjects in your test project with a configuration combo of default frame and clone / not clones marks.

[kieftrav]

Reverted this back - I didn't understand the interdependencies here so this helps me understand more contextually how this prop is used further in the component rendering pipeline :)

[kieftrav]

I reverted the changes to the SeparateFramesViewer, FlipbookViewerContainer, and useFreehandLineReductions to not modify the frame value based on workflow configuration. Instead, I inverted the responsibility into the part of the FlipbookViewerContainer that is responsible for choosing which marks to render per frame. I'm not sure of the unintended consequences of where I made this change - I don't understand the purpose of SHOWN_MARKS.NONE.

That said, I do think this becomes the right area to modify because it's the logic that controls which marks are rendered on which frame. Since the InteractionLayer is loaded for each frame of a drawing task, it's better to isolate this control flow into one file (i.e. you finding the bug meant instead of modifying 3 files we modify 1).

Finally, I had created a second project for cloneMarks enabled but hadn't added it to this PR description. I have updated the description with a local link to the project.

Also, not sure why coverage is failing. Running the classifier tests locally has no issues.

[goplayoutside3]

Not sure why tests failed either, but since this branch is several weeks behind master I'm going to update the branch and trigger Actions again.

[goplayoutside3]

While it's a good idea not to force frame number anymore, a couple of bugs still exist in the current implementation:

1. When I draw on the "don't clone marks" project while on separate frames, and submit my classification, the drawn mark is missing pathX and pathY again (seen in the browser console logs). This happened inconsistently on the "clone marks" project in separate frames view as well.
2. Do all of the subjects in this project have freehand line reductions? I do see the reductions load when using the flipbook, but after one classification with separate frames, I don't see any reductions load. This and the first bug seem specific to separate frames view.
3. InteractionLayer is used across lots of subject viewers, not just flipbook and separate frames. I think by modifying the code in that file, it's unintentionally broken multi-image subjects that use the MultiImageViewer. For example, load this transcription test project, draw a mark on the first frame and the mark will show up on the 2nd frame too. That project does not have "clone markers in all frames" enabled.

I think we might need to back up to the big picture of the requested feature.

Would you be willing to write an overview of the ideal data flow for this feature? For example, an idea of where marks are stored once reductions are loaded, where marks are stored when a volunteer draws a new mark on a subject viewer, how each of those marks obtains its frame value, how a subject viewer keeps track of its current frame, and lastly how to filter stored marks in the subject viewer depending on whether "clone marks" is true or false.

Manual testing of data when submitting a classification is crucial too. Unit tests can help for various cases, but it's helpful to load each of your test projects, draw on flipbook, draw on separate frames, and examine the submitted classification object in the console to double check the annotation. (Live FEM drawing projects with demo mode enabled is an option too)

[goplayoutside3]

To answer your question about SHOWN_MARKS.NONE, this bit of code handles the MetaTools UI below the subject viewer. The helper function for SHOWN_MARKS is here and pertains to the HidePreviousMarksButton component.

Volunteers can toggle marks on and off or filter to see their own without the ones pulled in from caesar.

For example in Correct a Cell, there are only two options: Hide Previous Marks or Show Previous Marks. In transcription projects, there's a third option to filter only your user's marks.

[kieftrav]

1) Not having pathX | pathY

Below are four videos wherein I can't replicate this issue in either cloneMarks enabled or disabled. I was only able to see this behavior after classifying the 4 subjects and getting the "you've already seen this subject" banner. The four videos are the four permutations - flipbook+clone, flipbook+noclone, separateframes+clone, separateframes+noclone.

2) Does each subject have reductions?

Yes, there are only 4 subjects. Each subject has 2 marks/reductions. Each subject has a different combination of frames for each subject (that's how they're different)... What I admit is a bit confusing is that the two marks have the same coordinates regardless of subject (simplicity of copy/paste in generating the data). I couldn't replicate the issue of not loading reductions in the 4 videos below.. I only didn't see the reductions once the "you've already seen this subject" banner appeared.

3) Interaction layer broke Multiframe (transcription project)

I tested this locally and replicated the issue.. The problem is that the MultiFrameViewerContainer was not passing the frame property into the SingleImageViewer, which then renders the InteractionLayer. This resulted in all of the frames=0 in the InteractionLayerContainer regardless of what the actual frame was. The most recent commit passes this prop down properly and fixes the bug.

4) Big Picture Control Flow

• SubjectViewer properly pulls in the right Viewer
• Viewer sets up each frame and passes its frame property into each sub-viewer (SingleImageViewer, for example). NOTE: This is not what happened before this PR. In MultiFrameViewerContainer never passed the frame={frame} prop down. Same with SeparateFramesViewer. This, to me, is a bug that went unnoticed.
• Individual Frame Views (SingleImageViewer for example) then passes the accurate frame prop to the InteractionLayer so that mark fetching, filtering, and rendering is done correctly.
• InteractionLayer then uses the correct frame prop that's been passed down all the way from the Viewer that's used so that when marks are created they have the current frame property.
• If we do all of the above then this PR will operate as expected

5) Big Picture Questions from Above

Where are marks stored once reduction is loaded?
Each mark is associated with a Subject so I'd imagine all Marks should be fundamentally tied to the Subject that they're connected to.

Where are marks stored when volunteer draws a new mark?
Same as once they're loaded. A mark is a mark. We can add an attribute that says it's from Caesar/Volunteer but it should be fundamentally stored+tied to the Subject.

How do each marks obtain their frame value?
If it's from Caesar, it should be part of the data passed from Caesar. If it's from the volunteer, it should get it's frame attribute from the InteractionLayer, which gets it from the SingleImageViewer, which gets it from the SeparateFramesViewer as it loops through the individual frames (as an example).

How does the subject viewer keep track of its current frame?
It shouldn't have to. It gets passed down as an accurate prop when state changes.

How to filter stored marks in the SubjectViewer?
So this shouldn't be in the SubjectViewer / SingleImageViewer but the InteractionLayer. The SubjectViewer / SingleImageViewer should determine if we render the InteractionLayer, not what's on the InteractionLayer. The InteractionLayer then controls volunteer interactions and already pulls the marks from the store, filters them for relevance, and then passes them in to the DrawingToolMarks component.

https://github.com/zooniverse/front-end-monorepo/assets/733986/36531ef0-8b2c-478e-9701-0b7069ab6b35
https://github.com/zooniverse/front-end-monorepo/assets/733986/d89d2df1-5e24-4447-bdb7-08f28ae42b86
https://github.com/zooniverse/front-end-monorepo/assets/733986/b129f1ac-dae3-4799-8184-47a8aa2392d6
https://github.com/zooniverse/front-end-monorepo/assets/733986/965febac-0f11-4c76-8167-38797db17fd1

[goplayoutside3]

I expanded on your data flow explanation after more investigation, and opened a draft PR #5851 to illustrate the points below. I think #5851 is on a more sustainable track than the changes in this PR at the moment. Please feel free it pull into this one, or continue working toward 5493 on my draft PR ( I can approve and merge that one if eventually needed ).

Fetching Reductions into the mobx-state-tree (MST) store

I only didn't see the reductions once the "you've already seen this subject" banner appeared.

I don’t think it’s expected behavior to not display caesar reductions if a volunteer has already seen a subject. We don’t want to waste a volunteer’s time, but it seems like a subject image and its reductions should always be coupled. That’s the precedent set by the transcription task.

Drawing tools projects’ reductions (only freehand line for now) are stored in the Subject.js MST store as Subject.caesarReductions via useCaesarReductions hook, which is called when a volunteer loads a new subject or new workflow. Following that logic, if a subject in the panoptes database has reductions, loading the subject on the frontend should always display its reductions too. Also, no need to replicate caesarReductionsLoadedForSubject in the SubjectStore because Subject.caesarReductions already exists.

Displaying Reductions per current frame

As mentioned above, freehand line reductions are fetched via useCaesarReductions hook and stored as Subject.caesarReductions.

A potential data flow: useCaesarReductions grabs all reductions for a subject regardless of “currently viewed frame” in the subject viewer. Then, when Subject.caesarReductions (MST store) is referenced for display in the flipbook or separate frames viewers, a helper component within those viewers accomplishes desired filtering.

• A conceptual example of filtering marks can be seen in the TranscribedLines component. This component grabs all lines (caesar reductions stored in MST store) and filters them based on whether a line still needs volunteer input or it’s complete.
• The DrawingToolMarks component serves the same purpose for drawing tools workflows as the TranscribedLines component does to transcription workflows.
• DrawingToolMarks is a child component of InteractionLayer and it gets passed an array of marks from InteractionLayerContainer, which grabs marks from the active task type (drawing or transcription) stored as Classification.annotations in the MST store.
• ❓ I think DrawingToolMarks and Subject.caesarReductions in the MST store are connected via useFreehandLineReductions hook in DrawingToolMarksConnector? I don’t have a good understanding of useFreehandLineReductions hook by reading the code, but in theory, I think this hook could be passed a frame param (the same frame passed to DrawingToolMarks) and it could filter reductions by frame.
• ❓ Some explanations that would help me understand - What is useFreehandLineReductions accomplishing, and why does it return only a task? I don’t understand marksUsed either.

Side note: In an ideal scenario, a project team who wants to clone all marks has reductions where each mark is frame: 0. If a project team’s reductions does not have frame: 0 on every mark and they want to clone marks, that might be something we coerce on the frontend for special cases (not in this PR). Conversly, when a project team does not want to clone frames, their reductions ideally include marks that specify which frame they should be displayed on.

Tracking Currently Viewed Frame

MultiFrameViewerContainer never passed the frame={frame} prop down. Same with SeparateFramesViewer. This, to me, is a bug that went unnoticed.

This is actually intentional because MultiFrameViewer was built to rely on SubjectViewerStore for frame, but SeparateFramesViewer intentionally does not.

MultiFrameViewer is a component designed to show only one frame at a time, and is exclusively used for transcription tasks where play/pause looping through multiple frames is never needed. MultiFrameViewer uses the SubjectViewerStore as a frame “source of truth”. This is fine because every frame in a transcription multi-image subject has different caesar reductions. Nothing is cloned across frames in transcription workflows. It’s tempting to simply use MultiFrameViewer for all workflows with drawing tools and multi-image subjects, but the separate frames feature, thumbnails below the subject, along with PFE parity are the motivation behind enabling drawing tools on FlipbookViewer and SeparateFramesViewer instead.

In contrast to MultiFrameViewer’s reliance on the MST store, FlipbookViewer currently uses local state to keep track of which frame is displayed. This works well especially for projects like Daily Minor Planet where the autoplay looping feature is crucial to a volunteer’s classification. Not all projects use the play/pause feature of flipbook, but when enabling drawing tools on the flipbook, we don’t want to degrade the UX for non-drawing flipbook projects by pushing a new frame value to the MST store when flipbook is looping 4 times per second.

Passing a locations index to SeparateFrame does works well for frame because it can be further passed onto InteractionLayer to track which separately displayed frame a volunteer clicked on to generate a new mark.

Newly drawn marks + current frame

Now we can consider how SubjectViewer > InteractionLayer > DrawingToolMarks plays a role here. If FlipbookViewer or SeparateFrame pass frame to InteractionLayer, the DrawingToolMarksConnector and DrawingToolMarks components can filter marks.

When a volunteer draws a new mark on the InteractionLayer, the mark is stored as an Annotation type on the Classification store (Classification.annotations). The Annotation model differs depending on the task type and tool type. For instance, when the active drawing tool is a freehand line, the Annotation model has marks as types.map(FreehandLine). A new mark utilizes createMark from the Mark model which takes a frame. This frame can come from InteractionLayer regardless of InteractionLayer’s parent component.

DrawingToolMarks gets passed marks from InteractionLayerContainer which got them from Classification.annotations. Therefore, as long as createMark is called with the desired frame, then DrawingToolsMarks can be passed a “currently viewed frame” prop and filter the marks appropriately.

InteractionLayer also has a PreviousMarks component for displaying marks from previous workflow steps during the same classification (i.e Elephant ID style workflows). The PreviousMarks component currently grabs frame from the SubjectViewerStore to determine which marks to display. This pertains to classification.previousInteractionTaskAnnotations in the MST store. It does not pertain to caesar reductions. PreviousMarks could be modified to get frame from its parent InteractionLayerContainer rather than grabbing it from the MST store (setting up a workflow with drawing tools and multiple steps is needed to test PreviousMarks functionality).

To Clone or Not to Clone

You had a similar implementation previously in this PR, but it was too rigid for the flipbook looping feature. In order to clone or not clone marks, I think frame can be coerced to 0 in InteractionLayerContainer instead of its parent viewer components. When not cloning marks, the currentFrame prop from FlipbookViewer or SeparateFramesViewer is passed to InteractionLayer for creating marks, otherwise frame=0 is passed to InteractionLayer.

Again, frame should be used accurately in createMark regardless of InteractionLayer’s parent component. In #5851 strategy, FlipbookViewer and SeparateFrame has its own local state for “currently viewed frame”, yet when we want to clone marks, I’m telling InteractionLayer that frame is always 0, even if the currently viewed frame is not 0 in FlipbookViewer or SeparateFrame.

Next Steps

The main follow-up steps to complete this feature are the addition of unit tests to #5851 (fix one of PreviousMark's tests too), and confirming that #5851 strategy can work with freehand line reductions in all cases. I did not test many live projects in #5851 yet, and left in-code comments to be filled in.

With your test projects, I recommend creating multiple workflows with a set of multi-image subjects (you don’t need multiple projecs, just multiple workflows). Test combos of drawing tasks with multiple steps (like Elephant ID), cloning or not cloning marks, and double check that live Zooniverse projects (Correct a Cell) won’t be affected. I also think an ADR summarizing this background info and final implementation would be super helpful.

[goplayoutside3]

Something to be careful with React - here you’re passing frame as a prop from SingleImageViewer to InteractionLayer, while InteractionLayerContainer is also grabbing frame from the SubjectViewerStore. There should be only one source of frame for InteractionLayer at any given time. This is the crux of deciding where a newly drawn mark gets its frame property from.

[goplayoutside3]

You’re passing frame as a prop from SingleImageViewer to InteractionLayer, while InteractionLayerContainer is also grabbing frame from the SubjectViewerStore. There should be only one source of frame for InteractionLayer at any given time.

This comment is still relevant with your latest changes~

The component InteractionLayer imported into SingleImageViewer.js is actually from InteractionLayerContainer.js. See the default export file here:

front-end-monorepo/packages/lib-classifier/src/components/Classifier/components/SubjectViewer/components/InteractionLayer/index.js

Line 1 in 2340ef2

export { default } from './InteractionLayerContainer'

Can frame be removed from InteractionLayerContainer's storeMapper() so there's not two sources of the frame prop?

[goplayoutside3]

There's no need to replicate caesarReductionsLoadedForSubject in the SubjectStore because Subject.caesarReductions already exists.

[kieftrav]

Thank you for the very thorough comment! High level, there's a couple things to clarify, and then I think the crux of the question is who is responsible for setting the frame property.

1. Fetching reductions

I can see continuing to show caesar reductions if a volunteer has already seen a subject. We should probably have a broader conversation about why we do the "you've already seen this subject" banner in the first place, but that's a sidebar conversation.

When we use the useCaesarReductions hook it operates asynchronously and gets called multiple times throughout the render process. Therefore, the caesarReductionsLoadedForSubject is a way to maintain whether or not the caesar reductions request has been completed and if we've processed the marks to have them associated with the subject. If we don't have this kind of check, we will add/create new marks every time the useCaesarReductions hook is invoked (which is a lot). This is also important state to maintain because if the volunteer deletes a caesar supplied mark, we don't want to re-create the mark whenever useCaesarReductions gets called again. This isn't the to say that this approach is the right way, but rather there is a very specific context and rationale as to why we are checking and maintaining this specific piece of state.

2. Displaying Reductions per current frame

A slight misunderstanding to clarify here is that setCaesarReductions does grab all the reductions for a subject - not a frame of a subject. When it stores those reductions in Subject.caesarReductions it's storing all reductions/marks for the subject on any/all frames of that subject.

useFreehandLineReductions is meant to be the glue that connects fetching the reductions asynchronously on subject change useCaesarReductions.js and then utilizing said reduction happens on every call to DrawingToolMarks. This means the useFreehandLineReductions gets called anytime we rerender the InteractionLayer (which is the 'a lot' I was mentioning above).

In theory, the hook could be passed the frame, but that wouldn't mitigate the issue of 1) what happens if the caesar mark has already been loaded / created and 2) what happens if the caesar mark has been deleted? The way this currently works to me is a little unnecessary... What I'd imagine instead of having this code run on every interaction layer render it instead has the reductions loaded via the store and then the UI listens for changes to the store so that the re-render only happens once and we're not running a bunch of code on everything that causes the InteractionLayer to re-render.

Side note about forcing frame: 0 is, to me, a secondary consideration that'll follow from the big picture decision of who is responsible for maintaining frame/currentFrame property in the viewer.

3. Tracking Currently Viewed Frame

I'm having a hard time understanding why we rely on the SubjectViewerStore as the source of truth so deep into the component stack instead of where it's actually needed/used (the Viewer level). To me, the components themselves dictate the data inheritance structure. The Viewer (which chooses Flipbook, MultiImage, etc) will have a subject that may or may not have multiple frames. The attribute of currentFrame would be the attribute that's stored in the SubjectViewerStore because the data it is responsible for is dependent upon the Viewer above (loosely, but a good example is that the SubjectViewerStore there is an attribute about the flipbookSpeed). What this means to me is that FlipbookViewer is a more fundamental source-of-truth than the SubjectViewerStore. If a subject has 5 frames, the FlipbookViewer has to know how to handle that and then may incorporate the SubjectViewerStore to get the currentFrame out of the 5 frames that the subject has. Instead, the way we've organized the code is to accept frame as the currentFrame, and renamed frames to locations so that we can sideload all of the frames that the subject has and then use setFrame() as a way to change the currentFrame in the store. What it looks like conceptually is combining the idea of frames as an attribute of a subject to frame as an individual frame we are (currently) viewing. This works in Flipbook but definitely doesn't work in SeparateFrameViewer.

What I'm proposing as the clear, unambiguous way of handling this is that frame always refers to the absolute index value of the Subject.location[i] that is currently viewed and that currentFrame, stored in the SubjectViewerStore is always the absolute index value that we want rendered now (in the case of Flipbook). If we wanted to change these to be locationIndex instead of frame to store this absolute value is a matter of naming consistency.. Whereas I'm trying to focus on conceptual consistency in this paragraph.

On your point about degrading non-drawing flipbook projects by pushing a new frame value... I don't think that's an actual issue. If the screen is being re-drawn, changing an individual store attribute isn't going to affect render performance (as compared to running the useFreehandLineReductions on every InteractionLayer rerender).

I'm not familiar with PreviousMarks so I'll have to dive into that more separately.

4. To Clone or Not to Clone
   I think forcing the frame to 0 in the InteractionLayerContainer is bad practice because it's putting the responsibility of a parent component into a child's child component. This makes the code brittle because the different viewers have different rendering paths and the InteractionLayerContainer gets rendered once in the case of Flipbook but multiple times in the case of SeparateFrameViewer. The impact of this is that the InteractionLayerContainer now has to be aware of which Viewer it got rendered from... In the case of Flipbook, all frames are 0. In the case of SeparateFrames, keep the inherited frame... Or, we can invert this responsibility and make it the FlipbookViewer / SeparateFrameViewer responsible for passing the right, current frame into the component that renders the InteractionLayerContainer. This is what I'm advocating for instead, and resolves the concern of picking which marks to render and ensuring createMark gets the correct frame through inheritance as opposed to custom viewer handling logic inside of the InteractionLayerContainer.

5. To summarize the above thoughts as best I can, I think we should:

• Have the frame attribute or locationIndex attribute always reference the currently rendered Subject index. If frame=0 it literally means the first frame in Subject.locations
• Have the 'currentFrame' attribute be stored in the SubjectViewerStore so it can be used wherever it's needed. It should be managed / maintained at the highest level of the Viewer that makes sense (i.e. Flipbook, MultiFrameViewerContainer) so that it can pass the correct frame/currentFrame as/if needed to sub-components (specifically SingleImageViewer).
• SingleImageViewer / InteractionLayerContainer / InteractionLayer / DrawingToolMarks should not care if the frame/currentFrame/locationIndex came from the SubjectViewerStore (in the case of Flipbook) or through the parent viewer looping (in the case of SeparateFramesViewer). It should inherit the correct value it needs and be able to pull that data without having to determine where that data should come from, and especially not where that data should come from depending on the parent Viewer.

I think the biggest reason to advocate for the changes I'm suggesting is that this solution handles the variety of Flipbook, SeparateFrame, and MultiFrame attributes that get passed to the SingleImageViewer (which does all the heavy lifting in all 3 cases). The biggest drawback to the SubjectViewerStore as a source of truth for currentFrame is that it doesn't work for SeparateFrames. By isolating the currentFrame responsibility to the Viewer itself, using SubjectViewerStore if needed, we eliminate sub-components needing to understand which parent component they came from.

[kieftrav]

Did a bit of refactor, cleanup, and lots of testing..

Updates:

• Each drawing task is now 3 Steps: Drawing Task 1, Yes/No question, Drawing Task 2
• Caesar now has 1 mark for 1 image in each drawing task. This means there will be 1 green mark pre-loaded for the first drawing task, and 1 blue mark loaded for the second drawing task.
• Moved mark process storage into the individual Subject store instead of SubjectStore
• Frame definition happens at the Viewer level
• All new marks get the actual frame value from where they were created (editing on a different frame won’t change the mark frame value)
• Because SeparateFrameViewer and MultiFrameViewer display multiple frames, we isolate the subjectViewerStore.frame to only those that display 1 frame at a time (FlipbookViewer)
• Each mark on a Subject from Caesar is unique to that Subject (before different subjects would have the same mark so it wasn’t easy to disambiguate between subjects and ensure clean state as we change subjects)
• PreviousMarks now gets passed the Frame reference because of SeparateFramesViewer & MultiFrameViewer

Update Tasks:

• Caesar reductions continue to show after “already seen” banner appears is now fixed
• Caesar reductions now have 1 mark loaded for 1 subject in each drawing task. In the Clone workflow this mark shows across all frames
• Deleted marks do not get re-added
• Hide previous marks hides all caesar-generated marks
• Does not affect Transcription projects
• All tests are passing locally.. Not sure what’s happening with GH CI
• When Done is clicked, all Caesar marks and Volunteer marks show up in the onComplete annotations

Issues for Future PR:

• Previous Marks are carried forward from a previous step to future steps (not sure if this is desired behavior)
• LineControls when viewed in Separate Frames doesn’t re-scale correctly

Screenshots:

[goplayoutside3]

Manual testing is looking good! I've left some questions on specific changes, but to address the following:

Previous Marks are carried forward from a previous step to future steps (not sure if this is desired behavior)

This is desired behavior. When a volunteer draws a mark on a step, and then goes to the next step, the previously drawn marks should remain as non-interactive. A live example of this is Elephant ID where the first step is to draw a point, and then answer a series of questions. The drawn point remains on the subject image while a volunteer answers the next steps' questions.

When I load https://local.zooniverse.org:3000/projects/kieftrav/freehand-line-multiframe/classify/workflow/3760, I draw on the subject image during the first step, and then click "Next" to the second step, the marks I've drawn remain. To my knowledge there hasn't been a Zooniverse project that involves freehand line reductions and multiple steps. Therefore, I don't know what the expected behavior is for those reductions, but I'll note that in the transcription project Corresponding with Quakers (which has transcription reductions), the caesar reductions only show on the first step. The second step asks volunteers a multiple choice question.

[goplayoutside3]

You’re passing frame as a prop from SingleImageViewer to InteractionLayer, while InteractionLayerContainer is also grabbing frame from the SubjectViewerStore. There should be only one source of frame for InteractionLayer at any given time.

This comment is still relevant with your latest changes~

The component InteractionLayer imported into SingleImageViewer.js is actually from InteractionLayerContainer.js. See the default export file here:

front-end-monorepo/packages/lib-classifier/src/components/Classifier/components/SubjectViewer/components/InteractionLayer/index.js

Line 1 in 2340ef2

export { default } from './InteractionLayerContainer'

Can frame be removed from InteractionLayerContainer's storeMapper() so there's not two sources of the frame prop?

[goplayoutside3]

By adding task.type !== 'drawing' here you're assuming that the current task a volunteer is being asked to complete is to draw something - is that correct?

Could there be a scenario where a subject is loaded with its freehand line reductions and the task is to answer "Are there machine generated lines on the image? Yes/no"? It's a contrived example, but should the task type really be coupled with findCurrentTaskMarks? I'm not sure about the correct answer, but wanted to ask for more details about why task.type must be drawing.

[kieftrav]

This is a very good point! When I built the code to fetch the reductions I made it as specific as possible to the step / task combo but there was nothing that said that was a requirement.. Your example is a good one where the mark should be loaded for a 1st task which is a question and a 2nd task which is then the actual drawing task. Code updated!

[goplayoutside3]

What is this function for? I don't see setIsUsed() anywhere else in the codebase, and looks like this PR removes it from useFreehandLineReductions() hook.

[kieftrav]

Cruft. Removed.

[goplayoutside3]

The changes in this file are still a bit confusing to me. The purpose of useFreehandLineReductions() is to fetch reductions via useCaesarReductions(), then filter the reductions for marks that belong to the current step. Is that the correct understanding? Or are the changes in this file intended to filter marks per task too, and why?

I think my confusion stems from knowing that in the FEM mobx store, each workflow has a map of steps (workflow.steps). Each step has an array of taskKeys, which maps to the workflow's tasks array. Essentially, tasks are grouped into steps, and having more than one of either is optional. There's further explanation in ADR 5, and ongoing work in the fem-lab to build the corresponding UI.

This question might help clarify too: Why is FreehandLineReductions.js's findCurrentTaskMarks concerned with the task.type? My understanding is that the FreehandLineReductions mobx store model is relevant when reductions fetched via useCaesarReductions() hook have a reducer that equals 'machineLearnt'. Why does reductions.data need to be filtered based on task type, tool type, etc?

Apologies for harping on this topic 😆 but because there are several changes in this PR to freehand line code files, I want to be sure a clear understanding is documented.

[kieftrav]

The useFreehandLineReductions() fetches reductions via useCaesarReductions() that get loaded per subject. They then were filtered for the current step, task, and tool. This was meant to ensure that's what the researcher wanted and is verifiable by the caesar data.

That said, as per your comment above with the "contrived example", I don't think that should be necessary. I think the constraint can instead be on the step it gets loaded and that's it. This is a good example to bring up to Cliff about how we load caesar data across the board and may be part of an ADR of some kind.

[goplayoutside3]

This line is related to my above questions in this file.

[kieftrav]

As per the comments above, this is being removed so it focuses exclusively on the step. The only benefit to also filtering by task is if you had 3 tasks in a step and you only wanted the marks to show on the 3rd task. The reason the code operated this way is that getting the step.tasks at this point always returned the current task (there was never an array of tasks). This perspective is from my own testing of configuring the workflow - nothing I sleuthed out in the code.

[goplayoutside3]

Thanks for the explanation about freehand lines - I think it'll prevent confusion and bugs related to task type in the future. While addressing behavior of caesar reductions and the freehand line tool, I think we've skipped over an important test for the combo of multi_image_clone_markers and frame. See my comment in InteractionLayerContainer for details.

[goplayoutside3]

By referencing multi_image_clone_markers only in this one spot, we're not meeting the expectation described in your comment on the linked Issue:

...when we clone marks across frames the mark.frameIndex will always be 0 instead of whichever frame the mark was drawn or edited.

Here's an example describing the combo of multi_image_clone_markers and frame:

I created a test production workflow where the subjects are multi-frame and there are two steps. Each step is one point tool. https://localhost.zooniverse.org:3000/projects/goplayoutside3/fem-flipbook-viewer/classify/workflow/24066?env=production

Run the project locally on this branch, and when multi_image_clone_markers is true, the mark I draw in the first step displays on all frames. I drew it on the first frame while testing. However, when I go to the next step, the mark I drew in the first step is no longer cloned and it only displays on the first frame. This is also true for the second step. For example if I'm on the second step, I draw a mark on 3rd frame and it displays as cloned, then go back to the first step, that mark is no longer cloned. It only shows up on the 3rd frame.

I believe this is happening because when multi_image_clone_markers is true, there's no handling of mark.frame for new marks in this PR. Try out the above test workflow by drawing marks on different frames, submit the classification and in the logged object, you'll see the annotations array contains marks with frame as the actual frame I clicked on. According to the comment above, new marks created when multi_image_clone_markers = true should all have frame: 0.

Accomplishing this might look something like interactionFrame = multiImageCloneMarkers ? 0 : frame in InteractionLayerContainer, and then passing interactionFrame to the InteractionLayer for its createMark() function. I think you'd also need to pass interactionFrame to PreviousMarks.

(feel free to toggle cloning config on my test workflow while in admin mode: https://www.zooniverse.org/lab/19645/workflows/24066).

[goplayoutside3]

Similar to my previous review - is this actually used anywhere? What is it for?

[kieftrav]

Fixed

[goplayoutside3]

@kieftrav copying over your comment from Slack:

...what I’m trying to accomplish is to have PreviousMarks render the right marks on the right frame depending on if cloneMarksAcrossFrames has been enabled. What I’m noticing is that advancing to the next task and rendering of the PreviousMarks is not operating as expected. Specifically in the scenario of not cloning marks.
How to recreate in my Drawing MultiFrame workflow locally:

1. Draw a mark on frames 0, 1, and 2
2. Click to switch to frame 3
3. Advance to the next task (yes/no question)

What happens is that the last frame you added a mark to will show the correct previous marks (in my case frame 2). None of the marks show up on/from the other frames (0, 1).

This description ^ is a recreation of the bug I described in this comment. It's fixed by using the solution in #5913.

What I did to dive into this is go to where the previous annotations / marks are pulled out. In this case, PreviousMarks references previousInteractionTaskAnnotations. In the PreviousMarks code if you console.log() these previousAnnotations what you end up getting is only the marks that were created on that frame.

I then went into the Classification.previousInteractionTaskAnnotations() and started console logging each step of the way for how it filters / processes annotations. The very first thing I notice is that the Caesar sent marks aren’t registered as marks in the current self.annotations object. If I then draw a mark it updates itself to have both the caesar mark and the mark I just created. If I decide to switch frames at this point, the console.log() continues to show these two markIds in the value attribute… But, if I now draw a new mark on this different frame, the annotation object now only has the mark I created on this frame. So, it feels like there are two bugs 1) caesar marks aren’t registered as potential previous marks unless a new mark is created / existing mark is edited and 2) marks created on different frames are no longer in the Classification.annotations object in the Classification.previousInteractionTaskAnnotation method.

If you’ve done the above, and then click the Next button to go to the next task it will create the issue I outlined at the very top: only the most recently drawn mark’s frame and associated marks gets recognized as PreviousMarks and the Classification.previousInteractionTaskAnnotations() has no memory of other mark(s) from other frames.

In short, PreviousMarks component does not care about whether there's a config for cloning or not cloning. It cares only about frame and self.annotations in the mobx store. See my implementation in #5913.

For caesar reductions:

It's okay that caesar marks aren’t registered as potential previous marks unless a new mark is created / existing mark is edited. This is not a bug. For instance, earlier in this PR discussion I mentioned that "in the transcription project Corresponding with Quakers, the caesar reductions only show on the first step." This is because the transcription caesar reductions are not previous marks made by the volunteer. It's fine to merge this PR without changing "caesar reductions as previous marks" behavior.

I also don't want to unnecessarily speculate further about a project that has different caesar reductions in two different steps. That's not config we've supported yet, and the zoo-team can handle that special case when it arises.

I opened #5913 to illustrate how to solve all of the above concerns. Please review + merge it into this PR, and then I'll approve this PR 👍

[kieftrav]

@goplayoutside3 - as discussed on #5913, the observed bug was unrelated to the changes made in that PR. I'm also going to copy over the reasoning about not changing the frame value at such a broad level so as to create hidden assumptions about the code. Instead, I want to make it abundantly clear in the code that ground reality has each frame maintain its true value and any deviation from that is documented in the code.. Particularly that we're pulling in a configuration value that says "ignore frame reality and pretend everything is 0".

====

Because modifying the frame is a data export issue decided in #5493 and is confined to the mark itself, I think it's important to keep the comment about the GitHub issue next to the area that sets its value to 0 instead of creating a chain of assumptions about why we changed the frame itself to 0 for the whole InteractionLayer when all that matters is that the classification export registers the mark's frame as 0 upon creation. I think this is particularly poignant because allowing the frame number to be set to its real frame when multiImageCloneMarkers is true doesn't change behavior at all - it still works as expected cloning all marks across all frames.

[goplayoutside3]

Noting that #5919 is included here 👍

Responding to your comment on #5913 for context since that PR will be probably be closed:

I don't think all of the other changes here make sense - they're focused on changing the frame on the container element instead of focusing on the mark itself having frame=0.

This is intentional. InteractionLayerContainer's purpose is to grab MST values and pass them to its children. When multiImageCloneMarkers = true, the frame number is coerced to 0 for all of InteractionLayerContainer's children. Setting frame = 0 in the parent of both InteractionLayer and PreviousMarks in #5913 is intentional.

Because modifying the frame is a data export issue decided in #5493 and is confined to the mark itself, I think it's important to keep the comment about the GitHub issue next to the area that sets its value to 0...

Yes agreed it's good to tag the decision in the linked Issue.

...instead of creating a chain of assumptions about why we changed the frame itself to 0 for the whole InteractionLayer when all that matters is that the classification export registers the mark's frame as 0 upon creation.

However, it's not just a data export decision. It matters that frame = 0 for PreviousMarks too. Nothing is exported from PreviousMarks.

I think this is particularly poignant because allowing the frame number to be set to its real frame when multiImageCloneMarkers is true doesn't change behavior at all - it still works as expected cloning all marks across all frames.

I don't think I understand this part - when would this scenario occur? When multiImageCloneMarkers is true, the frame number is not the real frame. It's coerced to 0 for all of InteractionLayerContainer's children.

Because we resolved the issue with losing track of marks in the PreviousMarks component (i.e. Jim's fix) the current PR works as expected as far as I can tell.

Correct. Issues are resolved in this PR so I'm approving it. Feel free to close other related PRs as needed, or take the above comments into consideration.

[goplayoutside3]

I think multiIimageCloneMarkers should simply be passed to InteractionLayer from its container rather than hooking up the store in this file. InteractionLayerContainer's purpose is to grab MST values and pass them to its children, so it's not worth breaking that pattern in this PR.

Also withStores is an outdated method for Class components. New connections to the MST store should have useStores instead.

[kieftrav]

Good point on this. I pulled all of the code around this out and instead am passing the multiImageCloneMarkers prop from the InteractionLayerContainer to the InteractionLayer.

Noted on useStores vs withStores going forward.