Fix shader compilation errors with scatterplot extensions, start using UBOs for deck.gl extensions #273
Conversation
WalkthroughThe pull request introduces shader system refactoring for ScatterDeckExtension, transitioning to a module-based uniform injection pattern with runtime state updates. Additional changes consist of documentation comments and configuration placeholders across related extension and component files, with no functional impact. Changes
Sequence DiagramsequenceDiagram
participant Layer as Deck.gl Layer
participant Extension as ScatterDensityExension
participant Shader as Fragment Shader
Layer->>Extension: updateState(params)
activate Extension
Extension->>Extension: Propagate opacity prop<br/>into shader inputs
Extension-->>Layer: ✓ State updated
deactivate Extension
Layer->>Extension: getShaders(context)
activate Extension
Extension->>Extension: Create ScatterDensityExension<br/>module with opacity uniform
Extension->>Shader: Inject module & uniforms
Extension-->>Layer: Return merged modules
deactivate Extension
Layer->>Shader: Render with<br/>scatterDensity.opacity
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utilities/jpeg2000/Jpeg2000Decoder.ts (1)
62-82: Static OpenJPEG cache looks good; add type assertion at return statement for strict modeThe switch to a static
Promise<OpenJpegModule>cache plus thefinally‑baseddecoder?.delete()cleanup are solid improvements. However, with"strict": truein your tsconfig, TypeScript'sstrictNullCheckswill flag the return statement at line 81. Static fields are not type-narrowed after assignment, so you need an explicit non-null assertion:private async getOpenJPEG(): Promise<OpenJpegModule> { if (!Jpeg2000Decoder.openjpeg) { try { Jpeg2000Decoder.openjpeg = openJpegFactory({ locateFile: (file: string) => { if (file.endsWith(".wasm")) { return openjpegWasm.href; } else { return file; } }, }); } catch (error) { console.warn("WASM version failed, JS version fallback not attempted:", error); throw new Error("Failed to initialize OpenJPEG codec"); } } - return Jpeg2000Decoder.openjpeg; + return Jpeg2000Decoder.openjpeg!; }This satisfies strict type checks while preserving runtime behavior. The decoder cleanup in
decodeBlock(lines 87–122) is already correct.
🧹 Nitpick comments (2)
src/webgl/ImageArrayDeckExtension.ts (1)
17-18: UBO migration note is clear; keep in sync with other extensionsThe TODOs correctly flag that this extension still uses classic uniforms while others move to UBO/shaderInputs. No functional issue, just make sure when you do the migration here you mirror the module/uniformTypes patterns used in the newer extensions so behavior stays consistent.
vitest.config.ts (1)
4-14: Commented BrowserTestDriver stub is fine as documentationThe added comments documenting the planned BrowserTestDriver setup are harmless and useful context. When you revisit this, you’ll likely want to swap the
webpack-dev-serverreference for whatever vite-based runner you settle on.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
package.json(2 hunks)src/react/components/VivScatterComponent.tsx(1 hunks)src/tests/ScatterDeckExtension.spec.ts(1 hunks)src/utilities/jpeg2000/Jpeg2000Decoder.ts(3 hunks)src/webgl/ImageArrayDeckExtension.ts(1 hunks)src/webgl/ScatterDeckExtension.ts(3 hunks)src/webgl/VivContrastExtension.ts(1 hunks)vitest.config.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 190
File: src/lib/deckMonkeypatch.ts:10-15
Timestamp: 2025-04-30T12:20:44.468Z
Learning: The removal of `EventManager.setElement()` in mjolnir.js necessitated the development of a custom monkeypatching solution in MDV to handle events in popout windows for deck.gl components.
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 190
File: src/lib/deckMonkeypatch.ts:68-81
Timestamp: 2025-04-30T12:24:02.415Z
Learning: The implementation in `src/lib/deckMonkeypatch.ts` mimics the official deck.gl source code, where EventManager is initialized with only three pointer events (pointerdown, pointermove, pointerleave) and not with explicit wheel, keydown, or keyup events.
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 162
File: src/react/components/AxisComponent.tsx:58-67
Timestamp: 2025-04-23T09:25:51.968Z
Learning: When implementing deck.gl components in React, axis scales can appear to be a frame behind due to synchronization issues between deck.gl's render cycle and React's component updates.
📚 Learning: 2025-04-23T09:25:51.968Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 162
File: src/react/components/AxisComponent.tsx:58-67
Timestamp: 2025-04-23T09:25:51.968Z
Learning: In the deck.gl scatter plots implementation, the unproject function has issues with coordinate spaces (boundingClientRect vs. zero-based), which causes selection problems with the EditableGeoJsonLayer.
Applied to files:
src/tests/ScatterDeckExtension.spec.tssrc/react/components/VivScatterComponent.tsxsrc/webgl/ScatterDeckExtension.ts
📚 Learning: 2025-04-23T08:48:20.916Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 162
File: src/react/components/DeckScatterReactWrapper.tsx:42-44
Timestamp: 2025-04-23T08:48:20.916Z
Learning: In DeckScatterReactWrapper.tsx, properties like maxZoom and minZoom in viewState are currently treated as constants rather than user-configurable settings, and are not preserved during serialization in getConfig().
Applied to files:
src/react/components/VivScatterComponent.tsx
📚 Learning: 2025-04-30T12:24:02.415Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 190
File: src/lib/deckMonkeypatch.ts:68-81
Timestamp: 2025-04-30T12:24:02.415Z
Learning: The implementation in `src/lib/deckMonkeypatch.ts` mimics the official deck.gl source code, where EventManager is initialized with only three pointer events (pointerdown, pointermove, pointerleave) and not with explicit wheel, keydown, or keyup events.
Applied to files:
src/react/components/VivScatterComponent.tsx
📚 Learning: 2025-04-09T20:29:56.245Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 0
File: :0-0
Timestamp: 2025-04-09T20:29:56.245Z
Learning: When working with D3.js in React components, it's appropriate to intentionally exclude certain dependencies from useEffect dependency arrays to prevent unnecessary recreation of D3 elements. This approach should be documented with explanatory comments and ESLint suppression comments like `// eslint-disable-next-line react-hooks/exhaustive-deps` to clarify the intentional exclusion.
Applied to files:
src/react/components/VivScatterComponent.tsx
📚 Learning: 2025-04-23T09:25:51.968Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 162
File: src/react/components/AxisComponent.tsx:58-67
Timestamp: 2025-04-23T09:25:51.968Z
Learning: When implementing deck.gl components in React, axis scales can appear to be a frame behind due to synchronization issues between deck.gl's render cycle and React's component updates.
Applied to files:
src/react/components/VivScatterComponent.tsx
📚 Learning: 2025-07-23T09:12:07.073Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 247
File: src/utilities/jpeg2000/Jpeg2000Decoder.ts:160-184
Timestamp: 2025-07-23T09:12:07.073Z
Learning: In this codebase's JPEG2000 decoder implementation, Chafey's openjpegwasm fork (https://github.com/chafey/openjpegjs) is recommended over the Cornerstone codec for handling raw JPEG2000 codestreams from TIFF tiles. The fork includes specific changes for partial bitstream decoding which is needed when TIFF stores JPEG2000 as raw codestreams without JP2 wrapper headers.
Applied to files:
src/utilities/jpeg2000/Jpeg2000Decoder.ts
📚 Learning: 2025-08-04T12:10:00.862Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 247
File: src/utilities/jpeg2000/Jpeg2000Decoder.ts:122-136
Timestamp: 2025-08-04T12:10:00.862Z
Learning: In the JPEG2000 decoder implementation at `src/utilities/jpeg2000/Jpeg2000Decoder.ts`, the `getPixelData` function must return `pixelData.buffer.slice()` (an ArrayBuffer) rather than the TypedArray directly. The slice operation is critical to prevent image corruption, and returning the TypedArray directly would be the wrong type for the decoder interface.
Applied to files:
src/utilities/jpeg2000/Jpeg2000Decoder.ts
📚 Learning: 2025-07-23T09:12:07.073Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 247
File: src/utilities/jpeg2000/Jpeg2000Decoder.ts:160-184
Timestamp: 2025-07-23T09:12:07.073Z
Learning: HTJ2K (High-throughput JPEG2000) via OpenJPH library offers faster processing performance but is not expected in current data formats for this codebase. It may be considered for future optimization when processing speed becomes critical.
Applied to files:
src/utilities/jpeg2000/Jpeg2000Decoder.ts
🧬 Code graph analysis (1)
src/tests/ScatterDeckExtension.spec.ts (1)
src/webgl/ScatterDeckExtension.ts (1)
ScatterSquareExtension(10-48)
🔇 Additional comments (1)
src/react/components/VivScatterComponent.tsx (1)
186-191: Found the version:deck.gl ~9.1.11. Now let me verify thatdebugSpectorJSis a valid option for device creation:Commented
devicePropswithdebugSpectorJSis valid and supportedThe commented
devicePropsblock withdebugSpectorJS: trueis a valid option for luma.gl device creation. Your project uses deck.gl ~9.1.11, which fully supports this feature. The inlinecontrollerconfig and commented-outdevicePropsare fine as-is and don't affect behavior. If you later enabledevicePropsand derive any values from props/state, remember to include those in theuseMemodependency array to keep the deck props consistent.
package.json
Outdated
| "@deck.gl/test-utils": "^9.1.11", | ||
| "@luma.gl/constants": "~9.1.9", | ||
| "@luma.gl/core": "~9.1.9", | ||
| "@luma.gl/engine": "~9.1.9", | ||
| "@luma.gl/shadertools": "~9.1.9", | ||
| "@luma.gl/test-utils": "~9.1.9", | ||
| "@luma.gl/webgl": "~9.1.9", | ||
| "@luma.gl/webgpu": "~9.1.9", |
There was a problem hiding this comment.
🧩 Analysis chain
New WebGL test deps make sense; consider deduping @luma.gl/core
The added deck.gl/luma.gl test-utils and puppeteer entries are consistent with the new extension/WebGL tests in this PR. One small cleanup: @luma.gl/core now exists in both dependencies and devDependencies at the same version, which is redundant and can be simplified to a single entry (probably under dependencies only).
If you want to double‑check usage and where it belongs, you can run something like:
Also applies to: 154-154
🏁 Script executed:
#!/bin/bash
# Show all references to @luma.gl/core in the repo
rg -n "@luma.gl/core" .Length of output: 2279
🏁 Script executed:
#!/bin/bash
# Search for @luma.gl/core imports in test files
rg -n "@luma.gl/core" . -g "*test*" -g "*spec*" -g "*\.test\.*" -g "*\.spec\.*"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if src/ files that import @luma.gl/core are bundled into dist/
fd -e ts -e js -e tsx -e jsx . src/ | head -20Length of output: 564
🏁 Script executed:
#!/bin/bash
# Verify the npm structure: devDependencies should be for dev-only code (tests, build tools, etc.)
# Check if any non-src files import @luma.gl/core (tests, build scripts, etc.)
rg -n "@luma.gl/core" . --type-not=json -g "!package-lock.json" -g "!node_modules" | grep -v "^\.\/src/"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Verify npm duplication is a real problem using npm best practices
pip install -q -U pip 2>/dev/null && python3 << 'EOF'
import json
import sys
# npm best practice: a package should appear in either dependencies OR devDependencies, not both
# Exception: if a package is needed for production (src/) AND tests, it goes in dependencies
# and the test build inherits it; no need to duplicate in devDependencies
print("npm best practice analysis:")
print("- Production dependencies: packages needed to run the code")
print("- Dev dependencies: packages needed only for development/testing/building")
print("- If a package is imported in src/, it belongs in dependencies")
print("- If it's also imported in tests, it still only needs to be in dependencies")
print("- Having a package in BOTH sections is redundant and creates maintenance burden")
EOFLength of output: 379
Remove @luma.gl/core from devDependencies (line 125)
@luma.gl/core is imported only in production source files (src/webgl/HeatmapContourExtension.ts, src/webgl/ImageArray.ts, src/dataloaders/DataLoaders.ts, src/datastore/DataStore.js) and not in test files. It should appear in dependencies only; remove the redundant entry from devDependencies at line 125 to simplify maintenance.
🤖 Prompt for AI Agents
In package.json around lines 123 to 130, devDependencies contains
"@luma.gl/core" but that package is only imported by production source files;
remove the "@luma.gl/core" entry from devDependencies (line 125) and ensure the
package remains listed under dependencies so production builds continue to work;
update package.json accordingly and run a quick install/check to confirm no
test-only references remain.
There was a problem hiding this comment.
These dependencies changed in order to allow everything to resolve with the various test-utils, but actually we still haven't managed to configure those correctly, and now the conflicts with main are problematic so I think I'm going to revert changes to package.json & package-lock.json so that the actual bug-fixes in this branch can be merged, and review the test utils later probably along with another update of viv/deck/luma stack (and the addition of initial spatialdata.js).
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
src/webgl/VivContrastExtension.ts
Outdated
| const { props } = params; | ||
| const { contrast, brightness } = props; | ||
| // should be arrays of 6... probably in imageSettingsStore. | ||
| // should be arrays of N_CHANNELS... probably in imageSettingsStore. | ||
| // --- would be nice if viv etc were better about managing variable number of channels --- | ||
| if (contrast.length !== 6 || brightness.length !== 6) { | ||
| if (contrast.length !== N_CHANNELS || brightness.length !== N_CHANNELS) { | ||
| // throw new Error('contrast and brightness must be arrays of length 6'); | ||
| const contrastA = Array(6); | ||
| const brightnessA = Array(6); | ||
| for (let i = 0; i < 6; i++) { | ||
| //don't really car about default values for missing entries... | ||
| const contrastA = Array(N_CHANNELS); | ||
| const brightnessA = Array(N_CHANNELS); | ||
| for (let i = 0; i < N_CHANNELS; i++) { | ||
| //don't really care about default values for missing entries... | ||
| contrastA[i] = contrast[i] ?? 1; | ||
| brightnessA[i] = brightness[i] ?? 0; | ||
| } | ||
| for (const model of this.getModels()) | ||
| model.setUniforms({ | ||
| contrast: contrastA, | ||
| brightness: brightnessA, | ||
| //(method) Layer<ContrastProps>.getModels(): Model[] | ||
| for (const model of this.getModels()) { | ||
| // todo - update this to fit current deck.gl syntax for setting buffer... | ||
| //Instead of calling `model.setUniforms` (or `model.setBindings`) use `model.shaderInputs.setProps` to update the UBO with props | ||
| // model.setUniforms({ | ||
| // contrast: contrastA, | ||
| // brightness: brightnessA, | ||
| // }); | ||
| //We've tried to make sure that this extension has an appropriate type parameter, so ShaderPropsT should be ContrastProps | ||
| //However, although `this` has a generic parameter, `getModels` is returning `Model[]` | ||
| // so I don't understand how `model.shaderInputs` is supposed to have the correct type. | ||
| //definition in deck.gl: setProps(props: Partial<{[P in keyof ShaderPropsT]?: Partial<ShaderPropsT[P]>}>): void | ||
| //LSP says | ||
| //(method) ShaderInputs<Partial<Record<string, Record<string, unknown>>>>.setProps(props: Partial<{ | ||
| //[x: string]: Partial<Record<string, unknown> | undefined>; | ||
| //}>): void | ||
|
|
||
| //... but then again, the LSP seems a bit confused about basic indentation, let alone type generics. | ||
|
|
||
| //model.shaderInputs.setProps({custom: CustomProps}); | ||
|
|
||
| model.shaderInputs.setProps({ | ||
| "VivContrastExtension": { | ||
| // vivContrast: { | ||
| // contrast: contrastA, | ||
| // brightness: brightnessA, | ||
| // }, | ||
| contrast: contrastA, | ||
| brightness: brightnessA, | ||
| redTest: 0.5, | ||
| greenTest: 0.5, | ||
| }, | ||
| }); | ||
| } | ||
| return; | ||
| } | ||
| for (const model of this.getModels()) | ||
| model.setUniforms({ contrast, brightness }); | ||
| for (const model of this.getModels()) { | ||
| // model.setUniforms({ contrast, brightness }); | ||
| model.shaderInputs.setProps({ | ||
| "VivContrastExtension": { | ||
| // vivContrast: { | ||
| // contrast, | ||
| // brightness, | ||
| // }, | ||
| // re-using the arrays - is there a possibility that luma/deck.gl will not update the mutated contents? | ||
| contrast, | ||
| brightness, | ||
| redTest: 1.5, | ||
| greenTest: 0.9, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Pass through redTest/greenTest props instead of hard-coded constants.
updateState always writes fixed numbers into the UBO, so any redTest/greenTest values supplied via ContrastProps never reach the shader. That makes the new props useless and prevents callers from configuring the extension. Please forward the props (with sensible fallbacks) when updating shaderInputs.
- const { contrast, brightness } = props;
+ const { contrast, brightness, redTest, greenTest } = props;
@@
- "VivContrastExtension": {
- contrast: contrastA,
- brightness: brightnessA,
- redTest: 0.5,
- greenTest: 0.5,
- },
+ "VivContrastExtension": {
+ contrast: contrastA,
+ brightness: brightnessA,
+ redTest: redTest ?? 0.5,
+ greenTest: greenTest ?? 0.5,
+ },
@@
- "VivContrastExtension": {
- contrast,
- brightness,
- redTest: 1.5,
- greenTest: 0.9,
- },
+ "VivContrastExtension": {
+ contrast,
+ brightness,
+ redTest: redTest ?? 1.5,
+ greenTest: greenTest ?? 0.9,
+ },🤖 Prompt for AI Agents
In src/webgl/VivContrastExtension.ts around lines 83 to 146, updateState
currently writes hard-coded redTest/greenTest values into shaderInputs, so
props.redTest and props.greenTest never reach the shader; extract redTest and
greenTest from params.props (with sensible fallbacks, e.g. props.redTest ?? 0.5
and props.greenTest ?? 0.5) and use those variables when calling
model.shaderInputs.setProps in both the "padded arrays" branch and the normal
branch so the passed-in values are forwarded to the UBO.
There was a problem hiding this comment.
Those properties and associated uniforms etc should be removed before merging
There was a problem hiding this comment.
@xinaesthete, understood! I'll disregard the suggestion to forward those props since redTest and greenTest are temporary test properties that should be removed before merging. Once they're cleaned up, the ContrastProps interface will only include contrast and brightness arrays, and the shader module will no longer reference those test uniforms.
…'t working and we have conflicts and issues with resolution.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/react/components/VivScatterComponent.tsx (1)
196-199: Consider consolidating or removing the commented-out debugging configuration.You now have two separate commented-out
devicePropsblocks (lines 186-192 and 196-199), which creates duplication. If you plan to use SpectorJS debugging, consider merging both configurations into a single commented block for clarity:controller: { doubleClickZoom: false, }, - // deviceProps: { - // // todo - get this working more usefully. - // debugSpectorJS: true, - // } }),Alternatively, if you do intend to enable SpectorJS for debugging, consolidate both blocks:
// deviceProps: { // webgl: { // depth: true, // preserveDrawingBuffer: true, // antialias: true, // }, // debugSpectorJS: true, // },
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/react/components/VivScatterComponent.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-23T08:48:20.916Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 162
File: src/react/components/DeckScatterReactWrapper.tsx:42-44
Timestamp: 2025-04-23T08:48:20.916Z
Learning: In DeckScatterReactWrapper.tsx, properties like maxZoom and minZoom in viewState are currently treated as constants rather than user-configurable settings, and are not preserved during serialization in getConfig().
Applied to files:
src/react/components/VivScatterComponent.tsx
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/tests/ScatterDeckExtension.spec.ts (1)
1-61: Clarify placeholder status and double‑check symbols before re‑enabling these testsRight now this file is entirely commented, so it has no effect on the test suite and is fine as a parked scaffold. Two things to consider so it doesn’t bit‑rot:
- Make the intent explicit (e.g., a
TODOon Line 1 pointing to the deck.gl test‑utils config issue or a tracking ticket), so future readers know why this spec is disabled and when to revisit it.- Before uncommenting, verify:
- The imported
ScatterDensityExensionname matches the actual export fromScatterDeckExtension.ts.- The Vitest
testcallback usage ((t) => { ... t.annotate(...) }) is compatible with the current Vitest API, or drop thetparameter and the annotation if not needed.- The
ScatterplotLayer<Position>generic usage andOrthographicViewoptions line up with the deck.gl 9.1.11 test‑utils examples you plan to follow.If you’d like, I can help turn this into a
test.skip/describe.skipversion that still type‑checks and imports correctly while staying out of the active test run.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/tests/ScatterDeckExtension.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-23T09:25:51.968Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 162
File: src/react/components/AxisComponent.tsx:58-67
Timestamp: 2025-04-23T09:25:51.968Z
Learning: In the deck.gl scatter plots implementation, the unproject function has issues with coordinate spaces (boundingClientRect vs. zero-based), which causes selection problems with the EditableGeoJsonLayer.
Applied to files:
src/tests/ScatterDeckExtension.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tsc
xinaesthete
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/webgl/ScatterDeckExtension.ts (1)
47-47: Typo in class name: "Exension" should be "Extension".The class name
ScatterDensityExensionhas a typo (missing 't'). This typo propagates to the component name (line 49), module name (lines 54, 86), and type references throughout. While consistent within this file, consider renaming toScatterDensityExtensionfor clarity unless already referenced externally.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/webgl/HeatmapContourExtension.ts(2 hunks)src/webgl/ScatterDeckExtension.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/webgl/HeatmapContourExtension.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-23T09:25:51.968Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 162
File: src/react/components/AxisComponent.tsx:58-67
Timestamp: 2025-04-23T09:25:51.968Z
Learning: In the deck.gl scatter plots implementation, the unproject function has issues with coordinate spaces (boundingClientRect vs. zero-based), which causes selection problems with the EditableGeoJsonLayer.
Applied to files:
src/webgl/ScatterDeckExtension.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tsc
🔇 Additional comments (4)
src/webgl/ScatterDeckExtension.ts (4)
1-1: LGTM: Type imports support new method signatures.The added type imports are correctly used for the refactored
getShadersand newupdateStatemethod signatures.
26-26: Simplified fragment shader declaration aligns with UBO migration.The removal of explicit opacity and lineWidth uniform declarations is consistent with migrating to module-based uniforms.
51-80: UBO migration follows deck.gl patterns correctly.The refactored
getShadersmethod correctly implements the module-based UBO pattern:
- Exports
uniformTypesfor deck.gl to create the UBO- Declares the uniform block in the fragment shader
- Accesses uniforms via the module namespace (
scatterDensity.opacity)- Properly spreads
super.getShaders(extension)for compositionThis aligns with the deck.gl upgrade guide for uniform buffer migration.
Note: The UBO struct name includes the same typo as the class name.
81-91: No changes needed – opacity is always available and code is correct.The code is safe as-is. All deck.gl layers define default values for their props, and these values are used internally if not specified. The opacity prop defaults to 1, so line 83's destructuring (
const { opacity } = props) will always succeed.The suggested change detection via
changeFlagsis optional optimization, not required. updateState typically involves updating uniforms by calling model.shaderInputs.setProps, and the pattern of iteratinggetModels()and callingsetPropsmatches deck.gl conventions. No null checks or guards are needed.Likely an incorrect or invalid review comment.
1 participant
Migrating to the new way of setting
shaderInputsbased on Uniform Buffer Objects in deck.glhttps://deck.gl/docs/upgrade-guide#uniform-buffers
Also adding test-utils and fixing related bugs. WebGL tests need more setup config.
As of this writing, I haven't figured out how to make array uniforms work with the new way of working in deck.gl (I have some experimental code that has some of the data-structures in GLSL but haven't managed to bind appropriate properties in JS).
edit: having investigated array uniforms more thoroughly and asked about it on deck.gl slack, have confirmed that currently luma.gl doesn't have ways of representing such things. This is related to the work in hms-dbmi/viv#924
I think I'm going to merge changes here in a somewhat partial state, as there was previously very bad behaviour if a user selected non-default "Shape" in Viv scatter-plot, and this fixes that without any other likely regressions I can think of.
Summary by CodeRabbit
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.