-
Notifications
You must be signed in to change notification settings - Fork 47
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
New Avivator #421
New Avivator #421
Conversation
@@ -23,7 +24,7 @@ flat out vec3 transformed_eye; | |||
void main() { | |||
|
|||
// Step 1: Standard MVP transformation (+ the scale matrix) to place the positions on your 2D screen ready for rasterization + fragment processing. | |||
gl_Position = proj * view * model * scale * vec4(positions, 1.0); | |||
gl_Position = proj * view * model * scale * resolution * vec4(positions, 1.0); |
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.
The reason for splitting this is that scale
encodes the pixel-based scaling of the volume while resolution is for "scaling up" to the volume's full resolution dimensions. We can't make one matrix unfortunately because scale
is used on the fragment shader (and pre-multiplying resolution
would make that problematic).
avivator/src/state.js
Outdated
newState[property] = [...state[property]]; | ||
channels.forEach((channel, j) => { | ||
newState[property].splice(channel, 1, values[j]); |
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.
This was a bug :(
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.
Minor notes on some API choices. Nice work!
getMultiSelectionStats({ | ||
loader, | ||
selections: newSelections, | ||
use3d: false | ||
}).then(({ domains, sliders }) => { | ||
setImageSetting({ | ||
onViewportLoad: () => { | ||
setPropertiesForChannels(range(newSelections.length), { | ||
domains, | ||
sliders | ||
}); | ||
setImageSetting({ onViewportLoad: () => {} }); | ||
setViewerState({ | ||
isChannelLoading: selections.map(i => false) | ||
}); | ||
} | ||
}); |
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.
Similar to the comment in the other PR, is it possible to remove the "multi" variants of these settings. It would be easier to use them in a modular manner. In addition, currently we need to wait for all the "channel" stats before updating any of the UI. Why not fire off all these changes asynchronously? Update each domain & slider independently, whenvever the data comes back.
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.
Similar to the comment in the other PR, is it possible to remove the "multi" variants of these settings.
You mean the multiple channel version? Sure I can just use the single channel version and remove the other.
In addition, currently we need to wait for all the "channel" stats before updating any of the UI. Why not fire off all these changes asynchronously? Update each domain & slider independently, whenvever the data comes back.
The idea here is that the channel stats come back much much quicker than the actual data often times for 3d volumes so we only want the sliders change once the 3d volume is loaded and ready for rendering. Otherwise you get a sort of flashing effect where the sliders change but the actual data has not.
`${truncateDecimalNumber(label === 'r' ? v : v / Math.PI, 4)}${ | ||
label === 'r' ? '' : 'π' | ||
}` |
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.
Possible to hoist this up into a function? hard for me to see what's going on. Not sure what r is.
function formatter(label)
const isRadius = label === 'r'; // useful variable name
return (value) => {
const val = truncateDecimalNumber(isRadius ? : v / Math.PI, 4);
...
}
}
//...
valueLabelFormat={formatter(label)}
setClippingPlaneSettings(0, 'radius', radius); | ||
setClippingPlaneSettings(0, 'phi', phi); | ||
setClippingPlaneSettings(0, 'theta', theta); |
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.
Why not use an object?
setClippingPlaneSettings(0, { radius, phi, theta });
setViewerState({ | ||
isChannelLoading: selections.map(_ => true) | ||
}); | ||
getMultiSelectionStats({ loader, selections, use3d: !use3d }).then( | ||
({ domains, sliders }) => { | ||
setPropertiesForChannels(range(selections.length), { | ||
domains, | ||
sliders | ||
}); | ||
setViewerState({ | ||
isChannelLoading: selections.map(_ => false) | ||
}); | ||
} |
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.
Another example of where getting stats independently would allow the UI to update on demand rather than waiting for each channel before any UI update.
export function truncateDecimalNumber(value, maxLength) { | ||
if (!value && value !== 0) return ''; | ||
const stringValue = value.toString(); | ||
return stringValue.length > maxLength | ||
? stringValue.substring(0, maxLength).replace(/\.$/, '') | ||
: stringValue; | ||
} | ||
|
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.
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 issue is its behavior on non-floats, like
Number.parseFloat(1234).toFixed(6)
"1234.000000"
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.
Maybe there's another way of doing this?
* 3D Viv Core (#409) * Initial commit * CHANGELOG. * Address comments. * Prettier. * 3D Docs (#411) * First pass at docs. * Clean up. * Add to docs. * Spelling & brief docs. * Change docs target * Update docs/3D_RENDERING_BRIEF.md Co-authored-by: Trevor Manz <trevor.j.manz@gmail.com> Co-authored-by: Trevor Manz <trevor.j.manz@gmail.com> * Fix 3D types/docs * Remove header. * Basic 3D Avivator (#412) * Initial commit * CHANGELOG. * Address comments. * Prettier. * [WIP] Avivator first pass. * Fix webpack config. * Fix restricted syntax. * 3D Non-Orthogonal Clipping Planes (#418) * Initial commit * CHANGELOG. * Address comments. * Prettier. * [WIP] Avivator first pass. * Add non-orthogonal clipping planes. * Use `flat` * Fix typo * Fix numPlanes padding * Fix plane equation. * Use sign function instead of ceil * Remove cast. * Use Plane API * Remove `numPlanes` argument. * (For 3D) Avivator Refactor (#420) * Initial commit * CHANGELOG. * Address comments. * Prettier. * [WIP] Avivator first pass. * [WIP] State management, some updates to Avivator. * [WIP] Adding more use of the global state. * [WIP] More state replacement. * Change color palette selector. * Use imgae settings. * Use viewer store. * Clean up unnecessary Avivator props. * Create self-contained buttons. * Refactor main `useEffect` to use hooks. * Fix snackbars. * Refactor Viewer. * Change names of buttons. * Refactor Snackbars * Remove unused vars. * Refactor menu. * Reset with new image. * Fix linting error. * Refactor props -> global state for dropzone components. * Remove more unnecessary props. * Colorpalette use global state. * Refactor selections. * Fix async selection bug. * Fix viewState/initialViewState issue. * Fix toggle. Remove dead code. * Small ids fix. * Refactor channel stats. * Use > instead of epsilon. * Revert loader change. * Fix import paths. * Tweaks. * Prettier. * Fix state bug and lens display. * Handle undefined loader in state. Small entries suggestion. * Reform state management. * Small code compleixty change. * Fix artifacts for additive. * Small Features/Fixes for Viv Src (#422) * Fixes for viv src * Update src/layers/ImageLayer.js Co-authored-by: Trevor Manz <trevor.j.manz@gmail.com> * Prettier, Co-authored-by: Trevor Manz <trevor.j.manz@gmail.com> * New Avivator (#421) * Initial commit * CHANGELOG. * Address comments. * Prettier. * [WIP] Avivator first pass. * [WIP] State management, some updates to Avivator. * [WIP] Adding more use of the global state. * [WIP] More state replacement. * Change color palette selector. * Use imgae settings. * Use viewer store. * Clean up unnecessary Avivator props. * Create self-contained buttons. * Refactor main `useEffect` to use hooks. * Fix snackbars. * Refactor Viewer. * Change names of buttons. * Refactor Snackbars * Remove unused vars. * Refactor menu. * Reset with new image. * Fix linting error. * Refactor props -> global state for dropzone components. * Remove more unnecessary props. * Colorpalette use global state. * Refactor selections. * Fix async selection bug. * Fix viewState/initialViewState issue. * Fix toggle. Remove dead code. * Small ids fix. * Refactor channel stats. * Use > instead of epsilon. * Revert loader change. * Fix import paths. * Tweaks. * Prettier. * Styling. * Slicers using polar coordinates. * Merge branch 'ilan-gold/avivator_refactor' into ilan-gold/new_avivator * Use epsilon value. * Use better range of theta. * onViewportLoad callbacks. * Remove plane flip * Move onViewportLoad to image settings. * Set up isLoading callbacks. * Fix AddChannel call backs. * Allow options to be disabled as well. * Simplify global selections * AbortController for volume layer * Only do keydown change selection * Add delay to onViewportload prop for volumes * Add slicer presets. Move slicer location. * Fix Camera Axis. * Debounce view state change. * Fix coordinate system for volume. * Change scaling of normal planes. * Linting. * Fix old merge issues * Change coordinates of slicing * Add isLoading for channel. * Add isChannelLoading removal. * Chane when channel is set to loading. * Fix jsdocs. Small fix for volume button. * Don't always use physical size scaling. * Fix loader physical size inference. * Make typescript happy. * Small fix. * Fix properties state bug. * Fix rotation recentering. * Use copy of clipping planes. * Fixes for viv src * Address comments. * Slicing Reversion + New Demo Datasets (#430) * Revert to orthogonal slicing. * Add idr datasets * Fix slicer styling. * Small doc fixes. * Add interpolation to 3D * Add check for interpolation. * prettier. * Final Changes for Avivator (#434) * Use tabs for controller. * WRong disabled cpondition for volume button * Change non-3d rendering mode display. * Small style tweaks. * Fix checkbox. Small styling issues. * Rename * Add disabled styling. * Add spacing. * Fix slicer styling. * Set bounding cube intiially. * Add warning about WebGL2 * Remind to refresh page. * Fix for 3D Blending. (#440) * Minimal Fix for 3D Blending. * Remove culling on layer * Add comment. * Prettier. * Add progress indicator option + onUpdate callback. (#446) Co-authored-by: Trevor Manz <trevor.j.manz@gmail.com>
For #398 this is the next issue on the list for #415. It contains all of the features in the list on #415
Change List
as well as makingImageLayer
capable of cancelling requests when it finalizes its stateUnify all rendering to happen in "physical coordinates" (so this is a resolution of 3D Viv Core #409 (comment)). I can't see a good reason to use the pixel coordinates since we have to scale volumes anyway to make them render properly i.e if you wanted to overlay something, you would have to use physical coordinates anyway, but it's possible I am wrong in this inclination. I opened an issue in image.sc about this: https://forum.image.sc/t/ome-roi-coordinate-system/51781. Previously, downsampled resolutions rendered in a space defined by their literal pixel sizes but now those pixels are scaled up by the layer to what the full volume would be (just like an image pyramid). This was an oversight on my part, but while implementing the plane-clipping/resetting the camera, it became apparent that this would be necessary for consistency of those implementations with an external applicatoin.Checklist