search

ridatadiscoverycenter / react-aframe-volume-renderer

afreame-webgl2 volumen renderer as a reactjs app
MIT License
3 stars 0 forks source link

refractor: Use volume viewer as an external library #64

Closed RobertGemmaJr closed 2 years ago

RobertGemmaJr commented 2 years ago

I'm trying to work out exactly how the VolumeViewer package will look. I think we should set up this repo in such a way that we consolidate the files needed for the package into one place, merge that, and use the consolidated folder to build the package as its own repo. Then we can set up this repo with the published package in a different PR. Let me know if that's a totally bad idea. Just a heads up, I refer to the package-to-be as VV a bunch.

Files

I created a VolumeViewer-package folder within src/ to hold the package files temporarily. React would get mad if I moved some of the needed files outside of src/. Here's what we have:

The package will also need it's own:

Things I'm not sure about:

Context

A big thing I'm looking for help on is deciding what state should be contained to the VV package and what state is specific to this (RIDDC) website. The same is true for what config.json values are pertinent to RIDDC versus the package itself. Below is a list of what we have so far and also some of my thoughts about where they should go.

controls-context

Context for the Volume Viewer controls. This will be within the package itself.

selector-context

Context for the RIDDC website. All of these values will inevitably be passed into the VV package as props.

Currently volumePosition, volumeRotation, and volumeScale are read from config.json directly inside of VolumeViewer.jsx. I think they should be read into selector-context.js and passed into VV as props from there. However, if we chose to expose these to the user, then I think we should expose the position of the camera as well. Should we let the user change these variables or is it reasonable to assume the end user will always want the model directly in the middle of the screen?

Volume Viewer config.json

Currently this is left empty but here's what I think should be moved into that file:

RIDDC config.json

This is mostly data about the volumes that is eventually passed into Aframe. It will include the min/max data points from branch #56 once it's merged into master. I know we've talked about this before - should we change this such that there is a .config file for each volume? The data min/max values are already different for each model. We'd have repeated values (e.g. each volume config would have "slices": 55 but it would simplify the structure of the config files for the RIDDC project and the general setup of future projects.

Volume Viewer props

The RIDDC website's specific state should be passed into the Volume Viewer package as props, (right?) so I'm thinking the package schema will look something like:

export default function App(props) {
  const { 
    useTransferFunction = true,       // Whether or not to color the model with the transfer function
    path = "",                        // Path to the model
    slices = 55,                      // Number of slices in the png
    spacing = {x: 2, y: 2, z: 1},     // Spacing of the slices, consolidated into 1 object
    position = "0 0 0",               // Position of the model
    rotation = "0 0 0",               // Rotation of the model, default isn't the rotation of the RIDDC models
    scale = "1 1 1",                  // Scale of the models, default isn't the scale of the RIDDC models
    dataRange = {min: 0, max: 0, unit: ""} // Data points used in OpacityControls.js from unmerged branch
  } = props;

  return (
    /* Components */
  )
}
github-actions[bot] commented 2 years ago

Visit the preview URL for this PR (updated for commit ec56dd7):

https://riddc-volume-viewer--pr64-feat-prep-package-zd10u3tv.web.app

