-
Notifications
You must be signed in to change notification settings - Fork 8
Changes to support Simularium integration #255
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
Conversation
…imated/volume-viewer into simularium-volumes
…imated/volume-viewer into simularium-volumes
This reverts commit 479df99.
ShrimpCryptid
left a comment
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.
Looking really good overall, had a couple questions but once they're clarified I'm good to approve 👍
| const { normPhysicalSize, normRegionSize } = this.volume; | ||
| // Set offset | ||
| this.geometryMesh.position.copy(this.volume.getContentCenter()); | ||
| this.geometryMesh.position.copy(this.volume.getContentCenter().multiply(this.settings.scale)); |
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 does a volume's scale affect its offset position? Is the scaling origin always the origin of the scene?
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 offset position is relative to the center of the volume, which for this viewer should almost always be the origin of the scene. It's there to compensate for an irregular subregion. E.g. if the subregion in the x-axis were 0.5 - 1.0, getContentCenter would return 0.25 in the x-axis, because the center of the mesh has to move from the center of the volume bounding box to halfway to its edge:

Since that offset is in world units, it also has to account for the scale of the volume.
…imated/volume-viewer into simularium-volumes
…imated/volume-viewer into simularium-volumes
ShrimpCryptid
left a comment
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.
LGTM!
Review time: medium (20-30min). Most of the diff is package-lock.json!
Misc. changes required to incorporate volume rendering from this codebase into Simularium, including:
ImageInfo.transformto correspond to this, even though fields of that object are never read within the core package.VolumeDrawabletype.doRendermethod ofVolumeRenderImplno longer depends on our internalThreeJsPanelclass, and instead accepts the three.js objects it was previously getting from it.RayMarchedAtlasVolumenow generates its own fallback empty depth texture on its first render, to allow its render method to be called without a depth texture. (Future work should allow this class to accept the full view space position buffer generated by Simularium in its first render pass in place of a depth texture.) Producing this depth texture currently generates errors in Simularium, which is running a slightly older version of three.plugin-transform-runtime, to resolve some conflicts.