feat(webgpu): experimental WebGPU render backend for GenericViewport stacks and MPR - #2796
feat(webgpu): experimental WebGPU render backend for GenericViewport stacks and MPR#2796sedghi wants to merge 11 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds WebGPU planar image-stack and volume-slice rendering with shared viewport windows, backend registration, registry-aware routing, segmentation integration, interactive examples, diagnostics, and Generic Viewports catalog entries. ChangesWebGPU planar rendering
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Example
participant RenderingEngine
participant WebGPURenderPath
participant WebGPUViewportWindow
participant Canvas
Example->>RenderingEngine: register and select WebGPU backend
RenderingEngine->>WebGPURenderPath: create image or volume path
WebGPURenderPath->>WebGPUViewportWindow: acquire window and render
WebGPUViewportWindow->>Canvas: blit completed WebGPU output
WebGPURenderPath->>RenderingEngine: emit image-rendered state
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
Full details: Description checkExplanation The description includes all required sections: context, changes and results, testing, checklist, and tested environment. It provides implementation details, dependencies, verification results, and test instructions. The documentation checkbox includes an explicit explanation for the experimental API status.
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarResolvedView.ts (1)
426-432: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
currentImageto the shared image-rendering shape
PlanarRenderingis a closed union, but this branch also handles extension image modes likewebgpuImage, and those states already carrycurrentImage. Move that field onto the shared image-rendering type instead of casting here so the branch stays type-safe.🤖 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/Planar/PlanarResolvedView.ts` around lines 426 - 432, Update the shared image-rendering type used by the closed PlanarRendering union to include the optional currentImage field, then simplify the image selection in the isImageRenderMode branch of PlanarResolvedView by accessing rendering.currentImage directly and removing the local cast. Ensure all image-mode variants, including extension modes, satisfy the updated type.packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewReferenceController.ts (1)
618-621: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider using
isVolumeRenderModefor consistency with the registry-aware approach.
resolveVolumeReferenceImageIdIndexstill checksrendering.renderMode !== ActorRenderMode.CPU_VOLUME && rendering.renderMode !== ActorRenderMode.VTK_VOLUME_SLICEexplicitly. While correct for current core volume modes, this won't recognize future registered volume backends (e.g., a hypothetical WebGPU volume mode).getViewportLabelmapRenderMode.tsin this same PR already usesisVolumeRenderModefor this purpose — aligning here would keep the pattern consistent.This is deferred-friendly since the WebGPU backend is image-only in this PR.
♻️ Optional refactor for consistency
if ( - rendering.renderMode !== ActorRenderMode.CPU_VOLUME && - rendering.renderMode !== ActorRenderMode.VTK_VOLUME_SLICE + !isVolumeRenderMode(rendering.renderMode) ) { return; }🤖 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/Planar/PlanarViewReferenceController.ts` around lines 618 - 621, Replace the explicit CPU_VOLUME and VTK_VOLUME_SLICE checks in resolveVolumeReferenceImageIdIndex with the registry-aware isVolumeRenderMode helper, matching getViewportLabelmapRenderMode.ts. Preserve the existing non-volume control flow while allowing future registered volume render modes.
🤖 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/Planar/WebGPUImageMapperRenderPath.ts`:
- Around line 124-168: Ensure addData() releases the acquired WebGPU window when
setup fails. Wrap the setup from acquireWebGPUViewportWindow through
actor/rendering initialization in try/catch, call
releaseWebGPUViewportWindow(ctx.viewportId) in the catch before rethrowing, and
preserve normal ownership for successful setup and removeData().
---
Nitpick comments:
In
`@packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarResolvedView.ts`:
- Around line 426-432: Update the shared image-rendering type used by the closed
PlanarRendering union to include the optional currentImage field, then simplify
the image selection in the isImageRenderMode branch of PlanarResolvedView by
accessing rendering.currentImage directly and removing the local cast. Ensure
all image-mode variants, including extension modes, satisfy the updated type.
In
`@packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewReferenceController.ts`:
- Around line 618-621: Replace the explicit CPU_VOLUME and VTK_VOLUME_SLICE
checks in resolveVolumeReferenceImageIdIndex with the registry-aware
isVolumeRenderMode helper, matching getViewportLabelmapRenderMode.ts. Preserve
the existing non-volume control flow while allowing future registered volume
render modes.
🪄 Autofix (Beta)
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: 95647821-8d48-4f8a-a177-32ac42f64f66
📒 Files selected for processing (14)
packages/core/examples/genericWebGPUStack/index.tspackages/core/src/RenderingEngine/GenericViewport/Planar/PlanarResolvedView.tspackages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewReferenceController.tspackages/core/src/RenderingEngine/GenericViewport/Planar/WebGPUImageMapperRenderPath.tspackages/core/src/RenderingEngine/GenericViewport/Planar/planarRenderPathProjection.tspackages/core/src/RenderingEngine/GenericViewport/Planar/planarViewReference.tspackages/core/src/RenderingEngine/GenericViewport/Planar/vtkWebGPUModules.d.tspackages/core/src/RenderingEngine/GenericViewport/Planar/webgpuViewportRenderWindow.tspackages/core/src/RenderingEngine/helpers/registerWebGPURenderBackend.tspackages/core/src/index.tspackages/core/src/types/RenderBackendRegistry.tspackages/tools/src/stateManagement/segmentation/helpers/getViewportLabelmapRenderMode.tspackages/tools/src/tools/displayTools/Labelmap/labelmapActorStyle.tsutils/ExampleRunner/example-info.json
| const window = acquireWebGPUViewportWindow(ctx.viewportId); | ||
| this.window = window; | ||
|
|
||
| // 16-bit handling: the vtk.js WebGPU texture manager uploads non-8-bit | ||
| // scalar types (Int16/Uint16/Float) as r16float (half-float). There is no | ||
| // norm16 equivalent in WebGPU, so precision matches the WebGL | ||
| // no-EXT_texture_norm16 fallback path. | ||
| const mapper = vtkImageMapper.newInstance(); | ||
| const actor = vtkImageSlice.newInstance(); | ||
| const imageData = | ||
| payload.imageData ?? createVTKImageDataFromImage(payload.image); | ||
|
|
||
| ctx.display.activateRenderMode(WEBGPU_IMAGE_RENDER_MODE); | ||
| mapper.setInputData(imageData); | ||
| actor.setMapper(mapper); | ||
| // Multi-component images (e.g. RGB ultrasound) must render as direct | ||
| // color; see VtkImageMapperRenderPath. | ||
| if (imageData.getPointData().getScalars().getNumberOfComponents() > 1) { | ||
| actor.getProperty().setIndependentComponents(false); | ||
| } | ||
| window.renderer.addActor(actor); | ||
|
|
||
| const rendering: PlanarWebGPUImageRendering = { | ||
| renderMode: WEBGPU_IMAGE_RENDER_MODE, | ||
| actorEntryUID: uuidv4(), | ||
| actor, | ||
| currentImage: payload.image, | ||
| mapper, | ||
| imageData, | ||
| useWorldCoordinateImageData: payload.useWorldCoordinateImageData, | ||
| currentImageIdIndex: payload.initialImageIdIndex ?? 0, | ||
| defaultVOIRange: getDefaultImageVOIRange(payload.image), | ||
| dataPresentation: undefined, | ||
| loadRequestId: 0, | ||
| }; | ||
|
|
||
| // Only the source binding drives the slice scrollbar; see | ||
| // VtkImageMapperRenderPath. | ||
| if (options.role !== 'overlay') { | ||
| triggerPlanarNewImage(ctx, { | ||
| image: payload.image, | ||
| imageIdIndex: payload.initialImageIdIndex ?? 0, | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- outline target file ---'
ast-grep outline packages/core/src/RenderingEngine/GenericViewport/Planar/WebGPUImageMapperRenderPath.ts --view expanded || true
echo '--- relevant slice ---'
sed -n '1,260p' packages/core/src/RenderingEngine/GenericViewport/Planar/WebGPUImageMapperRenderPath.ts
echo '--- search acquire/release helpers ---'
rg -n "acquireWebGPUViewportWindow|releaseWebGPUViewportWindow" packages/core/src -S
echo '--- search for try/catch or cleanup in webgpu image mapper path ---'
rg -n "removeData|releaseWebGPUViewportWindow|acquireWebGPUViewportWindow|WebGPUImageMapperRenderPath" packages/core/src/RenderingEngine/GenericViewport/Planar -SRepository: cornerstonejs/cornerstone3D
Length of output: 15614
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- webgpuViewportRenderWindow.ts ---'
sed -n '1,220p' packages/core/src/RenderingEngine/GenericViewport/Planar/webgpuViewportRenderWindow.ts
echo '--- WebGPUImageMapperRenderPath removeData slice ---'
sed -n '410,460p' packages/core/src/RenderingEngine/GenericViewport/Planar/WebGPUImageMapperRenderPath.ts
echo '--- inspect createVTKImageDataFromImage references for throw-prone code ---'
rg -n "function createVTKImageDataFromImage|createVTKImageDataFromImage\\(" packages/core/src/RenderingEngine -SRepository: cornerstonejs/cornerstone3D
Length of output: 9349
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- addData call sites in rendering engine ---'
rg -n "\.addData\(" packages/core/src/RenderingEngine -S
echo '--- RenderPathAttachment / mount flow ---'
sed -n '1,260p' packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarMountedData.ts
echo '--- viewport / render path orchestration around binding add ---'
sed -n '1,260p' packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewport.ts
echo '--- search for try/catch around addData consumers ---'
rg -n "try \\{|catch \\(|finally \\{|addData\\(" packages/core/src/RenderingEngine/GenericViewport -SRepository: cornerstonejs/cornerstone3D
Length of output: 18883
Release the WebGPU window on setup failure.
addData() acquires the per-viewport WebGPU window before scene setup completes. If createVTKImageDataFromImage, scalar access, or actor wiring throws, removeData() is never returned, so the refcounted window is left alive. Wrap this block in try/catch and call releaseWebGPUViewportWindow(ctx.viewportId) on error.
🤖 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/Planar/WebGPUImageMapperRenderPath.ts`
around lines 124 - 168, Ensure addData() releases the acquired WebGPU window
when setup fails. Wrap the setup from acquireWebGPUViewportWindow through
actor/rendering initialization in try/catch, call
releaseWebGPUViewportWindow(ctx.viewportId) in the catch before rethrowing, and
preserve normal ownership for successful setup and removeData().
|
Nice 👍🏼 |
sedghi
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/RenderingEngine/GenericViewport/Planar/planarViewReference.ts (1)
265-286: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
isVolumeRenderModehere sowebgpuVolumegets a volume view-reference ID
getPlanarViewReferenceIdstill hard-codesActorRenderMode.CPU_VOLUME/VTK_VOLUME_SLICE.webgpuVolumeis a volume mode elsewhere in this path, so it falls through to theimageId:fallback instead ofgetVolumeViewReferenceId(...).🐛 Proposed fix
- if ( - rendering.renderMode === ActorRenderMode.CPU_VOLUME || - rendering.renderMode === ActorRenderMode.VTK_VOLUME_SLICE - ) { + if (isVolumeRenderMode(rendering.renderMode)) {🤖 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/Planar/planarViewReference.ts` around lines 265 - 286, Update getPlanarViewReferenceId to use the existing isVolumeRenderMode helper instead of checking only ActorRenderMode.CPU_VOLUME and ActorRenderMode.VTK_VOLUME_SLICE, ensuring webgpuVolume and all supported volume modes enter the getVolumeViewReferenceId path.
🧹 Nitpick comments (1)
packages/core/examples/genericWebGPUOrtho/index.ts (1)
335-347: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider adding error handling for the discarded scroll promise.
void viewport.scroll(delta).then(...)silently swallows rejections. In an example this is low-impact, but adding a.catch()would surface errors during debugging.♻️ Optional improvement
- void viewport.scroll(delta).then(() => viewport.render()); + viewport.scroll(delta).then(() => viewport.render()).catch(console.error);🤖 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/examples/genericWebGPUOrtho/index.ts` around lines 335 - 347, Add rejection handling to the promise chain in scrollViewports so failures from viewport.scroll(delta) or viewport.render() are surfaced during debugging; append a catch handler that logs the error with clear context while preserving the existing scroll-then-render behavior.
🤖 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.
Outside diff comments:
In
`@packages/core/src/RenderingEngine/GenericViewport/Planar/planarViewReference.ts`:
- Around line 265-286: Update getPlanarViewReferenceId to use the existing
isVolumeRenderMode helper instead of checking only ActorRenderMode.CPU_VOLUME
and ActorRenderMode.VTK_VOLUME_SLICE, ensuring webgpuVolume and all supported
volume modes enter the getVolumeViewReferenceId path.
---
Nitpick comments:
In `@packages/core/examples/genericWebGPUOrtho/index.ts`:
- Around line 335-347: Add rejection handling to the promise chain in
scrollViewports so failures from viewport.scroll(delta) or viewport.render() are
surfaced during debugging; append a catch handler that logs the error with clear
context while preserving the existing scroll-then-render behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5565ce39-5251-4e74-ac70-4bfb54f44b5f
📒 Files selected for processing (19)
packages/core/examples/genericWebGPUOrtho/index.tspackages/core/src/RenderingEngine/GenericViewport/Planar/DefaultPlanarDataProvider.tspackages/core/src/RenderingEngine/GenericViewport/Planar/PlanarLegacyCompatibilityController.tspackages/core/src/RenderingEngine/GenericViewport/Planar/PlanarResolvedView.tspackages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewReferenceController.tspackages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewport.tspackages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewportLegacyAdapter.tspackages/core/src/RenderingEngine/GenericViewport/Planar/WebGPUImageMapperRenderPath.tspackages/core/src/RenderingEngine/GenericViewport/Planar/WebGPUVolumeSliceRenderPath.tspackages/core/src/RenderingEngine/GenericViewport/Planar/planarViewReference.tspackages/core/src/RenderingEngine/GenericViewport/Planar/webgpuViewportRenderWindow.tspackages/core/src/RenderingEngine/helpers/registerWebGPURenderBackend.tspackages/core/src/index.tspackages/tools/src/stateManagement/segmentation/helpers/labelmapImageMapperSupport.tspackages/tools/src/tools/displayTools/Labelmap/labelmapRenderPlan/planarGenericVolumeLabelmap.tspackages/tools/src/tools/displayTools/Labelmap/labelmapRenderPlan/volumeSliceImageMapperPlan.tspackages/tools/src/tools/displayTools/Labelmap/volumeLabelmapSliceData.tspackages/tools/src/tools/segmentation/strategies/compositions/setValue.tsutils/ExampleRunner/example-info.json
✅ Files skipped from review due to trivial changes (1)
- utils/ExampleRunner/example-info.json
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/index.ts
- packages/core/src/RenderingEngine/GenericViewport/Planar/WebGPUImageMapperRenderPath.ts
- packages/core/src/RenderingEngine/GenericViewport/Planar/webgpuViewportRenderWindow.ts
sedghi
changed the title
…image stacks Adds an opt-in 'webgpu' render backend for planar GenericViewports, rendering image stacks through the vtk.js WebGPU view API via a self-rendering render path with a per-viewport WebGPU render window blitted into the cpu surface canvas. Shared planar code and the labelmap display now resolve render-mode kind/surface through the render backend registry instead of hardcoded ActorRenderMode literals, so extension render modes participate in view resolution, view references, slice scrolling and labelmap styling. isImageRenderMode/isVolumeRenderMode are exported from core. Includes the genericWebGPUStack example: live backend switching (webgpu/gpu/cpu), manipulation and annotation tools, segmentation editing tools on a fake labelmap, and a debug panel reporting the effective backend and WebGPU adapter.
Lists all GenericViewport (next architecture) examples, including the new genericWebGPUStack WebGPU render backend example.
Adds the 'webgpuVolume' render mode: a volume-slice render path using the stock vtkImageResliceMapper through the per-viewport WebGPU window, with a shared reference-counted mapper-input imageData per volume (cornerstone volumes are image-backed and own no contiguous scalar array; materializing one is a full copy that must not be duplicated per viewport). Shared planar code and the labelmap display now resolve volume-kind render modes through the render backend registry (isVolumeRenderMode / an isGpuVolumeSliceRenderMode helper) instead of hardcoded ActorRenderMode literals, the addImages overlay gate keys on the CanvasActor cpuVolume mode rather than the composited surface, the resolved camera is mirrored onto the engine's vtk renderer for camera consumers, and the WebGPU window honors the viewport background (with a setWebGPUViewportBackground helper). Includes the genericWebGPUOrtho example: sagittal/coronal CT MPR with live webgpu/gpu/cpu switching, manipulation and annotation tools, segmentation editing on a mock labelmap, and a renderBackend URL parameter.
setValue runs per voxel; resolving the segmentation and labelmap layer through ensureLabelmapState there re-normalized the entire binding state for every voxel of a stroke. Cache the resolved layer per operation data (created fresh per stroke, so bindings cannot change mid-stroke) and map labelValue to segmentIndex through the layer's existing table.
… reorients the plane The planar fit parallel scale is recomputed from the reoriented plane's projected extent, which varies with obliquity, while the relative zoom was carried over unchanged -- so rotating a viewport through setViewReference pulsed in and out as the plane swept the volume. Resolve the hypothetical post-patch view and rescale the relative zoom so the absolute parallel scale is preserved, matching the legacy setViewReference semantics.
…kends genericWebGPUPetMIP: an inverted full-volume PET slab projection (maximum/minimum/average blend with a thickness slider) on the WebGPU volume-slice render mode, wheel-rotated around the patient axis, beside a drag-to-rotate CT-Bone 3D volume viewport. Live webgpu/gpu switching with a backend-colored background cue and a debug panel reporting each viewport's actual render mode (the 3D viewport has no WebGPU mode yet and stays on WebGL).
… visible bounds Legacy VolumeViewport3D follows the wide clipping-range default with renderer.resetCameraClippingRange() on every camera change, pulling the near/far planes tight around the actor bounds. The generic vtkVolume3d path only applied the wide +/-1e6 default, and volume ray casting derives its sampling from the clipping range, so renders showed banding and streaking that the legacy viewport does not. Apply the same wide-then-reset sequence on mount and on every camera update.
…MIP example Mirror the legacy TrackballRotateTool interaction trick: double the volume mapper's sample distance on drag start and restore it on release with a final full-resolution render.
b94fc89 to
a1abe84
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/tools/src/stateManagement/segmentation/helpers/labelmapImageMapperSupport.ts (1)
438-449: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant render-mode check.
Since
isNextPlanarViewportis defined usingisPlanarGpuVolumeSliceViewport(viewport)at line 362, it already enforces thatisGpuVolumeSliceRenderMode(...)is true (via line 101). Therefore, ifisNextPlanarViewportis true, the condition!isGpuVolumeSliceRenderMode(...)will never evaluate to true.Unsupported render modes for Next planar viewports are already caught at line 364, where the function returns
unsupported:viewport. You can safely remove this entire block.♻️ Proposed refactor
- if ( - isNextPlanarViewport && - !isGpuVolumeSliceRenderMode( - getPlanarPrimaryRenderMode(compatibilityViewport) - ) - ) { - return { - key: `unsupported:renderMode:${orientationKey}`, - sliceIndex: sliceIndex ?? NaN, - supported: false, - }; - } - return { // Use the discrete slice index instead of focal-point-derived world position.🤖 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/tools/src/stateManagement/segmentation/helpers/labelmapImageMapperSupport.ts` around lines 438 - 449, Remove the unreachable render-mode validation block guarded by isNextPlanarViewport and !isGpuVolumeSliceRenderMode in the label-mapping helper. Keep the existing unsupported:viewport handling for Next planar viewports unchanged, along with all subsequent mapping logic.
🤖 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/examples/genericWebGPUStack/index.ts`:
- Around line 330-343: Guard the “Use webgpu backend” toolbar registration in
the generic WebGPU example with isWebGPURenderingAvailable(), or disable its
handler when unavailable, so it cannot invoke switchBackend('webgpu') unless
WebGPU was successfully registered. Leave the GPU and CPU toolbar actions
unchanged.
---
Nitpick comments:
In
`@packages/tools/src/stateManagement/segmentation/helpers/labelmapImageMapperSupport.ts`:
- Around line 438-449: Remove the unreachable render-mode validation block
guarded by isNextPlanarViewport and !isGpuVolumeSliceRenderMode in the
label-mapping helper. Keep the existing unsupported:viewport handling for Next
planar viewports unchanged, along with all subsequent mapping logic.
🪄 Autofix (Beta)
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: 9a5a94cf-de9b-4232-a374-ec11997438d0
📒 Files selected for processing (27)
packages/core/examples/genericWebGPUOrtho/index.tspackages/core/examples/genericWebGPUPetMIP/index.tspackages/core/examples/genericWebGPUStack/index.tspackages/core/src/RenderingEngine/GenericViewport/Planar/DefaultPlanarDataProvider.tspackages/core/src/RenderingEngine/GenericViewport/Planar/PlanarLegacyCompatibilityController.tspackages/core/src/RenderingEngine/GenericViewport/Planar/PlanarResolvedView.tspackages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewReferenceController.tspackages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewport.tspackages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewportLegacyAdapter.tspackages/core/src/RenderingEngine/GenericViewport/Planar/WebGPUImageMapperRenderPath.tspackages/core/src/RenderingEngine/GenericViewport/Planar/WebGPUVolumeSliceRenderPath.tspackages/core/src/RenderingEngine/GenericViewport/Planar/planarRenderPathProjection.tspackages/core/src/RenderingEngine/GenericViewport/Planar/planarViewReference.tspackages/core/src/RenderingEngine/GenericViewport/Planar/vtkWebGPUModules.d.tspackages/core/src/RenderingEngine/GenericViewport/Planar/webgpuViewportRenderWindow.tspackages/core/src/RenderingEngine/GenericViewport/Volume3D/VtkVolume3DRenderPath.tspackages/core/src/RenderingEngine/helpers/registerWebGPURenderBackend.tspackages/core/src/index.tspackages/core/src/types/RenderBackendRegistry.tspackages/tools/src/stateManagement/segmentation/helpers/getViewportLabelmapRenderMode.tspackages/tools/src/stateManagement/segmentation/helpers/labelmapImageMapperSupport.tspackages/tools/src/tools/displayTools/Labelmap/labelmapActorStyle.tspackages/tools/src/tools/displayTools/Labelmap/labelmapRenderPlan/planarGenericVolumeLabelmap.tspackages/tools/src/tools/displayTools/Labelmap/labelmapRenderPlan/volumeSliceImageMapperPlan.tspackages/tools/src/tools/displayTools/Labelmap/volumeLabelmapSliceData.tspackages/tools/src/tools/segmentation/strategies/compositions/setValue.tsutils/ExampleRunner/example-info.json
🚧 Files skipped from review as they are similar to previous changes (24)
- packages/core/src/RenderingEngine/GenericViewport/Planar/vtkWebGPUModules.d.ts
- utils/ExampleRunner/example-info.json
- packages/tools/src/tools/displayTools/Labelmap/labelmapActorStyle.ts
- packages/tools/src/tools/displayTools/Labelmap/labelmapRenderPlan/planarGenericVolumeLabelmap.ts
- packages/core/src/RenderingEngine/GenericViewport/Volume3D/VtkVolume3DRenderPath.ts
- packages/core/src/types/RenderBackendRegistry.ts
- packages/tools/src/tools/segmentation/strategies/compositions/setValue.ts
- packages/tools/src/tools/displayTools/Labelmap/volumeLabelmapSliceData.ts
- packages/core/src/RenderingEngine/GenericViewport/Planar/DefaultPlanarDataProvider.ts
- packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarLegacyCompatibilityController.ts
- packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewport.ts
- packages/core/src/RenderingEngine/helpers/registerWebGPURenderBackend.ts
- packages/tools/src/tools/displayTools/Labelmap/labelmapRenderPlan/volumeSliceImageMapperPlan.ts
- packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewportLegacyAdapter.ts
- packages/core/src/index.ts
- packages/tools/src/stateManagement/segmentation/helpers/getViewportLabelmapRenderMode.ts
- packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewReferenceController.ts
- packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarResolvedView.ts
- packages/core/src/RenderingEngine/GenericViewport/Planar/WebGPUImageMapperRenderPath.ts
- packages/core/examples/genericWebGPUOrtho/index.ts
- packages/core/src/RenderingEngine/GenericViewport/Planar/planarViewReference.ts
- packages/core/src/RenderingEngine/GenericViewport/Planar/webgpuViewportRenderWindow.ts
- packages/core/src/RenderingEngine/GenericViewport/Planar/WebGPUVolumeSliceRenderPath.ts
- packages/core/examples/genericWebGPUPetMIP/index.ts
…mageMapper and translucent-pass fixes
sedghi
changed the title
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
james-hanks
changed the title
|
Superseded by #2894 — a request for a vtk-wasm render backend, delivered as a separable rendering module covering planar, MPR and 3D. The rejection here is about direction, not quality. vtk.js's WebGPU work is not going to be completed; the supported route to WebGPU is Most of what this branch established carries forward into #2894 and is credited there:
Several changes on this branch are not WebGPU-specific and should land on their own rather than being lost with it:
Could these be split out as their own PRs? |
Revised position — not rejected, if it moves out of core@sedghi I've gone back through the full diff and I'm withdrawing the rejection. I want to take a different route than dropping this. The offer: package the WebGPU backend as its own module — What changed my mind: of 43 changed files, only four core/tools edits are actually WebGPU-specific. Everything else is either the generic registry refactor — But "entirely outside core" is not achievable, and I don't want to pretend otherwise. Three things have to come from core, and I'd rather state them as the terms up front than have you discover them mid-rework: 1. Core has to export a render-path SDKThe two WebGPU paths import nine internal modules. None of them are exported from
Plus the type contracts: Roughly 15 named exports and a types entry point. This is the reason I'd rather do it this way than drop the branch: that surface is exactly the seam the vtk-wasm work in #2894 needs. Defining it against a real second backend that already exists is a far better test of it than designing it in the abstract and discovering the gaps later. 2. Backend-owned surfaces, so the two workarounds can goTwo of the four WebGPU-specific edits exist only because
A registry capability for "hosts vtk actors" (or backend-declared surfaces, which is the fuller fix and is already on the #2894 list) makes both special cases disappear. I don't want either literal landing in core. 3. The registry augmentation belongs in the pluginThis branch adds One packaging detailCore pins Suggested split
Happy to discuss the ordering, and to take the core-side pieces (2 and 3) if that's easier than reworking the whole branch yourself. Removing the rejection marker from the title now. |
wayfarer3130
changed the title
|
@wayfarer3130 Could you clarify how VTK.js is expected to be deprecated in favor of VTK.wasm? Is there an official statement from Kitware confirming this direction? as of today it's still maintained and WebGPU support is in parity with WebGL Kitware/vtk-js#3473 I also have some concerns about VTK.wasm as the primary path. While exposing VTK C++ has benefits, it makes debugging, patching, and contributing fixes more difficult(debug build with DWARF). We will have to deal with all their complex CPP tooling and build systems, even small fixes may need to go through the VTK development and release cycle. With VTK.js, we can debug directly in the browser, land fixes quickly or do postinstall patchs. VTK.wasm is a 22Mb(78Mb uncompressed) file that you have to load before doing anything. Do we gain any performance improvement compared to VTK.wasm compared to VTK.js WebGL or even WebGPU version ? Some NPM stats: VTK.js ESM : 100K downloads/week |
3 participants
Context
Groundwork for evaluating WebGPU as the rendering backend: an opt-in
webgpurender backend for planar GenericViewports covering image stacks and volume-slice (MPR) rendering through the vtk.js WebGPU view API.Depends on vtk.js fixes: Kitware/vtk-js#3547 (ImageMapper projection + imageData texture invalidation) and Kitware/vtk-js#3548 (translucent-pass depth for coplanar overlays, needed for labelmap segmentation). Both merged upstream and shipped in vtk.js v36.4.2; this branch now pins
@kitware/vtk.js36.4.2, so no linked build is required.Changes & Results
registerWebGPURenderBackend()(opt-in, throws withoutnavigator.gpu): registers thewebgpubackend with self-rendering image (webgpuImage) and volume-slice (webgpuVolume) render paths; each viewport gets a ref-counted vtk.js WebGPU render window whose frames are blitted into thecpusurface canvas after the device work completes. Volume rendering materializes one shared, ref-counted mapper-input scalar array per volume (cornerstone volumes are image-backed and own no contiguous array).ActorRenderModeliterals (image and volume kinds), so extension render modes participate in view resolution, view references, scrolling, labelmap styling, and the labelmap image-mapper plan.isImageRenderMode/isVolumeRenderModeare exported from core. TheaddImagesoverlay gate keys on the CanvasActorcpuVolumemode rather than the composited surface.setValueresolves labelmap bindings once per stroke instead of per voxel; labelmap slice extraction walks flat voxel indices incrementally (original per-voxel IJK loop kept as fallback; obliques are unaffected — axis-aligned extraction only) and reuses slice buffers.genericWebGPUStack(stack + tools + annotations + segmentation editing on a fake labelmap) andgenericWebGPUOrtho(sagittal/coronal CT MPR + mock ellipsoid labelmap + tools), both with live webgpu/gpu/cpu switching, a?renderBackend=URL parameter, and a debug panel reporting the effective backend, mounted render mode, and WebGPU adapter. New Generic Viewports section in the example index.Verified on the webgpu backend: stack and MPR rendering with pixel parity to WebGL, wheel scroll with exact pixel round-trip, zoom/pan/VOI/invert/rotation, annotation tools, labelmap segmentation rendering and editing with segment colors matching the gpu and cpu backends, and live backend switching without errors. JS heap holds a single shared volume copy regardless of viewport count (verified via heap snapshots).
Testing
pnpm installis enough, no linked build needed.genericWebGPUStackandgenericWebGPUOrthoexamples in Chrome; the debug panels should showwebgpuImage/webgpuVolumeand the WebGPU adapter. Use the backend buttons (or?renderBackend=gpu|cpu) to compare against gpu/cpu.Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals. (Experimental opt-in API; not documented on the docs site yet by design.)
Tested Environment
Summary by CodeRabbit