(expires Tue, 02 Nov 2021 16:35:12 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

RobertGemmaJr commented 2 years ago

Should I include some way to pass the width and height of the component into the VV package? Or should users manage the sizing within their own application?

mcmcgrath13 commented 2 years ago

I'm trying to work out exactly how the VolumeViewer package will look. I think we should set up this repo in such a way that we consolidate the files needed for the package into one place, merge that, and use the consolidated folder to build the package as its own repo. Then we can set up this repo with the published package in a different PR. Let me know if that's a totally bad idea. Just a heads up, I refer to the package-to-be as VV a bunch.

I would just start moving to a new repo, you can test with a very simple example app there, or install from local files while developing here. That way you can get the folder structure, build setup, etc how you want for a package from the start. It will be a bit disconnected for a bit, but I think we can manage that given the current pace of development.

Files

  • assets/config.json: left empty for now. We need to figure out what should go there (see below). The color map images could go here, or should they be in the VV public/?

The config shouldn't be part of the new VV component. It's an implementation detail for the app using it in terms of how to determine/pass on the props needed by VV.

  • components/: The actual react components of VV. VolumeViewerWrapper would be App.jsx in VV.

I don't think we'll need an App.jsx in the library - take a look at https://hinammehra.medium.com/build-a-private-react-component-library-cra-rollup-material-ui-github-package-registry-1e14da93e790 (I am not an expert here and this is the first tutorial I clicked on that felt reasonable/at the right level/scope).

  • context/controls-context.js: context used within the package. Could change to volume-context.js or just context.js?

Just context seems fine given there will only be one.

  • libs/: not sure if these files are actually used but they were in the original aframe-react repo
  • loader/: not sure if these files are actually used but they were in the original aframe-react repo. From what I understand, a .nrrd file is a different way to load 3D data? So these files are how to load a .nrrd file into the project?

only migrate if used (not migrating and seeing what breaks is a way to find out). Skip the .nrrd loader at this point. We want to go as small/contained as possible, then can add in more if we need it.

  • misc/: not sure if these files are actually used but they were in the original aframe-react repo.

don't migrate unless definitely used

  • shaders/: looks to be the shader loaded into loader.js. Could we move the file into Aframe/ to consolidate the folders?

seems reasonable to me

The package will also need it's own:

  • firebase, would likely need help

Probably won't need firebase for the component.

Things I'm not sure about:

  • Dockerfile and .dockerignore - do we need docker for either project?

Ignore this - it's an artifact of how we used to deploy and definitely not needed for the component.

  • RE the different subfolders in VolumeViewer-package, I suppose it's safest just to move everything to th VV package for now? Perhaps in the future we could set it up to take either a .png or .nrrd? I don't know enough about the graphics side of things to make a call.

Only move things that are used and for now that means just .png

Context

A big thing I'm looking for help on is deciding what state should be contained to the VV package and what state is specific to this (RIDDC) website. The same is true for what config.json values are pertinent to RIDDC versus the package itself. Below is a list of what we have so far and also some of my thoughts about where they should go.

controls-context

Context for the Volume Viewer controls. This will be within the package itself.

  • allColorMaps: Constant array of the color maps used in the dropdown menu.

We can have a couple defaults, but this should be override-able via props.

  • sliderRange: Constant for the minimum and maximum value of the slider. Right now it's 0-1, I think this get's transposed to different x values?

I'd say keep this internal until we have a use case to start something clipped.

  • useTransferFunction: Boolean that right now it's always true. Perhaps this should be a prop passed into the VV package? e.g. the opacity controls wouldn't be displayed if it was passed in as false

Make a prop - it could even default to false and we don't look for color maps (or provide any) unless true

  • colorMap: The current color map

The color map control being split between the component and the exterior could probably use some thinking. Maybe initColorMap?

  • transferFunctionNodes: Array of points in the transfer function. What should be the default here?

Default can be the same default we use for riddc, but it again should be overridable, but probably initTransferFunctionNodes

  • xSliderBounds, ySliderbounds, zSliderBounds: Current start and end (minimum and maximum?) values of the sliders, used to cut the volume on each axis. Could be consolidated into a single object?

This seems like an internal implementation detail to me.

selector-context

Context for the RIDDC website. All of these values will inevitably be passed into the VV package as props.

  • selection: The values of the three ButtonGroups in ModelSelector.jsx. Used to build the path to the model, which is what actually gets passed into VV

This should instead be the path to the .png file.

  • slices: Constant number of slices in the png. As far as I understand, this is specific to each volume which is 55 for every model in RIDDC.
  • x_spacing, y_spacing, z_spacing: I think this is the spacing between each of the slices in the .png? I'm not really sure. It's definitely tied to each model.

Currently volumePosition, volumeRotation, and volumeScale are read from config.json directly inside of VolumeViewer.jsx. I think they should be read into selector-context.js and passed into VV as props from there. However, if we chose to expose these to the user, then I think we should expose the position of the camera as well. Should we let the user change these variables or is it reasonable to assume the end user will always want the model directly in the middle of the screen?

Agreed, I think we should expose these, but have reasonable defaults. Not sure about position of the camera - seems like we'd always want to have the model in the middle? The camera position prop could also be a future feature if needed.

Volume Viewer config.json

Currently this is left empty but here's what I think should be moved into that file:

I don't think there should be a config.json for the volume viewer. It will just have props and some of those will have default values.

RIDDC config.json

This is mostly data about the volumes that is eventually passed into Aframe. It will include the min/max data points from branch #56 once it's merged into master. I know we've talked about this before - should we change this such that there is a .config file for each volume? The data min/max values are already different for each model. We'd have repeated values (e.g. each volume config would have "slices": 55 but it would simplify the structure of the config files for the RIDDC project and the general setup of future projects.

I think the volume viewer component will be agnostic to this config, so no need to worry about it.

Volume Viewer props

The RIDDC website's specific state should be passed into the Volume Viewer package as props, (right?) so I'm thinking the package schema will look something like:

export default function App(props) { const { useTransferFunction = true, // Whether or not to color the model with the transfer function

if this is true, we also need color maps I think? default to false?

path = "",                        // Path to the model

skip the default on path since it is required.

slices = 55,                      // Number of slices in the png
spacing = {x: 2, y: 2, z: 1},     // Spacing of the slices, consolidated into 1 object
position = "0 0 0",               // Position of the model
rotation = "0 0 0",               // Rotation of the model, default isn't the rotation of the RIDDC models
scale = "1 1 1",                  // Scale of the models, default isn't the scale of the RIDDC models

@kmilo9999 are these reasonable defaults?

dataRange = {min: 0, max: 0, unit: ""} // Data points used in OpacityControls.js from unmerged branch

Maybe make the default max 1 to match a greyscale png.

} = props;

RobertGemmaJr commented 2 years ago

I would just start moving to a new repo, you can test with a very simple example app there, or install from local files while developing here. That way you can get the folder structure, build setup, etc how you want for a package from the start. It will be a bit disconnected for a bit, but I think we can manage that given the current pace of development.

Will Do! I'll probably make a few small changes here first but I'll link the repo once it's ready.

  • RE the different subfolders in VolumeViewer-package, I suppose it's safest just to move everything to th VV package for now? Perhaps in the future we could set it up to take either a .png or .nrrd? I don't know enough about the graphics side of things to make a call.

Only move things that are used and for now that means just .png

I'll try to keep a running list of what is/isn't actually need as I go along. Are there places where we keep "just in case" backups? @kmilo9999 let me know if you can tell what some of the JS files in the project that aren't used for RIDDC do.

  • allColorMaps: Constant array of the color maps used in the dropdown menu.

We can have a couple defaults, but this should be override-able via props.

We could keep the original color maps in the package and have RIDDC add the haline and thermal color maps?

  • colorMap: The current color map

The color map control being split between the component and the exterior could probably use some thinking. Maybe initColorMap?

Yeah this seems to be the biggest thorn in the design so far. We could expect a colorMap prop and default it to one stored in the component? It should be possible to have the component update the colorMap whenever the passed in prop changes OR it's changed in the controls but I'll have to do a little more research.

  • selection: The values of the three ButtonGroups in ModelSelector.jsx. Used to build the path to the model, which is what actually gets passed into VV

This should instead be the path to the .png file.

We still need a way to keep track of the season/tide/measurement separately for the buttons. I could just add the path as an additional state variable in the context?

Currently volumePosition, volumeRotation, and volumeScale are read from config.json directly inside of VolumeViewer.jsx. I think they should be read into selector-context.js and passed into VV as props from there. However, if we chose to expose these to the user, then I think we should expose the position of the camera as well. Should we let the user change these variables or is it reasonable to assume the end user will always want the model directly in the middle of the screen?

Agreed, I think we should expose these, but have reasonable defaults. Not sure about position of the camera - seems like we'd always want to have the model in the middle? The camera position prop could also be a future feature if needed.

In that case we should just expose the rotation and scale.

I don't think there should be a config.json for the volume viewer. It will just have props and some of those will have default values.

Makes total sense, the config is just an extra step for importing initial/constant values. Will do!

We'd have repeated values (e.g. each volume config would have "slices": 55 but it would simplify the structure of the config files for the RIDDC project and the general setup of future projects.

I think the volume viewer component will be agnostic to this config, so no need to worry about it.

Should it become an issue for the RIDDC website repo itself then? After the control points branch has been merged in?

useTransferFunction = true, // Whether or not to color the model with the transfer function

if this is true, we also need color maps I think? default to false?

I think when useTransferFunction is false we should use the default function and not expose the controls to the user. Is there a better name? What would be the reason for displaying the model without a color map?

Either way, I do think it would be a good idea to add a colorMaps and colorMap prop as discussed. The user can pass additional color maps and change the color map in ways other than just the controls.

skip the default on path since it is required.

How would I denote a prop as required? Just check to make sure it's not null or is there a react specific way?

dataRange = {min: 0, max: 0, unit: ""} // Data points used in OpacityControls.js from unmerged branch

Maybe make the default max 1 to match a greyscale png.

Okay yeah will do! Should the default initial color map be grayscale as well then?

mcmcgrath13 commented 2 years ago
  • RE the different subfolders in VolumeViewer-package, I suppose it's safest just to move everything to th VV package for now? Perhaps in the future we could set it up to take either a .png or .nrrd? I don't know enough about the graphics side of things to make a call.

Only move things that are used and for now that means just .png

I'll try to keep a running list of what is/isn't actually need as I go along. Are there places where we keep "just in case" backups? @kmilo9999 let me know if you can tell what some of the JS files in the project that aren't used for RIDDC do.

git/GitHub acts as backups, so no need to worry about losing things. It's here if we need it.

  • allColorMaps: Constant array of the color maps used in the dropdown menu.

We can have a couple defaults, but this should be override-able via props.

We could keep the original color maps in the package and have RIDDC add the haline and thermal color maps?

we probably want a mechanism to add or overwrite, but yeah, I think in general this is a good approach.

  • selection: The values of the three ButtonGroups in ModelSelector.jsx. Used to build the path to the model, which is what actually gets passed into VV

This should instead be the path to the .png file.

We still need a way to keep track of the season/tide/measurement separately for the buttons. I could just add the path as an additional state variable in the context?

But that's just for RIDDC. The VV only needs a path, right? This is also an issue in @kmilo9999 's current PR. I don't know if state makes sense, but maybe a method to compute it? In Vue this is done with computed properties or getters with vuex - seems like there should be a similar React thing.

We'd have repeated values (e.g. each volume config would have "slices": 55 but it would simplify the structure of the config files for the RIDDC project and the general setup of future projects.

I think the volume viewer component will be agnostic to this config, so no need to worry about it.

Should it become an issue for the RIDDC website repo itself then? After the control points branch has been merged in?

If it's a pain point with working with the RIDDC config, sure! But if it's working and not a pain point, I'd leave as is.

useTransferFunction = true, // Whether or not to color the model with the transfer function

if this is true, we also need color maps I think? default to false?

I think when useTransferFunction is false we should use the default function and not expose the controls to the user. Is there a better name? What would be the reason for displaying the model without a color map?

Either way, I do think it would be a good idea to add a colorMaps and colorMap prop as discussed. The user can pass additional color maps and change the color map in ways other than just the controls.

Would it make more sense from a graphics perspective to just show the raw data with no transfer function (greyscale)? Definitely shouldn't expose the opacity controls to the user either way.

skip the default on path since it is required.

How would I denote a prop as required? Just check to make sure it's not null or is there a react specific way?

I think not setting a default, letting people know in the documentation it's required, then erroring somehow if it's not passed.

dataRange = {min: 0, max: 0, unit: ""} // Data points used in OpacityControls.js from unmerged branch

Maybe make the default max 1 to match a greyscale png.

Okay yeah will do! Should the default initial color map be grayscale as well then?

I think no color map is greyscale already, so no need for a color map at all in that case. @kmilo9999 can confirm/deny

RobertGemmaJr commented 2 years ago

The discussion from before was when we were initially splitting volume-viewer into it's own package. This now uses the npm library