docs(volume3d): analysis of the three Volume3D render paths and an improvement plan - #2853
Open
wayfarer3130 wants to merge 1 commit into
Open
docs(volume3d): analysis of the three Volume3D render paths and an improvement plan#2853wayfarer3130 wants to merge 1 commit into
wayfarer3130 wants to merge 1 commit into
Conversation
…provement plan
Adds a documentation set covering the three Volume3D render paths (vtk WebGL,
vtk WebGPU, mview/fuberlin), why they differ in performance and fidelity, what
limits renderable volume size, and a sequenced plan for improving them.
Docs only - no source changes.
The analysis is based on the `webgpuSpike` branch in mbellehumeur/cornerstone3D;
the fuberlin and WebGPU Volume3D render paths do not exist on main yet, so source
links point at that branch. Proposed against main so the analysis and plan can be
discussed independently of the spike landing.
- RENDERING-Volume3D-overview.md how a render mode is selected, shared
plumbing, and the preset-fidelity
differences between the three paths
- RENDERING-vtkVolume3d-webgl.md the gl path
- RENDERING-webgpuVolume3d.md the gpu path
- RENDERING-fuberlinVolume3D-mview.md the mview path
- RENDERING-performance-compare.md why mview > gpu > gl interactively, and
the options for closing each gap
- RENDERING-large-volumes.md texture/memory limits, LOD, bricking,
MPR implications, server-side storage
- RENDERING-PLAN.md phased plan with a gl-vs-mview estimate
Findings worth reviewer attention:
- mview's speed advantage is partly a fidelity defect. It omits vtk's
sample-distance opacity correction, so per-sample alpha is ~1.8x higher and
rays reach the early-out ~2.5x sooner. That is also why its interactive and
still frames differ in density.
- gl never reduces resolution during interaction. Both halves of vtk's
`isAnimating() && _lastScale > 1.5` gate fail: Cornerstone's offscreen window
has no animating interactor, and createVolumeMapper leaves
initialInteractionScale at 1.0, which cannot clear the gate.
- gpu is the only path that does not RAF-coalesce presents, so a drag can issue
several complete frames per displayed frame.
- MPR and 3D share one GPU representation on both backends
(mapperImageDataByVolumeId is keyed by volumeId alone), so a reduced-resolution
volume for 3D would silently degrade MPR too.
- Bricking is a poor first move for DVR but a natural fit for MPR: MPR does no
compositing along a ray, so the ordering constraint does not apply.
The plan is phased 0-5, with a decision engine (0), gl quick wins (1), gl via
vtk.js (2), a surface renderer (3), precomputed hierarchical storage generated
by static-dicomweb (4a) and bricked client rendering (4b), and WebGPU parity (5).
Phases 1-2 give an independently measurable deliverable; the estimate is that gl
lands within 1.3-2.2x of mview after phase 1 and 1.1-1.6x after phase 2, against
a 2x acceptance threshold.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds documentation for Volume3D rendering paths, including vtk.js WebGL, vtk.js WebGPU, and mview. It also documents performance comparisons, large-volume strategies, capability-aware fallback behavior, and a phased rendering improvement plan. ChangesVolume3D Rendering Documentation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
Author
|
I'm creating a PR with documentation about a possible path forward for the volume 3d performance/large volume rendering path. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-large-volumes.md`:
- Around line 844-847: The statement in “Remove the redundant CPU copies”
overclaims the CPU peak from slice-wise upload. Revise it to account for cached
source slices created by DefaultVolume3DDataProvider and state that reaching ~S
requires an explicit ownership, eviction, or release policy after all consumers
finish; otherwise describe only removal of the contiguous staging array without
promising the lower peak.
- Around line 22-40: Revise the projected voxel-size definition and base-level
selection guidance in this document to account for physical voxel spacing and
camera orientation, not just voxels across. Specify that selection must use
physical volume bounds, spacing, projection, orientation, and render-target
scale to calculate direction-dependent screen-space voxel footprints, while
preserving the existing lossless-for-the-display criterion.
- Around line 766-775: Update the sagittal access row in the uncompressed NIfTI
byte-range table to use bytesPerVoxel instead of the hardcoded 2-byte range
size, keeping the existing ny*nz range-count description unchanged.
- Around line 817-835: Update the table’s “Renderable with context?” column to
“3D texture payload fits” and revise the surrounding text to state that
end-to-end renderability remains conditional on runtime GPU memory, CPU cache
capacity, and active diagnostic MPR views, including the approximately 1 GB
context payload and additional CPU/full-resolution residency considerations.
- Around line 16-20: The document’s claim that reduced volumes are lossless at
displayed resolution is too strong for DVR. Revise the affected criterion and
related sections to describe a display-resolution-sufficient approximation,
define an explicit error tolerance, and require validation against
representative rendered images; do not label the approximation lossless for p ≤
1, including interaction targets.
- Around line 79-103: Update the volume-size guidance and table to use the
runtime device.limits.maxTextureDimension3D value instead of treating 2048 as
universal. Ensure the described validation compares width, height, and depth
against that runtime limit, and revise context-volume claims accordingly while
preserving the oversized-volume error behavior.
In
`@packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-performance-compare.md`:
- Around line 217-225: Update the vtk.js integration around
OpenGL/VolumeMapper.renderPieceFinish so reduced interaction renders do not
restore the full framebuffer and upscale through copyShader before
_copyToOnscreenCanvas. Bypass or conditionally skip that internal copy for
reduced rendering, while preserving normal full-resolution behavior; otherwise
revise section 5b to describe only moving the final canvas copy.
- Around line 19-34: Update the “Texture format is identical” section to remove
the claim that mview and vtk.js have identical linear filtering. Distinguish
their shared storage format from filtering behavior: vtk.js uses
textureSampleLevel with a linear clampSampler, while mview uses unfiltered
textureLoad; separately describe fetch cost and image fidelity without implying
equivalent filtering.
In
`@packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-PLAN.md`:
- Line 4: Update the document-count wording in the discussion PR paragraph to
match the six linked analysis documents: describe them as six supporting
documents, or explicitly state seven documents when including this plan.
- Around line 27-29: Update the level-selection rule in the rendering plan to
choose the coarsest (lowest-resolution) level satisfying p ≤ 1, rather than the
finest qualifying level. Preserve the requirement to round up in resolution and
use 1024 instead of 2048 for a 1024-pixel pane when both qualify.
- Around line 131-134: Specify the fenced pseudo-configuration block as text to
satisfy markdownlint MD040. Update Phase 3 to include
DefaultVolume3DDataProvider payload mapping, resolver or registry wiring for the
new surface renderMode, and corresponding tests, while preserving existing
volume and geometry behavior.
In
`@packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-Volume3D-overview.md`:
- Around line 40-43: Replace all machine-local mview source paths with stable
publicly accessible references or explicit private-source notes: update the
upstream reference in
packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-Volume3D-overview.md
lines 40-43, the build-status caveat in the same file lines 171-178, and the
renderer package reference in
packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-fuberlinVolume3D-mview.md
lines 7-9.
- Around line 61-64: Update the “Data provider” section to document only the
modes actually supported by DefaultVolume3DDataProvider, Volume3DRenderMode, and
the default resolver: vtkVolume3d and vtkGeometry3d. Remove the unsupported
webgpuVolume3d and fuberlinVolume3D claim rather than introducing undocumented
contracts.
In
`@packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-vtkVolume3d-webgl.md`:
- Around line 65-74: Update the Preset section to distinguish the zero-centered
shift range used for normalization from the installed RGB and scalar-opacity
control points, which are restored to absolute HU values. Keep the existing
descriptions of gradient opacity, shading, and interpolation unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e052dcd4-a997-45b2-8486-89168b6847d7
📒 Files selected for processing (7)
packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-PLAN.mdpackages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-Volume3D-overview.mdpackages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-fuberlinVolume3D-mview.mdpackages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-large-volumes.mdpackages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-performance-compare.mdpackages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-vtkVolume3d-webgl.mdpackages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-webgpuVolume3d.md
Comment on lines
+16
to
+20
| A display at a given magnification has a fixed sampling density. Any data finer than | ||
| that density is discarded by the display no matter what you stored, so a representation | ||
| that _meets_ that density is _lossless with respect to the displayed image_. The goal | ||
| is not "always full resolution", it is **lossless at the resolution actually being | ||
| displayed, up to the full resolution of the device**. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not call the reduced volume lossless for DVR at p ≤ 1.
A subpixel voxel can still change a pixel after transfer-function classification and ray integration. Box-filtering scalar data before a nonlinear transfer function is not generally equivalent to filtering the rendered result. An interaction target is also upscaled to the display and is therefore not lossless at the final display resolution. Rename this criterion to a display-resolution-sufficient approximation, define an error tolerance, and validate it with representative rendered images.
Also applies to: 62-68, 248-252
🧰 Tools
🪛 LanguageTool
[style] ~18-~18: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ... that meets that density is lossless with respect to the displayed image. The goal is not "...
(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-large-volumes.md`
around lines 16 - 20, The document’s claim that reduced volumes are lossless at
displayed resolution is too strong for DVR. Revise the affected criterion and
related sections to describe a display-resolution-sufficient approximation,
define an explicit error tolerance, and require validation against
representative rendered images; do not label the approximation lossless for p ≤
1, including interaction targets.
Comment on lines
+22
to
+40
| Let `p` be the projected voxel size — pane pixels spanned by the volume, divided by | ||
| voxels across: | ||
|
|
||
| | | meaning | consequence | | ||
| | ------- | ----------------------------------------------- | ------------------------------------------------------------------------------------------ | | ||
| | `p > 1` | magnification — one voxel covers several pixels | the display is _interpolating_; more data would show more. Reduction here is genuine loss. | | ||
| | `p = 1` | 1:1 | the ideal operating point | | ||
| | `p < 1` | minification — several voxels per pixel | the **display** is discarding data. A level with `p = 1` is lossless for this view. | | ||
|
|
||
| **A reduction is free exactly when the volume out-resolves the pane** — which is the | ||
| definition of the large-volume problem this document is about. The justification for | ||
| the scheme and the condition that creates the problem are the same condition: | ||
|
|
||
| | Volume across | Pane px | Full-res `p` | ½-res `p` | verdict | | ||
| | ------------- | ------- | ------------ | --------- | -------------------------------------------- | | ||
| | 512 | 1024 | 2.0 | 4.0 | already magnifying — ½-res is **real loss** | | ||
| | 1024 | 1024 | 1.0 | 2.0 | at 1:1 — ½-res is real loss | | ||
| | 2048 | 1024 | 0.5 | **1.0** | ½-res is **lossless for this display** | | ||
| | 4096 | 1024 | 0.25 | 0.5 | ¼-res is lossless; ½-res is wasted bandwidth | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Base level selection on projected physical voxel size.
p = pane pixels / voxels across ignores voxel spacing and camera orientation. The performance example uses spacing 0.7/0.7/1.0 mm in packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-performance-compare.md:66-68, so voxel footprints are not equal. Oblique views also produce direction-dependent screen-space footprints. Use physical volume bounds, spacing, projection, orientation, and render-target scale when selecting a level.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-large-volumes.md`
around lines 22 - 40, Revise the projected voxel-size definition and base-level
selection guidance in this document to account for physical voxel spacing and
camera orientation, not just voxels across. Specify that selection must use
physical volume bounds, spacing, projection, orientation, and render-target
scale to calculate direction-dependent screen-space voxel footprints, while
preserving the existing lossless-for-the-display criterion.
Comment on lines
+79
to
+103
| ### 1a. `maxTextureDimension3D` — a hard cliff, not a gradual slowdown | ||
|
|
||
| The WebGPU default limit is **2048** per axis. A volume with any axis above that | ||
| cannot be a single 3D texture, regardless of how much memory is free. mview checks | ||
| explicitly and throws: | ||
|
|
||
| ```js | ||
| const limit = this.device.limits.maxTextureDimension3D; | ||
| if (Math.max(width, height, depth) > limit) { | ||
| throw new RangeError( | ||
| `volume ${width}x${height}x${depth} exceeds maxTextureDimension3D ${limit}` | ||
| ); | ||
| } | ||
| ``` | ||
|
|
||
| The vtk.js WebGPU path has no equivalent guard — `getTextureForImageData` passes the | ||
| dimensions straight through, so an oversized volume surfaces as a WebGPU validation | ||
| error rather than a catchable application error. On the WebGL side the equivalent is | ||
| `MAX_3D_TEXTURE_SIZE`, typically 2048 on desktop but as low as **256** on GLES3-class | ||
| integrated parts, which is the most common "renders on my machine, blank on theirs" | ||
| cause. | ||
|
|
||
| A 512×512×2048 whole-body CT sits exactly on this boundary. Anything longer — or any | ||
| 1024²-in-plane study with more than 2048 slices — is simply unrenderable as one | ||
| texture. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target document ---'
sed -n '60,120p' packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-large-volumes.md
printf '%s\n' '--- later referenced lines ---'
sed -n '785,825p' packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-large-volumes.md
printf '%s\n' '--- relevant symbols ---'
rg -n -S \
'maxTextureDimension3D|getTextureForImageData|MAX_3D_TEXTURE_SIZE|WebGPU|webgpuSpike|brick|texture' \
packages/core packages 2>/dev/null | head -n 240
printf '%s\n' '--- dependency declarations ---'
rg -n -S 'vtk\.js|vtk.js|webgpuSpike|`@kitware`|version' package.json packages/*/package.json yarn.lock package-lock.json pnpm-lock.yaml 2>/dev/null | head -n 160Repository: cornerstonejs/cornerstone3D
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- planning text ---'
sed -n '55,125p' packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-PLAN.md
printf '%s\n' '--- context table and surrounding text ---'
sed -n '795,820p' packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-large-volumes.md
printf '%s\n' '--- exact texture creation references ---'
rg -n -S -C 4 'getTextureForImageData|maxTextureDimension3D|MAX_3D_TEXTURE_SIZE' \
packages/core/src packages/core/test \
-g '*.ts' -g '*.js' -g '*.md' | head -n 220
printf '%s\n' '--- repository state and tracked candidate files ---'
git status --short --branch
git ls-files | rg '(^|/)(WebGPUVolume3DRenderPath|FuberlinVolume3DRenderPath|VolumePass|Rendering/WebGPU|RENDERING-large-volumes|RENDERING-PLAN)' | head -n 160Repository: cornerstonejs/cornerstone3D
Length of output: 29569
🌐 Web query:
WebGPU specification maxTextureDimension3D default limit device.limits adapter limits
💡 Result:
In the WebGPU specification, the default value for the maxTextureDimension3D limit is 2048 [1][2][3]. This limit defines the maximum allowed value for the width, height, and depth (or array layers) of a texture created with the "3d" dimension [1][4][3]. Key points regarding WebGPU limits include: - Default Limits: When requesting a GPUDevice without specifying custom limits, the browser provides a set of default, baseline limits that are guaranteed to be supported across compliant implementations [5][6]. The value 2048 for maxTextureDimension3D is part of this baseline [1][3]. - Adapter Limits: The GPUAdapter interface exposes the actual limits supported by the underlying hardware and browser implementation via the limits property [7]. Because browsers may report these limits in "tiers" to mitigate fingerprinting, the value returned by an adapter may differ from the absolute hardware capability [7]. - Requesting Limits: Developers can request higher limits beyond the defaults when calling requestDevice by passing a requiredLimits object, provided those values do not exceed what the adapter supports [6][7].
Citations:
- 1: https://www.w3.org/TR/webgpu/
- 2: https://developer.mozilla.org/en-US/docs/Web/API/GPUSupportedLimits
- 3: https://github.com/gpuweb/gpuweb/blob/5ee37ea391ea5635dcd88a001318bd18cd2d1099/spec/index.bs
- 4: https://www.w3.org/TR/2026/CRD-webgpu-20260310/
- 5: https://www.w3.org/TR/2025/CRD-webgpu-20250131/
- 6: https://webgpufundamentals.org/webgpu/lessons/webgpu-limits-and-features.html
- 7: https://developer.mozilla.org/en-US/docs/Web/API/GPUAdapter/limits
Use the runtime maxTextureDimension3D limit.
2048 is WebGPU’s default guarantee, not a universal device limit. Compare each volume dimension with device.limits.maxTextureDimension3D, and update the context-volume claims and table to use that runtime value.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-large-volumes.md`
around lines 79 - 103, Update the volume-size guidance and table to use the
runtime device.limits.maxTextureDimension3D value instead of treating 2048 as
universal. Ensure the described validation compares width, height, and depth
against that runtime limit, and revise context-volume claims accordingly while
preserving the oversized-volume error behavior.
Source: MCP tools
Comment on lines
+766
to
+775
| Byte-range requests against an **uncompressed** `.nii` do give you something, just | ||
| much less than a pyramid: | ||
|
|
||
| | Access | Cost | | ||
| | ----------------------------- | -------------------------------------------------------------------------------- | | ||
| | One axial (k) slice | **one contiguous range** of `nx*ny*bpv` bytes — efficient | | ||
| | Every other k slice | nz/2 contiguous ranges — a clean half-resolution-in-K load | | ||
| | One coronal (j) plane | nz ranges of `nx*bpv` each — borderline; most servers cap multipart range counts | | ||
| | One sagittal (i) plane | ny\*nz ranges of 2 bytes each — impractical | | ||
| | Any reduced-resolution volume | **not possible** without reading everything | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use bytesPerVoxel for sagittal byte ranges.
The NIfTI formula uses bytesPerVoxel, but the sagittal row says each range is 2 bytes. The range size depends on datatype; use bytesPerVoxel consistently.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-large-volumes.md`
around lines 766 - 775, Update the sagittal access row in the uncompressed NIfTI
byte-range table to use bytesPerVoxel instead of the hardcoded 2-byte range
size, keeping the existing ny*nz range-count description unchanged.
Comment on lines
+817
to
+835
| Applied to the reference sizes, with a ½-per-axis context volume: | ||
|
|
||
| | Volume | Full-res GPU | Renderable today? | Context volume | Renderable with context? | | ||
| | -------------- | ------------ | ---------------------------------------- | ----------------------- | ------------------------ | | ||
| | 512×512×400 | 210 MB | yes | 26 MB | yes | | ||
| | 512×512×2000 | 1.05 GB | borderline on integrated | 131 MB | yes | | ||
| | 1024×1024×2000 | 4.2 GB | no | 524 MB | yes | | ||
| | 1024×1024×4096 | 8.6 GB | no — exceeds `maxTextureDimension3D` too | 1.05 GB (2048 max axis) | yes | | ||
|
|
||
| Only at the point where even the context volume will not fit — a second reduction | ||
| level, or genuinely out-of-core data — does bricking with a page table become the | ||
| necessary answer rather than an expensive one. | ||
|
|
||
| **This table describes the 3D pane in isolation.** Per §5b, if a diagnostic MPR is on | ||
| screen it holds the full-resolution representation resident, so the "renderable with | ||
| context?" column describes whether the _3D view_ can be drawn, not the total memory | ||
| footprint of the layout. For layouts that include reformats, the context volume is an | ||
| addition (1.125×) rather than a replacement, and the memory ceiling is set by MPR's | ||
| requirements — which is the case where options 2 and 3, applied to MPR residency, do |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not label the context rows simply “renderable.”
The 1024×1024×4096 context still requires about 1 GB of r16float GPU storage. The document also estimates ~2S/~3S CPU materialization, and diagnostic MPR can keep the full-resolution representation resident. Rename this column to “3D texture payload fits” and make end-to-end renderability conditional on runtime GPU memory, CPU cache capacity, and active MPR views.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-large-volumes.md`
around lines 817 - 835, Update the table’s “Renderable with context?” column to
“3D texture payload fits” and revise the surrounding text to state that
end-to-end renderability remains conditional on runtime GPU memory, CPU cache
capacity, and active diagnostic MPR views, including the approximately 1 GB
context payload and additional CPU/full-resolution residency considerations.
Comment on lines
+27
to
+29
| 1. Select the finest level with `p ≤ 1` — round up in resolution, never down. | ||
| 2. Reductions must be band-limited. Decimation aliases, which fabricates structure | ||
| rather than merely losing it. |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Select the coarsest qualifying level.
The current rule selects the finest level with p ≤ 1. For 512, 1024, and 2048 voxel levels in a 1024-pixel pane, this selects 2048 even though 1024 is already lossless for the displayed resolution. Choose the lowest-resolution level that satisfies p ≤ 1; otherwise the policy defeats the memory and large-volume goals.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-PLAN.md`
around lines 27 - 29, Update the level-selection rule in the rendering plan to
choose the coarsest (lowest-resolution) level satisfying p ≤ 1, rather than the
finest qualifying level. Preserve the requirement to round up in resolution and
use 1024 instead of 2048 for a 1024-pixel pane when both qualify.
Comment on lines
+131
to
+134
| ``` | ||
| backend: webgpuVolume3d → vtkVolume3d → (3D: refuse) / (MPR: cpu) | ||
| data: full resolution → reduced level → slab or bricks → refuse | ||
| ``` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Specify the fenced block language.
markdownlint-cli2 reports MD040 for this block. Use text for the pseudo-configuration.
Proposed fix
-```
+```text
backend: webgpuVolume3d → vtkVolume3d → (3D: refuse) / (MPR: cpu)
data: full resolution → reduced level → slab or bricks → refuse</details>
</review_comment>
<review_comment line_ranges="328-333">
**Include data-provider and resolver wiring in Phase 3.**
The current `DefaultVolume3DDataProvider.load` contract accepts only `vtkVolume3d` and `vtkGeometry3d`. Its non-volume branch loads geometry and returns a geometry payload. A new surface `renderMode` therefore needs explicit provider payload mapping, resolver or registry wiring, and tests. List this Cornerstone work in Phase 3, even if the renderer itself does not require vtk.js changes.
</review_comment>
</file_review>
<consolidated_comments>
none
</consolidated_comments>
</review_response>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 131-131: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-PLAN.md`
around lines 131 - 134, Specify the fenced pseudo-configuration block as text to
satisfy markdownlint MD040. Update Phase 3 to include
DefaultVolume3DDataProvider payload mapping, resolver or registry wiring for the
new surface renderMode, and corresponding tests, while preserving existing
volume and geometry behavior.
Source: Linters/SAST tools
Comment on lines
+40
to
+43
| The upstream, OHIF-free version of the mview renderer documents itself in | ||
| `z:/src/gpu-viewer-3d/mview-webgpu-volume-core/README.md`. These files are the | ||
| OHIF/Cornerstone-tree counterpart: what each path actually does _inside_ a | ||
| `VolumeViewport3D`, including the plumbing the standalone renderer has no notion of. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace the machine-local mview source references.
The committed documents contain a developer-local path that other readers cannot access.
packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-Volume3D-overview.md#L40-L43: replace the local upstream reference with a stable reference or an explicit private-source note.packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-Volume3D-overview.md#L171-L178: replace the local package-source path in the build-status caveat.packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-fuberlinVolume3D-mview.md#L7-L9: replace the renderer package source path.
📍 Affects 2 files
packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-Volume3D-overview.md#L40-L43(this comment)packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-Volume3D-overview.md#L171-L178packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-fuberlinVolume3D-mview.md#L7-L9
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-Volume3D-overview.md`
around lines 40 - 43, Replace all machine-local mview source paths with stable
publicly accessible references or explicit private-source notes: update the
upstream reference in
packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-Volume3D-overview.md
lines 40-43, the build-status caveat in the same file lines 171-178, and the
renderer package reference in
packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-fuberlinVolume3D-mview.md
lines 7-9.
Comment on lines
+61
to
+64
| **Data provider.** `DefaultVolume3DDataProvider` is used for all three: it resolves | ||
| a volumeId, `createAndCacheVolume(...)`, and kicks `imageVolume.load()` _before_ the | ||
| render path's `addData` runs. Paths that need the callback have to hook the in-flight | ||
| `loadStatus.callbacks` list (see the mview path). |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
"renderMode|webgpuVolume3d|fuberlinVolume3D|DefaultVolume3DDataProvider" \
packages/core/src/RenderingEngine/GenericViewport/Volume3D \
packages/core/srcRepository: cornerstonejs/cornerstone3D
Length of output: 50386
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- candidate files ---'
git ls-files packages/core/src/RenderingEngine/GenericViewport/Volume3D \
| rg 'DataProvider|RenderPath|Registry|Types|overview|fuberlin|webgpu'
echo '--- provider declarations and load calls ---'
rg -n -C 12 \
'class DefaultVolume3DDataProvider|DefaultVolume3DDataProvider|load\(' \
packages/core/src/RenderingEngine/GenericViewport/Volume3D \
-g '*.ts' -g '*.tsx' -g '*.md' | head -n 500
echo '--- target render modes ---'
rg -n -C 12 \
"webgpuVolume3d|fuberlinVolume3D|vtkVolume3d|vtkGeometry3d" \
packages/core/src/RenderingEngine/GenericViewport/Volume3D \
-g '*.ts' -g '*.tsx' -g '*.md' | head -n 800Repository: cornerstonejs/cornerstone3D
Length of output: 50386
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- DefaultVolume3DDataProvider.ts ---'
cat -n packages/core/src/RenderingEngine/GenericViewport/Volume3D/DefaultVolume3DDataProvider.ts
echo '--- Volume3DDataProvider and payload types ---'
sed -n '1,180p' packages/core/src/RenderingEngine/GenericViewport/Volume3D/viewport3DTypes.ts
echo '--- Volume3DRenderPathResolver.ts ---'
cat -n packages/core/src/RenderingEngine/GenericViewport/Volume3D/Volume3DRenderPathResolver.ts
echo '--- exact mode references in tracked files ---'
git grep -n -E 'webgpuVolume3d|fuberlinVolume3D' -- \
':!packages/core/src/RenderingEngine/GenericViewport/Volume3D/*.md' \
':!*.map' || true
echo '--- provider load call and path selection ---'
rg -n -C 18 \
'dataProvider.*load|\.load\(dataId|renderPathResolver|matches\(' \
packages/core/src/RenderingEngine/GenericViewport/Volume3D/viewport3D.ts \
packages/core/src/RenderingEngine/GenericViewport/Volume3D/Volume3DRenderPathResolver.ts \
packages/core/src/RenderingEngine/GenericViewport/Volume3D/*.ts \
-g '*.ts' | head -n 600Repository: cornerstonejs/cornerstone3D
Length of output: 29458
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
root = Path("packages/core/src/RenderingEngine/GenericViewport/Volume3D")
provider = (root / "DefaultVolume3DDataProvider.ts").read_text()
types = (root / "viewport3DTypes.ts").read_text()
resolver = (root / "Volume3DRenderPathResolver.ts").read_text()
overview = (root / "RENDERING-Volume3D-overview.md").read_text()
provider_modes = re.search(
r"renderMode:\s*'([^']+)'\s*\|\s*'([^']+)'", provider
).groups()
type_modes = re.search(
r"export type Volume3DRenderMode =\s*'([^']+)'\s*\|\s*'([^']+)'", types
).groups()
registered_modes = re.findall(
r"renderMode === '([^']+)'|renderMode:\s*'([^']+)'", provider
)
registered_paths = re.findall(
r"new (Vtk\w+Path)\(\)", resolver
)
assert set(provider_modes) == {"vtkVolume3d", "vtkGeometry3d"}
assert set(type_modes) == {"vtkVolume3d", "vtkGeometry3d"}
assert {"VtkVolume3DPath", "VtkGeometry3DPath"} == set(registered_paths)
assert "webgpuVolume3d" not in provider
assert "fuberlinVolume3D" not in provider
assert "webgpuVolume3d" in overview and "fuberlinVolume3D" in overview
print("provider modes:", provider_modes)
print("type modes:", type_modes)
print("registered paths:", registered_paths)
print("source implementations for documented extra modes: none")
print("overview documents both extra modes: yes")
PYRepository: cornerstonejs/cornerstone3D
Length of output: 415
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
root = Path("packages/core/src/RenderingEngine/GenericViewport/Volume3D")
provider = (root / "DefaultVolume3DDataProvider.ts").read_text()
types = (root / "viewport3DTypes.ts").read_text()
resolver = (root / "Volume3DRenderPathResolver.ts").read_text()
overview = (root / "RENDERING-Volume3D-overview.md").read_text()
provider_modes = re.search(
r"renderMode:\s*'([^']+)'\s*\|\s*'([^']+)'", provider
).groups()
type_modes = re.search(
r"export type Volume3DRenderMode =\s*'([^']+)'\s*\|\s*'([^']+)'", types
).groups()
registered_paths = re.findall(
r"new (Vtk\w+Path)\(\)", resolver
)
assert set(provider_modes) == {"vtkVolume3d", "vtkGeometry3d"}
assert set(type_modes) == {"vtkVolume3d", "vtkGeometry3d"}
assert {"VtkVolume3DPath", "VtkGeometry3DPath"} == set(registered_paths)
assert "webgpuVolume3d" not in provider
assert "fuberlinVolume3D" not in provider
assert "webgpuVolume3d" in overview and "fuberlinVolume3D" in overview
print("provider modes:", provider_modes)
print("type modes:", type_modes)
print("registered paths:", registered_paths)
print("source implementations for documented extra modes: none")
print("overview documents both extra modes: yes")
PYRepository: cornerstonejs/cornerstone3D
Length of output: 415
Align the documented provider contract with the source.
DefaultVolume3DDataProvider, Volume3DRenderMode, and the default resolver support only vtkVolume3d and vtkGeometry3d. This tree has no webgpuVolume3d or fuberlinVolume3D implementation. Remove the claim that the provider supports all three modes, or add the corresponding contracts before documenting them.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-Volume3D-overview.md`
around lines 61 - 64, Update the “Data provider” section to document only the
modes actually supported by DefaultVolume3DDataProvider, Volume3DRenderMode, and
the default resolver: vtkVolume3d and vtkGeometry3d. Remove the unsupported
webgpuVolume3d and fuberlinVolume3D claim rather than introducing undocumented
contracts.
Comment on lines
+65
to
+74
| 2. **Preset** (`csUtils.applyPreset(actor, preset)`, driven by OHIF's hanging | ||
| protocol through `NextViewportBackend`). This is what makes a 3D volume actually | ||
| look like a 3D volume, and it sets, from the `VIEWPORT_PRESETS` entry: | ||
| - RGB transfer function and scalar-opacity piecewise function, both remapped into | ||
| a _shift range_ centred on zero rather than into the volume's real HU range; | ||
| - **gradient opacity** — `setUseGradientOpacity(0, true)` plus min/max | ||
| value/opacity; | ||
| - `setShade(preset.shade === '1')` and `ambient` / `diffuse` / `specular` / | ||
| `specularPower`; | ||
| - `setInterpolationTypeToFastLinear()` when `preset.interpolation === '1'`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Correct the transfer-function range description.
This paragraph says that csUtils.applyPreset installs RGB and opacity functions in a zero-centered shift range. RENDERING-Volume3D-overview.md states that the shift range is used for normalization and the installed control points are restored to absolute HU. The current wording changes the documented transfer-function domain.
Proposed wording
- a _shift range_ centred on zero rather than into the volume's real HU range;
+ an internal _shift range_ and then restored to the volume's absolute HU range;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 2. **Preset** (`csUtils.applyPreset(actor, preset)`, driven by OHIF's hanging | |
| protocol through `NextViewportBackend`). This is what makes a 3D volume actually | |
| look like a 3D volume, and it sets, from the `VIEWPORT_PRESETS` entry: | |
| - RGB transfer function and scalar-opacity piecewise function, both remapped into | |
| a _shift range_ centred on zero rather than into the volume's real HU range; | |
| - **gradient opacity** — `setUseGradientOpacity(0, true)` plus min/max | |
| value/opacity; | |
| - `setShade(preset.shade === '1')` and `ambient` / `diffuse` / `specular` / | |
| `specularPower`; | |
| - `setInterpolationTypeToFastLinear()` when `preset.interpolation === '1'`. | |
| 2. **Preset** (`csUtils.applyPreset(actor, preset)`, driven by OHIF's hanging | |
| protocol through `NextViewportBackend`). This is what makes a 3D volume actually | |
| look like a 3D volume, and it sets, from the `VIEWPORT_PRESETS` entry: | |
| - RGB transfer function and scalar-opacity piecewise function, both remapped into | |
| an internal _shift range_ and then restored to the volume's absolute HU range; | |
| - **gradient opacity** — `setUseGradientOpacity(0, true)` plus min/max | |
| value/opacity; | |
| - `setShade(preset.shade === '1')` and `ambient` / `diffuse` / `specular` / | |
| `specularPower`; | |
| - `setInterpolationTypeToFastLinear()` when `preset.interpolation === '1'`. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/core/src/RenderingEngine/GenericViewport/Volume3D/RENDERING-vtkVolume3d-webgl.md`
around lines 65 - 74, Update the Preset section to distinguish the zero-centered
shift range used for normalization from the installed RGB and scalar-opacity
control points, which are restored to absolute HU values. Keep the existing
descriptions of gradient opacity, shading, and interpolation unchanged.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Adds a documentation set covering the three Volume3D render paths (vtk WebGL, vtk WebGPU, mview/fuberlin), why they differ in performance and fidelity, what limits renderable volume size, and a sequenced plan for improving them.
Docs only - no source changes.
The analysis is based on the
webgpuSpikebranch in mbellehumeur/cornerstone3D; the fuberlin and WebGPU Volume3D render paths do not exist on main yet, so source links point at that branch. Proposed against main so the analysis and plan can be discussed independently of the spike landing.plumbing, and the preset-fidelity
differences between the three paths
the options for closing each gap
MPR implications, server-side storage
Findings worth reviewer attention:
isAnimating() && _lastScale > 1.5gate fail: Cornerstone's offscreen window has no animating interactor, and createVolumeMapper leaves initialInteractionScale at 1.0, which cannot clear the gate.The plan is phased 0-5, with a decision engine (0), gl quick wins (1), gl via vtk.js (2), a surface renderer (3), precomputed hierarchical storage generated by static-dicomweb (4a) and bricked client rendering (4b), and WebGPU parity (5). Phases 1-2 give an independently measurable deliverable; the estimate is that gl lands within 1.3-2.2x of mview after phase 1 and 1.1-1.6x after phase 2, against a 2x acceptance threshold.
Context
Changes & Results
Testing
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit