Repository navigation
[DEPRECATED] feat(ecg): ECG Next Viewport Enhancements - #2821
Harshika-Chandvani wants to merge 18 commits into
Conversation
…dapters to fix failing viewport tests
|
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:
📝 WalkthroughWalkthroughECG rendering now supports configurable sweep speed, sensitivity, amplitude labels, and multi-column lead layouts. Generic ECG viewports add scrolling, camera, image identity, calibration, and coordinate-mapping APIs. Examples, adapters, tests, annotation tools, metadata, and ECG measurement units are updated. ChangesECG rendering and viewport behavior
ECG tooling support
Non-functional maintenance
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ECGViewport
participant CanvasECGRenderPath
participant ECGUtilities
participant Canvas
User->>ECGViewport: scroll or change ECG presentation
ECGViewport->>CanvasECGRenderPath: request drawFrame
CanvasECGRenderPath->>ECGUtilities: compute metrics and channel layouts
ECGUtilities->>Canvas: draw calibrated grid, traces, and labels
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
wayfarer3130
changed the title
|
In general it is easier to review things individually rather than in bulk. |
|
For the physical units === -1, you need a NEW value for ms, not a change to the existing one - use -2 for ms if you want that, otherwise it is a breaking change. |
|
scroll() has an incompatible signature scroll(options?: { delta?: number }): void StackViewport.ts:2858 → scroll(delta: number, debounce = true, loop = false) (viewport as IStackViewport).scroll(delta, options.debounceLoading, options.loop); Fix: match the family convention: scroll(delta = 1, _debounceLoading = true, _loop = false): void { The scroll needs to be compatible and should be in the adapter class |
|
|
||
| type ECGViewportProperties = ViewportProperties & { | ||
| visibleChannels?: number[]; | ||
| sweepSpeed?: number; |
There was a problem hiding this comment.
There should be some consideration for what is displayed as part of the viewport, and what is external. Also, things like the layout type should probably come from a set of constants which actually define the layouts so there is a full object definition allowing other layouts to be added. I'm not quite sure where that should get added/setup. Perhaps a register new layout on the base class as a static? Or perhaps just allow it inline.
There was a problem hiding this comment.
export interface ECGLayout {
id: string;
label: string;
rows: number;
columns: number;
hasRhythmStrip?: boolean;
}
Then computeECGHeight, computeECGChannelLayouts, and computeECGRenderMetrics can look up the descriptor directly instead of using repeated if/else chains.
Two quick questions for you on this:
Does { id, label, rows, columns, hasRhythmStrip } cover everything needed, or are there other layout parameters you'd like included?
Would you prefer registering new layouts via a static method (e.g. ECGViewport.registerLayout(...)), or having ECG_LAYOUTS as a standalone object that callers can extend?
There was a problem hiding this comment.
It seems like there is a lot of hard coded layout information here - wondering if it can be split and unit tests added for various types of calculations.
There was a problem hiding this comment.
Agreed! I plan to split the pure math and layout functions (computeECGHeight, computeECGChannelLayouts, and computeECGRenderMetrics). Since these calculation functions have no DOM or canvas dependencies, add full unit test coverage for all layout variants (12x1, 6x2, 3x4, 3x4+1).
Quick question on file organization: Is it okay if I extract these calculation functions into a separate ECGLayoutCalculations.ts file, or would you prefer keeping them inside ECGUtilities.ts alongside static layout constants?
I'll include this refactor and test suite in my follow-up PR right after your feedback!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (4)
packages/core/src/RenderingEngine/GenericViewport/ECG/ECGViewport.ts (1)
505-507: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the existing top-level type import instead of an inline
import(...)type.The file already has a
import type { ... } from '../../../types'block (Lines 8-10); addIImageCalibrationthere.🤖 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/ECG/ECGViewport.ts` around lines 505 - 507, Update the top-level type import in ECGViewport to include IImageCalibration, then replace the inline import(...) type assertion for waveform.calibration with the imported IImageCalibration type while preserving the existing undefined union.packages/core/src/RenderingEngine/GenericViewport/ECG/ECGResolvedView.ts (1)
186-197: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the computed layouts like
getCanvasMappingdoes.
getChannelLayouts()recomputes the full layout (channel iteration, row-height pass, and an O(rows × items)findin the offset loop) on everycanvasToWorld/worldToCanvascall — these run per annotation handle per frame. The class already memoizescachedCanvasMapping; the same||=treatment applies here since the state is immutable per instance.🤖 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/ECG/ECGResolvedView.ts` around lines 186 - 197, Cache the result of getChannelLayouts() using the same ||= memoization pattern as getCanvasMapping, storing it on an instance field so repeated canvasToWorld/worldToCanvas calls reuse the computed layouts. Preserve the existing computeECGChannelLayouts inputs and behavior.packages/core/examples/genericEcg/index.ts (1)
186-193: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard the global keydown handler.
The listener is bound to
windowfor the page lifetime and reacts to arrow keys even when focus is in a toolbar input, and it never callspreventDefault(), so the page also scrolls. Binding toelement(withtabindex) or checkingevent.targetwould keep the demo behaviour predictable.🤖 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/genericEcg/index.ts` around lines 186 - 193, Update the keyboard handler around the window keydown listener so arrow-key scrolling only runs when the ECG demo element is focused or when the event target is outside interactive controls such as inputs and the toolbar. Prevent the browser’s default scrolling when handling ArrowLeft or ArrowRight, while preserving the existing viewport.scroll behavior.packages/core/src/RenderingEngine/GenericViewport/ECG/ECGViewportTypes.ts (1)
58-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the layout union into a shared type alias.
The same
'12x1' | '6x2' | '3x4' | '3x4+1'literal union is repeated inECGViewportProperties.ts,ECGUtilities.ts(three signatures), andECGResolvedView.ts. A single exportedECGLayoutType(or the layout-descriptor map already discussed in the earlier review thread) would keep them in sync.🤖 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/ECG/ECGViewportTypes.ts` around lines 58 - 65, Extract the repeated ECG layout literal union into an exported shared type alias, preferably ECGLayoutType, and update the layoutType declaration plus the corresponding usages in ECGViewportProperties, the three ECGUtilities signatures, and ECGResolvedView to reference it. Preserve the existing four supported layout values and use the shared alias consistently.
🤖 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/genericEcg/index.ts`:
- Around line 176-182: Update the Show/Hide All Traces synchronization near
addCheckboxToToolbar to retain the checkbox references returned by the helper
for each channel, rather than querying `#cornerstone-element-container`. Iterate
over that stored collection when applying the allVisible checked state so every
per-channel checkbox stays synchronized.
In `@packages/core/src/RenderingEngine/GenericViewport/ECG/ECGResolvedView.ts`:
- Around line 48-67: Update the layout selection in ECGResolvedView to avoid
falling back to channelLayouts[0] when subCanvasPos is outside a row’s bounds.
If no row contains the point, select the channel layout with the smallest
vertical distance to the point, preserving normal containing-row selection and
ensuring gaps or out-of-bounds positions resolve to the nearest row.
In `@packages/core/src/RenderingEngine/GenericViewport/ECG/ECGViewport.ts`:
- Around line 304-322: Update ECGViewport.setCamera to handle
cameraPatch.focalPoint in addition to scale updates. Mirror the legacy ECG
mapping by converting the focal point between canvas/world coordinates,
calculating the change delta from the current focal point, and passing that
delta to setPan(...), while preserving existing parallelScale and scale
behavior.
In
`@packages/core/src/RenderingEngine/GenericViewport/ECG/ECGViewportLegacyAdapter.ts`:
- Around line 46-53: Update ECGViewportLegacyAdapter.setProperties so partial
updates do not overwrite unspecified presentation fields with undefined. Before
calling setDisplaySetPresentation, omit undefined entries from the properties
object or merge them with the current presentation, preserving existing values
while applying only supplied fields.
In `@packages/core/src/utilities/ECGUtilities.ts`:
- Around line 297-300: Update the 3x4+1 layout handling in ECGUtilities so the
rhythm strip selects Lead II by channel identity rather than assuming
visibleChannels[1]. Give its layout entry a unique rhythm identity that cannot
collide with regular leadIndex values, and update ECGResolvedView
lookup/conversion logic to consistently match that identity for rhythm
annotations.
- Around line 546-560: Update the amplitude-label rendering around the
calibration block so mV values are calculated relative to each lead row’s
baseline rather than the canvas top, preserving a clinically meaningful per-lead
±mV scale; use the existing layout baseline information for each row or render
one calibration pulse. Also derive the label font size through the same
world-to-canvas scaling used by drawECGLabels instead of using the fixed
world-unit value 10.
- Around line 211-237: Update the row-height calculation around the visible
channel loop and totalHeight accumulation to use the same shared row-height
helper as computeECGChannelLayouts, including its 100 * channelScale * 1.25
fallback for empty rows. Replace any duplicated empty-row logic with that helper
so ecgHeight and rowYOffsets remain consistent for partially populated layouts.
- Around line 484-509: Update the calibrated grid-spacing logic in the
surrounding ECG grid-generation function to account for worldToCanvasRatio and
prevent excessive line counts when effective spacing falls below a few screen
pixels. Skip minor lines and coarsen major spacing as needed, while preserving
the physical 1 mm/5 mm spacing when it remains sufficiently visible; keep the
legacy auto-fit branch unchanged.
- Around line 409-413: Update the channelScale calculation in ECGUtilities to
apply the waveform’s actual ADC-to-mV calibration before converting to pixels,
rather than assuming one raw sample unit equals 1 mV. Thread the channel
sensitivity and units from getImageData().calibration through the ECG rendering
path, including drawECGTraces, and use that calibration when computing
channelScale so waveform amplitudes and mV grid labels remain consistent.
- Around line 584-602: Update computeECGChannelLayouts where
defaultStart/defaultEnd are resolved so each layout’s startSample/endSample is
intersected with the visible startIndex/endIndex window rather than taking
precedence over it. Preserve layout bounds while clamping the effective range to
the scroll time window, including the existing channel-length limits and valid
sampleCount behavior.
In `@packages/tools/src/tools/annotation/UltrasoundDirectionalTool.ts`:
- Around line 238-241: Persist the text-box state in the `editData` object
created by the surrounding drag flow, using the existing `movingTextBox`
property that `_dragCallback` reads. Map the local `_movingTextBox` value set in
the handle-selection logic into that property so text-box drags retain the flag
and do not enter the all-points movement path.
In `@packages/tools/src/utilities/getCalibratedUnits.ts`:
- Around line 128-133: The ECG millisecond paths currently overwrite the legacy
-1 calibration marker. Introduce a distinct ECG-specific enum value, then use it
for the millisecond scale conversion at
packages/tools/src/utilities/getCalibratedUnits.ts:128-133 and for millisecond
probe values and units at
packages/tools/src/utilities/getCalibratedUnits.ts:210-219, preserving -1’s
existing behavior elsewhere.
---
Nitpick comments:
In `@packages/core/examples/genericEcg/index.ts`:
- Around line 186-193: Update the keyboard handler around the window keydown
listener so arrow-key scrolling only runs when the ECG demo element is focused
or when the event target is outside interactive controls such as inputs and the
toolbar. Prevent the browser’s default scrolling when handling ArrowLeft or
ArrowRight, while preserving the existing viewport.scroll behavior.
In `@packages/core/src/RenderingEngine/GenericViewport/ECG/ECGResolvedView.ts`:
- Around line 186-197: Cache the result of getChannelLayouts() using the same
||= memoization pattern as getCanvasMapping, storing it on an instance field so
repeated canvasToWorld/worldToCanvas calls reuse the computed layouts. Preserve
the existing computeECGChannelLayouts inputs and behavior.
In `@packages/core/src/RenderingEngine/GenericViewport/ECG/ECGViewport.ts`:
- Around line 505-507: Update the top-level type import in ECGViewport to
include IImageCalibration, then replace the inline import(...) type assertion
for waveform.calibration with the imported IImageCalibration type while
preserving the existing undefined union.
In `@packages/core/src/RenderingEngine/GenericViewport/ECG/ECGViewportTypes.ts`:
- Around line 58-65: Extract the repeated ECG layout literal union into an
exported shared type alias, preferably ECGLayoutType, and update the layoutType
declaration plus the corresponding usages in ECGViewportProperties, the three
ECGUtilities signatures, and ECGResolvedView to reference it. Preserve the
existing four supported layout values and use the shared alias consistently.
🪄 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: 3738e52a-d63f-406e-97fd-fe5e5603b122
📒 Files selected for processing (17)
packages/core/examples/genericEcg/index.tspackages/core/src/RenderingEngine/ECGViewport.tspackages/core/src/RenderingEngine/GenericViewport/ECG/CanvasECGRenderPath.tspackages/core/src/RenderingEngine/GenericViewport/ECG/ECGResolvedView.tspackages/core/src/RenderingEngine/GenericViewport/ECG/ECGViewport.tspackages/core/src/RenderingEngine/GenericViewport/ECG/ECGViewportLegacyAdapter.tspackages/core/src/RenderingEngine/GenericViewport/ECG/ECGViewportTypes.tspackages/core/src/RenderingEngine/GenericViewport/ECG/ecgProjectionSnapshot.tspackages/core/src/RenderingEngine/GenericViewport/GenericViewport.tspackages/core/src/types/ECGViewportProperties.tspackages/core/src/utilities/ECGUtilities.tspackages/core/test/ecgViewport_test.jspackages/tools/src/tools/annotation/UltrasoundDirectionalTool.tspackages/tools/src/utilities/getCalibratedUnits.tspackages/tools/src/utilities/spatial/areViewportsSpatiallyLinked.tspackages/tools/src/utilities/spatial/spatial.spec.tspackages/tools/src/utilities/spatial/types.ts
💤 Files with no reviewable changes (1)
- packages/core/src/RenderingEngine/GenericViewport/ECG/ecgProjectionSnapshot.ts
…eform guard and calibration
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tools/src/utilities/getCalibratedUnits.ts`:
- Around line 244-247: Update the calibrationType assignment in
getCalibratedUnits to classify both -1 and -2 physicalUnitsYDirection values as
ECG regions, while preserving the existing US Region classification for other
values.
🪄 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: 2f559bf0-1e7c-4375-8241-93f6b41a1e72
📒 Files selected for processing (5)
packages/core/src/utilities/index.tspackages/core/src/utilities/viewportCapabilities.tspackages/metadata/src/utilities/metadataProvider/ecgFromInstance.tspackages/tools/src/tools/annotation/UltrasoundDirectionalTool.tspackages/tools/src/utilities/getCalibratedUnits.ts
…rasoundDirectionalTool and scroll window bounds in ECGUtilities
…ods to increase coverage
… grid line density, per-lead mV labels, and focalPoint panning
|
Hi @wayfarer3130, Following our discussion during OHIF Office Hours on the Modular Waveform Viewport Architecture, we are transitioning the changes from this PR (#2821) into a series of smaller, focused, and review-friendly PRs: 📦 Modular PR Roadmap: Please check out #2899 when you get a moment. Thank you! |
`ECGResolvedView.worldToCanvas` looked a layout cell up with `find(item => item.leadIndex === z)`. In the `3x4+1` layout the grid cell for lead II and the rhythm strip both carried `leadIndex === 1`, so the lookup always returned the grid cell and an annotation on the rhythm strip moved to the wrong cell. `leadIndex` was also an index into the *filtered* visible-channel list, so hiding one lead renumbered every lead after it. `leadIndex` now identifies one layout cell, and it is stable: - A grid cell carries the index of its channel in the unfiltered channel list, so the identifier survives a change of the visible leads or of the layout. `getVisibleECGChannelEntries` returns those indices, and both callers (`CanvasECGRenderPath`, `ECGResolvedView`) pass them through the new `leadIndices` argument. - The `3x4+1` rhythm strip carries a synthetic identifier above the last channel index, and it sets `isRhythm`. `getImageData` reserves that index in `dimensions[2]`, because `indexWithinDimensions` otherwise rejects a handle placed on the rhythm strip. Two related defects go with it: - `computeECGChannelLayouts` dropped every channel past the nominal grid size, so a 15-lead ECG lost three traces in the `12x1` layout. The grid now grows above the nominal size, and the column count drives the time segmentation instead of a hard-coded 2 or 4. - `computeECGHeight` and `computeECGChannelLayouts` chose the rhythm lead by different rules (`visibleChannels[1]` against a name match on `ii`), so the reserved height disagreed with the drawn layout. Both now derive from one `computeECGLayoutGrid` helper, and the shared rule uses `\bii\b` so lead III no longer matches. `ECGResolvedView` caches the layout, which it previously recomputed on every `canvasToWorld` and `worldToCanvas` call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nits Nine defects from the review of this pull request. Each one is independent of the others, except that the amplitude scale and the measurement units both need the channel sensitivity, which nothing read before. Amplitude scale. `computeECGRenderMetrics` set the calibrated `channelScale` to `sensitivityMmMv * ECG_PX_PER_MM`, which is pixels for each millivolt, and it applied that value to raw sample units. The scale was too large by the reciprocal of the millivolts for each unit, which is 1000 for a typical ECG: a `sensitivityMmMv` of 10 gave an `ecgHeight` of about 2.3 million pixels, the traces collapsed to a hairline, and the major-grid loop ran about 120 000 times for each frame. `ecgFromInstance` now reads `ChannelSensitivity`, `ChannelSensitivityCorrectionFactor` and `ChannelSensitivityUnitsSequence`, and it reports the millivolts for each raw unit as `mvPerUnit`. `loadECGWaveform` carries the value on each channel, and the scale uses it. The default stays 0.001 mV for each unit when the instance omits the sensitivity. Grid. `drawECGGrid` placed the vertical lines from the deprecated `ECG_SECONDS_WIDTH` (150 px/s) while `ecgWidth` came from `sweepSpeed * ECG_PX_PER_MM` (94.475 px/s at 25 mm/s), so a major block covered 0.317 s in place of 0.2 s. The metrics now report `pxPerSecond` and the resolved `sweepSpeed`, and one formula serves the calibrated path and the auto-fit path. The deprecated `RenderingEngine/ECGViewport` keeps its previous spacing, because the formula reproduces it exactly from its own pixels for each second. Traces. `drawECGTraces` lost its clamp against `channel.data.length` when the layout started to set `endSample` for every cell. A channel array shorter than `numberOfSamples` yielded `undefined` samples and `ctx.lineTo(x, NaN)`, which blanked the trace. The clamp is explicit again, and a cell whose window falls past the end of the data still draws its baseline. Camera. `ECGViewport.setCamera` added a world-space difference straight to a pan in canvas pixels, and the vertical direction was inverted because `buildICamera` sets `viewUp: [0, -1, 0]`. The focal point now goes through `worldToCanvas`, as `WSIViewportLegacyAdapter` does. Measurement units. `ecgCalibrationProvider` wrote `physicalUnitsYDirection: -2` to mean "the X axis is in milliseconds", so X-axis information sat in a Y-axis field. `getCalibratedUnits` then read `UNIT_MAPPING[-2]` for the area unit and reported `ms² ECG Region`. An ECG region now writes -1 for millivolts, the X axis keeps the DICOM code for seconds, and the display layer converts to milliseconds. The area unit reports `ms·mV`, because the area of an ECG region is a time multiplied by an amplitude and not a squared quantity. A region stored with -2 still reads. The same branch decided the measured axis by comparing a time in seconds against an amplitude in millivolts, which have no common unit, and it then wrote `scale = scaleY`, which made `calculateLengthInIndex` apply the amplitude scale to the X component. The axis now comes from the fraction of each axis that the annotation spans, and the two scales stay on their own axes. Viewport guard. `UltrasoundDirectionalTool` replaced `instanceof StackViewport` with `viewportSupportsImageSlices`, but the legacy `VolumeViewport` exposes all four of those methods, so the tool accepted a volume viewport. The guard now asks `getCurrentMode` for a generic viewport, and falls back to the volume capability for a legacy viewport, which `StackViewport` does not carry. Image identity. `ECGViewport.hasImageURI` used `dataId.includes(imageURI)`, which matched a UID that is a prefix of the bound UID. It compares whole identifiers now. Hit test. The nearest-cell fall-back of `ECGResolvedView.canvasToWorld` measured the vertical distance alone, so a point outside the grid in a multi-column layout always mapped to column 0. It measures the distance to the rectangle of the cell in both axes. Tests: 4 new files, 33 tests. The core, metadata and tools suites pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Design items D2, D4 and D7 of the review.
D2. `ECGResolvedView` took the render metrics as a constructor argument, and
`ECGViewport.getResolvedView` read them from `binding.rendering`, where
`drawFrame` had written them during the previous draw. A transform was therefore
one frame stale, and before the first draw it used the placeholder metrics of
`{ecgWidth: 1, ecgHeight: 1, channelScale: 1}`. The render path owned the world
geometry, which inverts the ownership that the view contract states: a resolved
view "is an ephemeral snapshot produced from data, canvas geometry, and
ViewState; it owns world/canvas transforms and renderer geometry".
`ECGResolvedView` now computes the metrics, the channel layouts and the canvas
transform from the data, the canvas and the view state, and it caches each one.
`ECGCanvasRenderContext` gains `getResolvedView`, so `drawFrame` reads all three
in place of computing them. The viewport therefore keeps one copy of the view
state and of the presentation, and the transform that a tool uses is always the
geometry that the frame drew.
`ECGCanvasRendering` no longer holds `metrics`, `currentCamera` or
`currentDataPresentation`, so `applyViewState` and `updateDataPresentation` have
nothing to store: the canvas redraws in full for each frame.
D4. `ChannelLayout` in `ECGViewportTypes` was dead, and it declared 4 of the 12
fields that `ECGChannelLayout` declares. It is deleted. `RenderWindowMetrics`
was a structural copy of `ECGRenderMetrics`, and `CanvasECGRenderPath` cast
between the two, which hid any future drift. It is now an alias.
D7. `getCamera` and `setCamera` are the legacy `ICamera` surface, and the Video,
WSI and Planar families all keep them on their legacy adapter. They move to
`ECGViewportLegacyAdapter`, together with the private `setFocalPoint`.
`ViewportType.ECG` resolves to that adapter, so an application that asks for a
legacy ECG viewport still finds both methods. A direct `ECG_NEXT` viewport now
exposes `getZoom`, `setZoom`, `getPan`, `setPan` and `getViewState` only, which
matches `WHOLE_SLIDE_NEXT`. `getRotation` stays on the core class, as it does on
WSI and Planar.
One defect fell out of the change. `getSignalScale` in `ecgProjectionSnapshot`
measured the sampling density between canvas `[0, 0]` and `[1, 0]`.
`canvasToWorld` clamps a sample index to the signal, and the ECG content is
centred, so the canvas origin normally lies outside the drawn content: both
samples clamped to sample 0 and the projection reported a density of 0. The
finite difference now starts at the centre of the canvas. The stub metrics in
`viewportProjectionService.jest.js` had masked this, and that test now derives
its expectations from the resolved view.
Tests: `packages/core/test/ecgResolvedView.jest.js`, 15 tests. They prove that
the metrics are real with no draw, that they follow a change of the time window,
of the sensitivity, of the sweep speed and of the layout, and that the transform
round trips through every cell of the `3x4+1` layout.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
wayfarer3130
changed the title
2 participants
DEPRECATED - in favour of:
PR 1 (#2899) — Core Primitive: GenericViewport ECG architecture & metadata parsing.
PR 2 (#2900) — Layouts & Calculations: Multi-lead presets, sensitivity scaling, and grid alignment.
PR 3 (#2901) — Tools & Demo: WaveformRegionOverlayTool, annotation calibration (ms/mV), and demo.
Context
First step toward a dedicated ECG mode in OHIF, following guidance from the team during office hours (see OHIF/Viewers#6068).
The team recommended building on the Cornerstone side first — extending the base viewport rather than creating a fully custom one — before layering an OHIF mode on top.
Changes & Results
New:
GenericViewportabstract base class (RenderingEngine/GenericViewport/GenericViewport.ts)Rendering-backend-agnostic base owning binding orchestration, shared view state, per-dataset presentation state, coordinate transforms, and camera events.
New:
ECGViewportextendingGenericViewport(GenericViewport/ECG/ECGViewport.ts)Full reimplementation:
setDisplaySets,scroll,scrollToTime,setZoom,setPan,getVisibleChannels,getWaveformData,resetViewState.New:
ECGViewportLegacyAdapterThin adapter re-exposing the existing
setEcg/setChannelVisibility/getProperties/setPropertiesAPI — no breaking changes to existing callers.New:
CanvasECGRenderPath— canvas 2D rendering extracted into a standalone render path.New:
ECGResolvedView— coordinate transforms: canvas pixels ↔ ECG world space (time ms, amplitude mV).Updated:
UltrasoundDirectionalTool— addedECGGenericViewportto supported viewport type guard.Updated:
getCalibratedUnits— ECG X-axis measurements now output milliseconds (not raw seconds).The existing
ECGViewportclass is untouched — no breaking changes. No new Zustand stores or event patterns.Verification & Testing
Unit tests added in
packages/core/test/ecgViewport_test.jscovering:canvasToWorld/worldToCanvasround-tripsresetViewStateFor manual testing, see the example app at
packages/core/examples/genericEcg/index.ts.Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit