Volume agent rendering #419
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
toloudis
reviewed
Oct 11, 2024
| export default class VolumeModel { | ||
| // TODO what to do with this `cancelled` property? Type check fails without it. | ||
| // When should it be set, if ever; what should it be used for, if anything? | ||
| public cancelled = false; | ||
| private image?: VolumeDrawable; | ||
| private drawable?: VolumeDrawable; |
There was a problem hiding this comment.
If I recall, I think the "cancelled" property mentioned above is for when the trajectory has been changed but there is still geometry download requests in-flight. We can use cancelled to just drop the arriving stuff.
…ium/simularium-viewer into feature/volume-rendering
frasercl
requested review from
meganrm and
tyler-foster
and removed request for
a team
ShrimpCryptid
approved these changes
Jan 7, 2025
| const volume = await loader.createVolume(new LoadSpec()); | ||
| const context = await this.getVolumeLoaderContext(); | ||
| const loader = await context.createLoader(url); | ||
| const loadCallback = model.onChannelLoaded.bind(model); |
There was a problem hiding this comment.
Why do you need to bind the callback here? :0
Comment on lines
-7
to
-12
| // TEMPORARY HACK to make things typecheck. TODO remove! | ||
| import { VolumeRenderImpl } from "@aics/volume-viewer/es/types/VolumeRenderImpl"; | ||
| interface TempRayMarchedVolume extends VolumeRenderImpl { | ||
| boxHelper: Box3Helper; | ||
| } | ||
|
|
Comment on lines
+93
to
+94
| const min = histo.findBinOfPercentile(0.5); | ||
| const max = histo.findBinOfPercentile(0.983); |
There was a problem hiding this comment.
I saw the note you had in the PR description, maybe tag all the hardcoded values with a TODO so they're easier to track down? Totally optional
| if (isOrtho) { | ||
| this.drawable.setOrthoScale(orthoScale); | ||
| } | ||
| this.drawable.setResolution(width, height); |
There was a problem hiding this comment.
What does setting the resolution here do?
| @@ -161,7 +166,7 @@ class VisGeometry { | |||
| public lightsGroup: Group; | |||
| public agentPathGroup: Group; | |||
| public instancedMeshGroup: Group; | |||
| public tempVolumeGroup: Group; // TODO remove | |||
| public volumeGroup: Group; // TODO remove | |||
There was a problem hiding this comment.
Is the TODO still needed?
interim17
reviewed
Jan 8, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Review size: medium (20-25min)
Resolves #406: makes volume agents render properly!
Adds some new semantics for volume agent data: if the agent has subpoints, subpoint 0 represents the time point of the volume to load and display, and the rest are indexes of channels to enable. By default, if no subpoints are included, timepoint 0 is loaded and only channel 0 is enabled.
Note that this change is accompanied by changes on the companion branch in volume-viewer,
simularium-volumes. As with the previous PR in this series (#417), this branch doesn't work without the changes on that branch. (allen-cell-animated/vole-core#255 will merge those changes into the viewer proper, after which we'll be able to use a plain old npm dependency and get tests to pass here.)Note also that I've hardcoded values for more than a few volume render settings. We may still have more semantics to figure out around whether to pick sensible defaults for these settings, or whether to add some new settings to the subpoints schema described above.
Screenshot