-
Notifications
You must be signed in to change notification settings - Fork 30
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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 --
- The Decision section is lacking info about why you chose Three.js and how that technology will fit in the existing classifier.
- 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.
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. | ||
|
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
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.