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

DRAFT: ADR 61 - Volumetric Viewer #6193

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

DRAFT: ADR 61 - Volumetric Viewer #6193

wants to merge 1 commit into from

Conversation

kieftrav
Copy link
Contributor

@kieftrav kieftrav commented Aug 6, 2024

Package

lib-classifier

Describe your changes

Distillation of Volumetric Viewer conversation at ZTM. No effect on any code in the codebase so minimal review requirements from a deployment perspective.

@kieftrav kieftrav requested review from goplayoutside3 and a team August 6, 2024 13:48
@coveralls
Copy link

coveralls commented Aug 6, 2024

Coverage Status

coverage: 79.056%. remained the same
when pulling 855e8c2 on ADR-60
into b1564f7 on master.

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.

Requesting changes primarily for two reasons --

  1. The Decision section is lacking info about why you chose Three.js and how that technology will fit in the existing classifier.
  2. You mentioned annotation models in this Decision section without explaining how they're connected to viewing a 3D subject. The annotation model is much more closely tied to the tool, task type, and classification object eventually sent to panoptes. Mention of those models needs to be isolated to the other ADR, unless you'd like to combine the two in which case I've left some questions on DRAFT: ADR 62 - Volumetric Point Tool #6194 which would guide a combined discussion of viewer + tool + annotation + classification + task type.

Comment on lines +13 to +14
The Volumetric Viewer is a separate viewer than all the other viewers because of how it handles 3D data. The concept of pan, zoom, rotate, and annotation models take on a different level of complexity within a 3D context, therefore the viewer should be separate with tailored controls for the 3D context.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the word "separate" here can be confusing because some subject viewers are already "separate" from each other. For instance, the JSONDataViewer is completely separate from the FlipbookViewer. I recommend using language such as:

"The Volumetric Viewer is a unique viewer in that it handles a new type of data. The concept of pan, zoom, and rotate take on a 3D context, therefore the viewer will have tailored controls instead of the default ImageToolbar".

Copy link
Contributor

Choose a reason for hiding this comment

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

You could expand on the above statement by mentioning that the viewer will use all internal state for pan, zoom, and rotation rather than the MobX Store (if that's something you're fairly certain about, for instance I don't see any reason why you'd need a mobx store for those UI features).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also like to see description of "why Three.js" here. The prototyping phase involved heavy research into how to display 3D subjects in our existing classifier and any dev wishing to expand on the concept should have insight into that researching phase.

1. Mouse and/or Keyboard interactions for annotating the volumetric data in both 2D & 3D
1. Hiding of the ImageToolbar component (pan, zoom, rotate, invert, etc)

### Viewer Configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

What part of lib-classifier does this file live in? Are these properties meant to be added to the SubjectViewerStore?

## Features

Features of the FEM Volumetric Viewer will include:
1. Configuration of Volumetric Viewer for 3D volumetric and/or 2D planar view choice
Copy link
Contributor

Choose a reason for hiding this comment

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

Configuration implies setup in a dashboard like the project builder. "Toggling between 3D volumetric and/or 2D planar view" communicates the UI/UX intent.

@kieftrav kieftrav marked this pull request as draft August 15, 2024 15:09
@kieftrav kieftrav changed the title ADR 60: Volumetric Viewer DRAFT: ADR 61 - Volumetric Viewer Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants