refactor: update type definitions for component data interfaces#1436
Open
wumaolinmaoan wants to merge 3 commits intofeat/2.9from
Open
refactor: update type definitions for component data interfaces#1436wumaolinmaoan wants to merge 3 commits intofeat/2.9from
wumaolinmaoan wants to merge 3 commits intofeat/2.9from
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request strengthens type safety across the codebase by updating Changes
Sequence DiagramsequenceDiagram
participant Canvas as Canvas/Page
participant ImGui as ImGui Input
participant Hidden as Hidden IME Textarea
participant Composer as Composition Handler
Canvas->>Hidden: blur (trigger blur check)
alt blur target is IME textarea
Hidden-->>Canvas: skip input state reset
else normal blur
Hidden-->>ImGui: reset input state
end
Canvas->>Hidden: compositionstart
Hidden->>Composer: set is_composing = true
Canvas->>Hidden: input event (user types)
Hidden->>Composer: ime_flush()
Composer->>ImGui: AddInputCharacter(...) for each char
Hidden->>Composer: clear textarea
Canvas->>Hidden: compositionend
Composer->>ImGui: ime_flush() (finalize text)
Hidden->>Composer: set is_composing = false
ImGui->>Hidden: NewFrame() checks WantTextInput
alt WantTextInput = true
ImGui->>Hidden: ime_focus()
else WantTextInput = false
ImGui->>Canvas: return focus to canvas
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR refactors the ImGui demo Sequencer into a set of smaller render/utility modules, while also tightening fromData(...) type signatures across multiple core/plugin components to use spec-defined data interfaces. It additionally updates some editor UI panels (Hierarchy/Composition) and improves text input handling via IME support in the ImGui backend.
Changes:
- Split the legacy
Sequencerimplementation into a dedicatedsequencer/module with separate renderers (track labels, clips, keyframes, properties) plus shared theme/state/utilities. - Add IME support in
imgui_impl.tsusing a hidden<textarea>and composition event handling. - Replace multiple
fromData(any|unknown)signatures with spec-typed data interfaces across core, threejs/webgl, and plugin packages.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| web-packages/imgui-demo/src/panels/sequencer/types.ts | Adds sequencer data interfaces (keyframes, channels, groups, selection info). |
| web-packages/imgui-demo/src/panels/sequencer/track-label-renderer.ts | New left-panel track/property label renderer with selection/expand UX. |
| web-packages/imgui-demo/src/panels/sequencer/timeline-utils.ts | New timeline utilities (markers, time↔pixel, track colors, etc.). |
| web-packages/imgui-demo/src/panels/sequencer/theme.ts | Centralizes sequencer colors/layout constants. |
| web-packages/imgui-demo/src/panels/sequencer/sequencer.ts | New Sequencer window orchestrating renderers, layout, scrolling, and scrubbing. |
| web-packages/imgui-demo/src/panels/sequencer/sequencer-state.ts | Introduces shared state container for sequencer interactions and caches. |
| web-packages/imgui-demo/src/panels/sequencer/selection.ts | Adds selection/expansion helpers for tracks and keyframes. |
| web-packages/imgui-demo/src/panels/sequencer/properties-panel-renderer.ts | Adds right-side properties panel renderer for track/clip/keyframe details. |
| web-packages/imgui-demo/src/panels/sequencer/keyframe-renderer.ts | Implements keyframe rendering + selection + cursor line drawing. |
| web-packages/imgui-demo/src/panels/sequencer/index.ts | Exposes Sequencer entrypoint from the new module folder. |
| web-packages/imgui-demo/src/panels/sequencer/data-extraction.ts | Extracts transform animation expressions into keyframe/channel/group data. |
| web-packages/imgui-demo/src/panels/sequencer/clip-renderer.ts | Implements clip rendering, dragging, end-behavior extensions, and section markers. |
| web-packages/imgui-demo/src/panels/sequencer.ts | Removes the monolithic legacy sequencer implementation. |
| web-packages/imgui-demo/src/panels/hierarchy.ts | Adds per-item type icons and adjusts label padding for the Hierarchy panel. |
| web-packages/imgui-demo/src/panels/composition.ts | Redesigns Composition panel with search, custom list rows, and playing indicator. |
| web-packages/imgui-demo/src/imgui/imgui_impl.ts | Adds IME support via hidden textarea + composition/input event forwarding. |
| plugin-packages/orientation-transformer/src/orientation-component.ts | Types fromData input with a local spec-aligned interface. |
| plugin-packages/model/src/plugin/model-tree-component.ts | Types constructor/fromData input with a local spec-aligned interface. |
| plugin-packages/editor-gizmo/src/gizmo-component.ts | Types fromData input with a local spec-aligned interface and simplifies destructuring. |
| packages/effects-webgl/src/gl-texture.ts | Types fromData as spec.EffectsObjectData and adjusts casts for texture source options. |
| packages/effects-threejs/src/three-texture.ts | Types fromData as spec.EffectsObjectData and casts for texture creation. |
| packages/effects-threejs/src/material/three-material.ts | Narrows fromData signature to spec.MaterialData (still unimplemented). |
| packages/effects-core/src/plugins/timeline/tracks/property-track.ts | Updates fromData signature to TrackAssetData. |
| packages/effects-core/src/plugins/timeline/track.ts | Updates fromData signature to spec.TrackAssetData. |
| packages/effects-core/src/plugins/particle/particle-system.ts | Updates constructor/fromData typing to spec particle system data. |
| packages/effects-core/src/plugins/interact/interact-item.ts | Updates fromData typing to spec.InteractComponentData. |
| packages/effects-core/src/plugins/camera/camera-controller-node.ts | Updates constructor/fromData typing via a local spec-aligned interface. |
| packages/effects-core/src/components/mesh-component.ts | Updates fromData typing via a local spec-aligned interface for mask options. |
| packages/effects-core/src/components/frame-component.ts | Updates fromData typing via a local spec-aligned interface for color data. |
| packages/effects-core/src/components/composition-component.ts | Updates fromData signature to spec.CompositionComponentData. |
| packages/effects-core/src/components/component.ts | Updates base Component.fromData signature to spec.ComponentData. |
| packages/effects-core/src/components/base-render-component.ts | Tightens fromData typing and simplifies local variable usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const io = ImGui.GetIO(); | ||
|
|
||
| for (const ch of text) { | ||
| io.AddInputCharacter(ch.codePointAt(0)); |
There was a problem hiding this comment.
ch.codePointAt(0) has type number | undefined under strictNullChecks, but io.AddInputCharacter(...) expects a number. This will fail type-checking (and could pass undefined at runtime in edge cases). Capture the code point into a variable, guard against undefined, and only then call AddInputCharacter (or use codePointAt(0) ?? 0 if a fallback is acceptable).
Suggested change
| io.AddInputCharacter(ch.codePointAt(0)); | |
| const codePoint = ch.codePointAt(0); | |
| if (codePoint !== undefined) { | |
| io.AddInputCharacter(codePoint); | |
| } |
Comment on lines
21
to
25
| const LAYOUT = { | ||
| visibilityColumnWidth: 16, | ||
| visibilitySpacing: 6, | ||
| iconPadding: ' ', | ||
| } as const; |
There was a problem hiding this comment.
Using a hard-coded string of spaces (iconPadding: ' ') to make room for the type icon is font-dependent and will misalign under different fonts/DPI or when the label is translated. Prefer reserving horizontal space via layout (e.g., ImGui.Dummy/ImGui.SameLine with a fixed pixel width, adjusting cursor X before TreeNodeEx, or drawing text separately) so the icon/text spacing is stable.
Comment on lines
+7
to
+18
| // Try to find the ActivationTrack or any track with clips to select its first clip | ||
| let targetTrackToSelectClip = track; | ||
| let clips = typeof track.getClips === 'function' ? track.getClips() : []; | ||
|
|
||
| if (!clips || clips.length === 0) { | ||
| for (const child of track.getChildTracks()) { | ||
| if (child.constructor.name === 'ActivationTrack' || child.constructor.name.includes('Activation')) { | ||
| const childClips = typeof child.getClips === 'function' ? child.getClips() : []; | ||
|
|
||
| if (childClips && childClips.length > 0) { | ||
| targetTrackToSelectClip = child; | ||
| clips = childClips; |
There was a problem hiding this comment.
selectTrack detects Activation tracks via child.constructor.name string matching. This is brittle (minification/obfuscation can change names) and inconsistent with other sequencer code that uses instanceof ActivationTrack. Import ActivationTrack here and switch to child instanceof ActivationTrack (or a capability check) to make selection reliable.
Comment on lines
+86
to
+94
| // 重叠检测 | ||
| const overlapSet = new Set<number>(); | ||
|
|
||
| for (let i = 0; i < keyframePositions.length; i++) { | ||
| for (let j = i + 1; j < keyframePositions.length; j++) { | ||
| if (Math.abs(keyframePositions[i].x - keyframePositions[j].x) <= LAYOUT.keyOverlapThresholdPx) { | ||
| overlapSet.add(keyframePositions[i].index); | ||
| overlapSet.add(keyframePositions[j].index); | ||
| } |
There was a problem hiding this comment.
Keyframe overlap detection is currently O(n²) (nested loops over keyframePositions). With many keyframes this will become a noticeable per-frame cost. Consider sorting keyframes by x once and only comparing adjacent (or within a sliding window) positions to find overlaps in O(n log n) / O(n).
Suggested change
| // 重叠检测 | |
| const overlapSet = new Set<number>(); | |
| for (let i = 0; i < keyframePositions.length; i++) { | |
| for (let j = i + 1; j < keyframePositions.length; j++) { | |
| if (Math.abs(keyframePositions[i].x - keyframePositions[j].x) <= LAYOUT.keyOverlapThresholdPx) { | |
| overlapSet.add(keyframePositions[i].index); | |
| overlapSet.add(keyframePositions[j].index); | |
| } | |
| // 重叠检测（按 x 排序后用滑动窗口在线性时间内检测） | |
| const overlapSet = new Set<number>(); | |
| if (keyframePositions.length > 1) { | |
| const sortedPositions = [...keyframePositions].sort((a, b) => a.x - b.x); | |
| const threshold = LAYOUT.keyOverlapThresholdPx; | |
| let windowStart = 0; | |
| for (let i = 1; i < sortedPositions.length; i++) { | |
| const currentX = sortedPositions[i].x; | |
| while (windowStart < i && currentX - sortedPositions[windowStart].x > threshold) { | |
| windowStart++; | |
| } | |
| for (let j = windowStart; j < i; j++) { | |
| if (currentX - sortedPositions[j].x <= threshold) { | |
| overlapSet.add(sortedPositions[i].index); | |
| overlapSet.add(sortedPositions[j].index); | |
| } | |
| } |
Comment on lines
+17
to
+21
| const state = this.state; | ||
| const isTransformTrack = trackAsset instanceof TransformTrack; | ||
| const hasKeyframeData = isTransformTrack && getTransformPropertyGroups(trackAsset).length > 0; | ||
| const hasChildren = trackAsset.getChildTracks().length > 0 || hasKeyframeData; | ||
| const frameHeight = LAYOUT.sectionHeight; |
There was a problem hiding this comment.
getTransformPropertyGroups(trackAsset) is called during label rendering just to compute hasKeyframeData, and then called again later when the track is expanded (and also from the clips renderer). Since this runs every frame, it can become expensive for timelines with many clips. Consider caching extracted property groups per track/clip in SequencerState (invalidate when composition or clip data changes) and reuse the cached result for hasKeyframeData + expanded rendering.
Comment on lines
+24
to
+28
| const state = this.state; | ||
| const isTransformTrack = trackAsset instanceof TransformTrack; | ||
| const hasKeyframeData = isTransformTrack && getTransformPropertyGroups(trackAsset).length > 0; | ||
| const hasChildren = trackAsset.getChildTracks().length > 0 || hasKeyframeData; | ||
| const frameHeight = LAYOUT.sectionHeight; |
There was a problem hiding this comment.
getTransformPropertyGroups(trackAsset) is executed every frame to compute hasKeyframeData, and then executed again when the track is expanded to actually render channels. This repeats relatively heavy extraction work per frame. Consider memoizing results (e.g., in SequencerState) so the extraction runs only when track/clip data changes.
Comment on lines
+363
to
+366
| // Section 级关键帧菱形标记 | ||
| const aggregatedTimes = getClipAggregatedKeyframeTimes(clip); | ||
|
|
||
| if (aggregatedTimes.length > 0) { |
There was a problem hiding this comment.
getClipAggregatedKeyframeTimes(clip) is called inside the per-frame clip render loop. This re-parses transform animation expressions every frame and can become a hotspot with many clips/keyframes. Consider caching normalized keyframe times per clip asset and only recomputing the absolute times when clip.start / clip.duration changes (or cache by clip id + start/duration).
| Selection.select(null); | ||
| void GalaceanEffects.playURL(this.currentCompositionURL); | ||
| if (listIndex !== undefined) { | ||
| this.playingIndex = listIndex; |
There was a problem hiding this comment.
When playing a URL via the input box (playURL(url) without listIndex), playingIndex is left unchanged, so the UI can keep highlighting a stale “playing” row that no longer corresponds to the active composition. Consider setting playingIndex = -1 (and possibly currentItem = -1) when listIndex is undefined.
Suggested change
| this.playingIndex = listIndex; | |
| this.playingIndex = listIndex; | |
| } else { | |
| this.playingIndex = -1; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin-packages/editor-gizmo/src/gizmo-component.ts (1)
592-594:⚠️ Potential issue | 🟠 MajorGuard optional
transformbefore reinitializingitem.transform.
transformis optional inGizmoComponentData, but this branch always runs and can reset transform when the field is absent.Proposed fix
- if (data) { - item.transform = new Transform(data.transform); - } + if (data.transform) { + item.transform = new Transform(data.transform); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin-packages/editor-gizmo/src/gizmo-component.ts` around lines 592 - 594, The code checks only `data` and then always overwrites `item.transform`, but `GizmoComponentData.transform` is optional so this can unintentionally reset transforms; update the logic in the block that touches `item.transform` (where `item.transform = new Transform(data.transform)` is set) to guard on the presence of `data.transform` (e.g., only construct a new `Transform` and assign to `item.transform` when `data.transform` is non-null/defined), leaving `item.transform` unchanged when the optional field is absent.
🧹 Nitpick comments (6)
packages/effects-core/src/plugins/particle/particle-system.ts (1)
120-121: Consider whetherParticleSystemPropsis still needed.This empty interface that extends
spec.ParticleContentis no longer used for constructor/fromDataparameters. Ifspec.ParticleSystemDataprovides the same shape, consider updatingthis.props(line 137) to usespec.ParticleSystemDatadirectly and removing this interface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects-core/src/plugins/particle/particle-system.ts` around lines 120 - 121, The empty ParticleSystemProps interface that only extends spec.ParticleContent is redundant; change usages to use spec.ParticleSystemData directly (update the type of this.props in the ParticleSystem class and any constructor/fromData signatures to spec.ParticleSystemData), then remove the ParticleSystemProps declaration and any now-unused imports/exports; make sure to update any references to ParticleSystemProps to the new spec.ParticleSystemData type (e.g., in the class field this.props, the constructor, and fromData).packages/effects-threejs/src/material/three-material.ts (1)
521-521: KeepfromDataoverride aligned with the base contract.At line 521, narrowing to
spec.MaterialDatanarrows the override parameter beyond the base contract inEffectsObject.fromData(data: spec.EffectsObjectData). While the method is currently stubbed and throws immediately, preferring the base parameter type maintains substitutability and avoids potential issues if an implementation is added later.♻️ Suggested change
- override fromData (data: spec.MaterialData): void { + override fromData (data: spec.EffectsObjectData): void { //FIXME: 暂时不实现 throw new Error('Method not implemented.'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects-threejs/src/material/three-material.ts` at line 521, The override in fromData currently narrows the parameter to spec.MaterialData; change the signature to match the base contract by using spec.EffectsObjectData (i.e., override fromData(data: spec.EffectsObjectData): void) so it remains substitutable with EffectsObject.fromData, and keep the current stub/throw implementation unchanged inside the method.packages/effects-webgl/src/gl-texture.ts (1)
40-40: Extract private helper to reduceunknowncasting in texture initialization.The constructor and
fromDatamethod use round-trip casting withunknownto bridge incompatible types:TextureSourceOptionsvs.spec.EffectsObjectData. While the current code functions, extracting a privateapplySourceOptions(source: TextureSourceOptions)helper reduces type-safety gaps. Note thatfromDatamust maintain its base class signature, so casting within it cannot be fully eliminated.♻️ Suggested refactor
constructor (engine: Engine, source?: TextureSourceOptions) { super(engine); if (source) { - this.fromData(source as unknown as spec.EffectsObjectData); + this.applySourceOptions(source); } } @@ override fromData (data: spec.EffectsObjectData): void { super.fromData(data); - const source = data as unknown as TextureSourceOptions; - const options = this.assembleOptions(source); + this.applySourceOptions(data as unknown as TextureSourceOptions); + } + + private applySourceOptions (source: TextureSourceOptions): void { + const options = this.assembleOptions(source); const { sourceType, sourceFrom, name = '' } = options; this.source = options; this.sourceType = sourceType; this.sourceFrom = sourceFrom; this.name = name; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects-webgl/src/gl-texture.ts` at line 40, Extract a private helper applySourceOptions(source: TextureSourceOptions) and move the logic currently performed via this.fromData(source as unknown as spec.EffectsObjectData) into that helper to avoid the unknown round-trip in the constructor; keep fromData(specData: spec.EffectsObjectData) with its original signature but have it call applySourceOptions when the incoming data originates from TextureSourceOptions (use casting only inside fromData to satisfy the base signature), and update the constructor to call applySourceOptions(source) directly. Ensure references to TextureSourceOptions, fromData, constructor, and the new applySourceOptions are updated so all initialization logic lives in applySourceOptions and fromData only performs the required signature-preserving cast before delegating.web-packages/imgui-demo/src/panels/hierarchy.ts (1)
24-24: Use one metric-driven icon gutter instead of mixing spaces and pixels.Line 24 reserves space with three characters, while Lines 631-858 place and size the icon with fixed pixel values. Those two systems will drift under font/DPI scaling, so the icon can end up too small or too close to the label. Derive both the gutter and the icon geometry from
rowHeight/ImGui.GetFontSize()so the tree and search rows stay aligned.Also applies to: 351-352, 467-467, 631-858
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-packages/imgui-demo/src/panels/hierarchy.ts` at line 24, The code uses a hardcoded string ' ' for iconPadding and fixed-pixel geometry elsewhere causing misalignment under font/DPI scaling; replace the string constant with a metric-driven gutter computed from rowHeight or ImGui.GetFontSize() (e.g., gutter = Math.round(rowHeight * 0.8) or based on fontSize), then derive iconSize, iconX and iconY from that same value (iconSize = Math.min(gutter, rowHeight - padding), iconX = indent + gutter/2 - iconSize/2, iconY = y + (rowHeight - iconSize)/2) and use those computed values everywhere currently using 'iconPadding' or fixed pixel constants (references: iconPadding, rowHeight, ImGui.GetFontSize(), and the locations where icon placement/size is hardcoded) so tree rows and search rows stay aligned under scaling.web-packages/imgui-demo/src/panels/sequencer/sequencer-state.ts (1)
57-59: Definite assignment assertion may cause runtime errors.
currentComposition!: Compositionassumes the field will be initialized before use, but there's no constructor or initialization logic in this class. If accessed before assignment, this will cause a runtime error.Consider making it nullable with explicit null checks, or requiring it in a constructor:
♻️ Option 1: Make nullable
- currentComposition!: Composition; + currentComposition: Composition | null = null;♻️ Option 2: Require in constructor
+ constructor(composition: Composition) { + this.currentComposition = composition; + } + - currentComposition!: Composition; + currentComposition: Composition;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-packages/imgui-demo/src/panels/sequencer/sequencer-state.ts` around lines 57 - 59, The field currentComposition in the SequencerState class uses a definite assignment assertion (currentComposition!: Composition) which can yield runtime errors if accessed before being set; either (A) make it nullable (currentComposition: Composition | null) and add explicit null checks where used (or guard methods like getCurrentComposition() that throw meaningful errors), or (B) require it via a constructor parameter and assign it there (add a constructor to SequencerState that accepts a Composition and sets this.currentComposition), and update any call sites to provide the composition accordingly so the property is always initialized.web-packages/imgui-demo/src/panels/sequencer/data-extraction.ts (1)
113-127: Use named constants instead of magic numbers for value types.The magic numbers
21(BEZIER_CURVE),5(LINE),7(BEZIER_PATH),12(LINEAR_PATH), and22(BEZIER_CURVE_PATH) are scattered across this file (lines 115, 207, 221, 253). While inline comments identify them, usingspec.ValueTypeenum members would improve code readability and maintainability.♻️ Proposed refactor to use named constants
+// Value type constants from spec +const VALUE_TYPE = { + LINE: 5, + BEZIER_CURVE: 21, + BEZIER_PATH: 7, + LINEAR_PATH: 12, + BEZIER_CURVE_PATH: 22, +} as const; + const collectFromExpression = (expr: spec.FixedNumberExpression | undefined) => { if (!expr || !Array.isArray(expr)) { return; } const type = expr[0] as number; - if (type === 21 || type === 5) { // BEZIER_CURVE or LINEAR + if (type === VALUE_TYPE.BEZIER_CURVE || type === VALUE_TYPE.LINE) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-packages/imgui-demo/src/panels/sequencer/data-extraction.ts` around lines 113 - 127, Replace the magic numeric value checks on the expression type with the corresponding spec.ValueType enum members: instead of 21 and 5 use spec.ValueType.BEZIER_CURVE and spec.ValueType.LINEAR (and likewise replace 7→spec.ValueType.BEZIER_PATH, 12→spec.ValueType.LINEAR_PATH, 22→spec.ValueType.BEZIER_CURVE_PATH) wherever the file checks expression types (e.g., the local variable type derived from expr[0] in the data-extraction logic that adds times to timeSet using clipStart and clipDuration). Update all occurrences at the referenced locations (lines around the shown block and the other spots at ~207, ~221, ~253) to use these named enum members to improve readability and maintainability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/effects-threejs/src/three-texture.ts`:
- Around line 129-133: fromData currently reassigns this.texture via
createTextureByType and sets needsUpdate without disposing the previous THREE
texture, causing GPU leaks; modify fromData (the override method) to check for
an existing this.texture and call its dispose (matching the pattern used in
updateSource) before assigning a new texture from createTextureByType, then set
this.texture.needsUpdate = true.
In `@web-packages/imgui-demo/src/imgui/imgui_impl.ts`:
- Around line 171-194: The IME textarea can keep focus and leave ImGui key state
latched when the window or the textarea blurs; update ime_create to attach a
blur handler on ime_input (and also listen for window blur) that calls the same
reset path as canvas_on_blur (so it clears io.KeysDown and modifier flags) —
e.g., add ime_input.addEventListener('blur', ...) and
window.addEventListener('blur', ...) that invoke canvas_on_blur (or the existing
key-reset routine) to ensure key state is cleared when focus is lost.
- Around line 204-208: The ime_focus() routine currently forces focus back to
ime_input whenever io.WantTextInput is true, which yanks focus from unrelated
DOM controls; change ime_focus (and the same check at the other site around
lines 455-460) to only call ime_input.focus() when the current
document.activeElement is either already ime_input, the canvas element used by
ImGui, or a non-focusable element (e.g., document.body/null) — do not focus when
activeElement is some other input/control; use the ImGui input intent flag
io.WantTextInput and the existing ime_input and NewFrame call sites to locate
and apply this conditional guard so unrelated inputs keep their focus.
In `@web-packages/imgui-demo/src/panels/composition.ts`:
- Around line 81-93: The URL-input Enter handler in drawURLSection triggers
playURL(this.currentCompositionURL) but doesn't clear any previously selected
row, leaving stale selection; update the manual-launch code path (the
ImGui.InputText Enter handlers in drawURLSection and the similar blocks at the
other mentioned spots) to call Selection.select(null) when launching a URL
without a list index, or alternatively modify playURL to accept an optional
index and perform Selection.select(null) when index is undefined; target the
ImGui.InputText Enter handlers and the playURL method (and ensure
currentCompositionURL is passed consistently) so both manual and list launches
clear/maintain selection state the same way.
In `@web-packages/imgui-demo/src/panels/hierarchy.ts`:
- Line 630: The drawItemTypeIcon helper currently types its drawList parameter
as any which disables type checking for calls like
AddRectFilled/AddCircleFilled; change the parameter type to ImGui.ImDrawList in
the function signature of drawItemTypeIcon so all draw-list method calls are
statically typed (update "private drawItemTypeIcon (drawList: any, itemType:
spec.ItemType, rowRectMin: ImGui.Vec2, rowHeight: number): void" to use
ImGui.ImDrawList) and ensure any uses of drawItemTypeIcon pass an
ImDrawList-compatible object.
In `@web-packages/imgui-demo/src/panels/sequencer/clip-renderer.ts`:
- Around line 252-270: The clip drag code mutates TimelineClip.start/duration
(variables: clip, state.clipDragMode, pixelToTime, minClipDuration) but never
updates the composition/timeline length used by sequencer.ts (timelineEndTime),
so the ruler/extension range can become stale; after applying the clip changes
and resetting the mouse drag delta (i.e., inside the if (state.clipDragMode !==
'none') block, after ImGui.ResetMouseDragDelta(0)), call the existing routine
that recomputes the composition/timeline length (or emit the composition-change
event) — e.g., invoke the project's function that recalculates the composition
duration (named similarly to
recomputeCompositionDuration/updateCompositionDuration/getCompositionDuration)
or trigger the same update flow sequencer.ts uses to refresh timelineEndTime so
the UI reflects the edited clip immediately.
In `@web-packages/imgui-demo/src/panels/sequencer/data-extraction.ts`:
- Around line 258-277: The loop that extracts `time` from `easingEntry` can
throw when `easingEntry` is an empty array; update the logic in the loop that
processes `easing` (the variables `easingEntry`, `points`, `xKeyframes`,
`yKeyframes`, `zKeyframes`) to defensively check array lengths before indexing:
when `Array.isArray(easingEntry)` ensure `easingEntry.length > 0` before reading
`[0]`, and if you access `easingEntry[1]` confirm `easingEntry.length > 1` and
that it is an array with length > 0 before reading `[0]`; if checks fail, skip
this entry so you only push keyframes when a valid `time` is obtained and
`points[i]` is a valid 3-element array.
In `@web-packages/imgui-demo/src/panels/sequencer/keyframe-renderer.ts`:
- Around line 75-107: keyframePositions is built from channel.keyframes but
never sorted, so later code that assumes increasing x (e.g., the overlap
detection loop and the KeyBar loop computing barLength = endX - startX) can
produce negative lengths or wrong neighbor connections; fix by sorting
keyframePositions by their x (pixelX) or by absoluteTime after populating the
array and before creating overlapSet and drawing KeyBar (references:
keyframePositions, overlapSet, channel.keyframes, timeToPixel, KeyBar loop that
computes barLength) so rendering and hover precedence are deterministic even if
source keyframes are out of order.
In `@web-packages/imgui-demo/src/panels/sequencer/selection.ts`:
- Around line 12-23: The loop is using fragile constructor.name checks which
break after minification; replace those checks with a proper instanceof check
against ActivationTrack: import ActivationTrack from '@galacean/effects' (or the
correct exported symbol) and change the condition to use (child instanceof
ActivationTrack) while keeping the existing fallback for child.getClips (i.e.,
call typeof child.getClips === 'function' ? child.getClips() : []), then set
targetTrackToSelectClip and clips and break as before.
In `@web-packages/imgui-demo/src/panels/sequencer/sequencer-state.ts`:
- Line 33: Replace the loose any typing on the sequencer state property
selectedClip with the concrete TimelineClip type to restore type safety: import
TimelineClip from the internal path
`@galacean/effects/dist/plugins/timeline/track` and change the declaration of
selectedClip to use TimelineClip | null (used elsewhere, e.g.
properties-panel-renderer.ts reads properties like endBehavior), ensuring the
new import is added to the top of sequencer-state.ts and the initialization
remains null.
In `@web-packages/imgui-demo/src/panels/sequencer/sequencer.ts`:
- Around line 50-76: When you detect and swap to the selected
CompositionComponent (after the block where compositionComponent is reassigned
based on selectedObject instanceof VFXItem and
getComponent(CompositionComponent)), also update state.currentComposition and
the local currentComposition to match that component's composition; then
recompute compositionId (and clear state.trackColorMap / set
state.lastCompositionId if needed) and set state.timelineEndTime from the new
currentComposition.getDuration() if present. In short: after the
compositionComponent reassignment, set state.currentComposition =
compositionComponent.ownerComposition (or derive the Composition instance from
compositionComponent), update the compositionId handling and timelineEndTime
using that updated currentComposition so the tracks, ruler, scrubbing and
transport use the same composition.
In `@web-packages/imgui-demo/src/panels/sequencer/track-label-renderer.ts`:
- Around line 369-380: The hover brightening currently extracts RGBA components
from the 32-bit color with R at bits 24-31 etc., but ImGui uses ABGR ordering
for GetColorU32; update the extraction in the isHovered block to read A = (color
>> 24) & 0xFF, B = (color >> 16) & 0xFF, G = (color >> 8) & 0xFF, R = color &
0xFF, normalize each to 0..1, brighten the RGB channels
(brightR/brightG/brightB) and then call ImGui.GetColorU32(new
ImGui.Vec4(brightR, brightG, brightB, a)) to compute drawColor so hovering
brightens the correct channels for the variables color, drawColor, and the
ImGui.GetColorU32/ImGui.Vec4 usage in track-label-renderer.ts.
---
Outside diff comments:
In `@plugin-packages/editor-gizmo/src/gizmo-component.ts`:
- Around line 592-594: The code checks only `data` and then always overwrites
`item.transform`, but `GizmoComponentData.transform` is optional so this can
unintentionally reset transforms; update the logic in the block that touches
`item.transform` (where `item.transform = new Transform(data.transform)` is set)
to guard on the presence of `data.transform` (e.g., only construct a new
`Transform` and assign to `item.transform` when `data.transform` is
non-null/defined), leaving `item.transform` unchanged when the optional field is
absent.
---
Nitpick comments:
In `@packages/effects-core/src/plugins/particle/particle-system.ts`:
- Around line 120-121: The empty ParticleSystemProps interface that only extends
spec.ParticleContent is redundant; change usages to use spec.ParticleSystemData
directly (update the type of this.props in the ParticleSystem class and any
constructor/fromData signatures to spec.ParticleSystemData), then remove the
ParticleSystemProps declaration and any now-unused imports/exports; make sure to
update any references to ParticleSystemProps to the new spec.ParticleSystemData
type (e.g., in the class field this.props, the constructor, and fromData).
In `@packages/effects-threejs/src/material/three-material.ts`:
- Line 521: The override in fromData currently narrows the parameter to
spec.MaterialData; change the signature to match the base contract by using
spec.EffectsObjectData (i.e., override fromData(data: spec.EffectsObjectData):
void) so it remains substitutable with EffectsObject.fromData, and keep the
current stub/throw implementation unchanged inside the method.
In `@packages/effects-webgl/src/gl-texture.ts`:
- Line 40: Extract a private helper applySourceOptions(source:
TextureSourceOptions) and move the logic currently performed via
this.fromData(source as unknown as spec.EffectsObjectData) into that helper to
avoid the unknown round-trip in the constructor; keep fromData(specData:
spec.EffectsObjectData) with its original signature but have it call
applySourceOptions when the incoming data originates from TextureSourceOptions
(use casting only inside fromData to satisfy the base signature), and update the
constructor to call applySourceOptions(source) directly. Ensure references to
TextureSourceOptions, fromData, constructor, and the new applySourceOptions are
updated so all initialization logic lives in applySourceOptions and fromData
only performs the required signature-preserving cast before delegating.
In `@web-packages/imgui-demo/src/panels/hierarchy.ts`:
- Line 24: The code uses a hardcoded string ' ' for iconPadding and
fixed-pixel geometry elsewhere causing misalignment under font/DPI scaling;
replace the string constant with a metric-driven gutter computed from rowHeight
or ImGui.GetFontSize() (e.g., gutter = Math.round(rowHeight * 0.8) or based on
fontSize), then derive iconSize, iconX and iconY from that same value (iconSize
= Math.min(gutter, rowHeight - padding), iconX = indent + gutter/2 - iconSize/2,
iconY = y + (rowHeight - iconSize)/2) and use those computed values everywhere
currently using 'iconPadding' or fixed pixel constants (references: iconPadding,
rowHeight, ImGui.GetFontSize(), and the locations where icon placement/size is
hardcoded) so tree rows and search rows stay aligned under scaling.
In `@web-packages/imgui-demo/src/panels/sequencer/data-extraction.ts`:
- Around line 113-127: Replace the magic numeric value checks on the expression
type with the corresponding spec.ValueType enum members: instead of 21 and 5 use
spec.ValueType.BEZIER_CURVE and spec.ValueType.LINEAR (and likewise replace
7→spec.ValueType.BEZIER_PATH, 12→spec.ValueType.LINEAR_PATH,
22→spec.ValueType.BEZIER_CURVE_PATH) wherever the file checks expression types
(e.g., the local variable type derived from expr[0] in the data-extraction logic
that adds times to timeSet using clipStart and clipDuration). Update all
occurrences at the referenced locations (lines around the shown block and the
other spots at ~207, ~221, ~253) to use these named enum members to improve
readability and maintainability.
In `@web-packages/imgui-demo/src/panels/sequencer/sequencer-state.ts`:
- Around line 57-59: The field currentComposition in the SequencerState class
uses a definite assignment assertion (currentComposition!: Composition) which
can yield runtime errors if accessed before being set; either (A) make it
nullable (currentComposition: Composition | null) and add explicit null checks
where used (or guard methods like getCurrentComposition() that throw meaningful
errors), or (B) require it via a constructor parameter and assign it there (add
a constructor to SequencerState that accepts a Composition and sets
this.currentComposition), and update any call sites to provide the composition
accordingly so the property is always initialized.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9a33f060-1050-41d4-83fd-802fc1b91de0
📒 Files selected for processing (32)
packages/effects-core/src/components/base-render-component.tspackages/effects-core/src/components/component.tspackages/effects-core/src/components/composition-component.tspackages/effects-core/src/components/frame-component.tspackages/effects-core/src/components/mesh-component.tspackages/effects-core/src/plugins/camera/camera-controller-node.tspackages/effects-core/src/plugins/interact/interact-item.tspackages/effects-core/src/plugins/particle/particle-system.tspackages/effects-core/src/plugins/timeline/track.tspackages/effects-core/src/plugins/timeline/tracks/property-track.tspackages/effects-threejs/src/material/three-material.tspackages/effects-threejs/src/three-texture.tspackages/effects-webgl/src/gl-texture.tsplugin-packages/editor-gizmo/src/gizmo-component.tsplugin-packages/model/src/plugin/model-tree-component.tsplugin-packages/orientation-transformer/src/orientation-component.tsweb-packages/imgui-demo/src/imgui/imgui_impl.tsweb-packages/imgui-demo/src/panels/composition.tsweb-packages/imgui-demo/src/panels/hierarchy.tsweb-packages/imgui-demo/src/panels/sequencer.tsweb-packages/imgui-demo/src/panels/sequencer/clip-renderer.tsweb-packages/imgui-demo/src/panels/sequencer/data-extraction.tsweb-packages/imgui-demo/src/panels/sequencer/index.tsweb-packages/imgui-demo/src/panels/sequencer/keyframe-renderer.tsweb-packages/imgui-demo/src/panels/sequencer/properties-panel-renderer.tsweb-packages/imgui-demo/src/panels/sequencer/selection.tsweb-packages/imgui-demo/src/panels/sequencer/sequencer-state.tsweb-packages/imgui-demo/src/panels/sequencer/sequencer.tsweb-packages/imgui-demo/src/panels/sequencer/theme.tsweb-packages/imgui-demo/src/panels/sequencer/timeline-utils.tsweb-packages/imgui-demo/src/panels/sequencer/track-label-renderer.tsweb-packages/imgui-demo/src/panels/sequencer/types.ts
💤 Files with no reviewable changes (1)
- web-packages/imgui-demo/src/panels/sequencer.ts
Comment on lines
+129
to
133
| override fromData (data: spec.EffectsObjectData): void { | ||
| super.fromData(data); | ||
|
|
||
| this.texture = this.createTextureByType(data); | ||
| this.texture = this.createTextureByType(data as unknown as TextureSourceOptions); | ||
| this.texture.needsUpdate = true; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify lifecycle paths and current disposal behavior around ThreeTexture.fromData.
rg -n --type=ts -C3 'new ThreeTexture\(' packages/effects-threejs
rg -n --type=ts -C3 '\.fromData\(' packages/effects-threejs
rg -n --type=ts -C3 'updateSource\s*\(|fromData\s*\(|this\.texture\.dispose\(' packages/effects-threejs/src/three-texture.tsRepository: galacean/effects-runtime
Length of output: 3515
🏁 Script executed:
#!/bin/bash
# 1. Check constructor and initialization
echo "=== Constructor and initialization ==="
rg -n --type=ts -A20 'class ThreeTexture' packages/effects-threejs/src/three-texture.ts | head -40
# 2. Check if fromData is called repeatedly or once
echo -e "\n=== fromData usage patterns ==="
rg -n --type=ts 'fromData\(' packages/effects-threejs --max-count=20
# 3. Look for texture property initialization
echo -e "\n=== Texture property declaration and initialization ==="
rg -n --type=ts 'texture\s*[:=]|this\.texture\s*=' packages/effects-threejs/src/three-texture.ts
# 4. Check parent class to understand the contract
echo -e "\n=== Check base class structure ==="
rg -n --type=ts 'class Texture|fromData' packages/effects-core/src/texture.ts | head -30Repository: galacean/effects-runtime
Length of output: 1772
🏁 Script executed:
#!/bin/bash
# 1. Get full constructor
echo "=== Constructor (full) ==="
rg -n --type=ts -B5 -A30 'constructor\s*\(' packages/effects-threejs/src/three-texture.ts
# 2. Find parent class location
echo -e "\n=== Find parent Texture class ==="
fd -t f '\.ts$' packages/effects-core | xargs rg -l 'export class Texture' | head -5
# 3. Check parent class fromData method
echo -e "\n=== Parent class fromData ==="
fd -t f '\.ts$' packages/effects-core | xargs rg -B3 -A10 'fromData\s*\(' | grep -A10 'class Texture' | head -30Repository: galacean/effects-runtime
Length of output: 1208
🏁 Script executed:
#!/bin/bash
# 1. Find parent Texture class definition
echo "=== Parent Texture class definition ==="
rg -n --type=ts 'export class Texture' packages/effects-core/src
# 2. Check parent class fromData method
echo -e "\n=== Parent fromData in Texture class ==="
rg -n --type=ts -B2 -A15 'class Texture\b' packages/effects-core/src/texture/*.ts | grep -A20 'fromData'
# 3. Search for repeated fromData calls on texture objects to understand lifecycle
echo -e "\n=== Check if fromData is called multiple times ==="
rg -n --type=ts -B5 -A5 'fromData.*TextureSourceOptions|\.fromData\(.*data.*\)' packages/effects-threejs
# 4. Check if components/layers reuse texture instances
echo -e "\n=== Layer/component lifecycle ==="
rg -n --type=ts 'class.*Layer|class.*Component' packages/effects-threejs/src | head -10Repository: galacean/effects-runtime
Length of output: 3381
🏁 Script executed:
#!/bin/bash
# 1. Check imports in three-texture.ts to find parent Texture
echo "=== Imports in three-texture.ts ==="
head -30 packages/effects-threejs/src/three-texture.ts | rg 'import|from'
# 2. Find Texture class in effects-core
echo -e "\n=== Search for Texture base class ==="
fd -t f '\.ts$' packages/effects-core/src | xargs rg -l 'export class Texture\b' 2>/dev/null
# 3. Check the actual parent class and its fromData method
echo -e "\n=== Texture base class fromData ==="
fd -t f '\.ts$' packages/effects-core | xargs rg -B5 -A15 'export class Texture\b' | head -60Repository: galacean/effects-runtime
Length of output: 338
🏁 Script executed:
#!/bin/bash
# 1. Search more broadly for Texture in effects-core
echo "=== Search for Texture class definition ==="
rg -n 'class Texture' packages/effects-core/src --type=ts | head -20
# 2. Check index file for exports
echo -e "\n=== Texture exports in effects-core ==="
rg -n 'Texture' packages/effects-core/src/index.ts | head -20
# 3. Find the actual file
echo -e "\n=== Locate texture.ts files ==="
fd 'texture' packages/effects-core/src --type=f | grep -v node_modules
# 4. Check one of these files for Texture class
echo -e "\n=== Check main texture file ==="
rg -n -B2 -A20 'export.*class Texture\b' packages/effects-core/src/texture.ts 2>/dev/null || rg -n 'class Texture' packages/effects-core/src/texture.ts 2>/dev/nullRepository: galacean/effects-runtime
Length of output: 835
🏁 Script executed:
#!/bin/bash
# 1. Check parent Texture class and its fromData method
echo "=== Parent Texture class fromData ==="
rg -n -B3 -A15 'fromData' packages/effects-core/src/texture/texture.ts
# 2. Check if Texture.texture is initialized elsewhere
echo -e "\n=== Texture property in base class ==="
rg -n 'texture\s*:' packages/effects-core/src/texture/texture.ts
# 3. Check the full Texture class structure
echo -e "\n=== Full Texture class structure (first 50 lines) ==="
head -80 packages/effects-core/src/texture/texture.tsRepository: galacean/effects-runtime
Length of output: 1944
🏁 Script executed:
#!/bin/bash
# 1. Check if fromData exists in parent Texture class
echo "=== Full search for fromData in texture.ts ==="
rg -n 'fromData' packages/effects-core/src/texture/texture.ts
# 2. Check EffectsObject for fromData
echo -e "\n=== fromData in EffectsObject ==="
rg -n -B3 -A10 'fromData' packages/effects-core/src/effects-object.ts
# 3. Check if texture instance can be reused across multiple fromData calls
echo -e "\n=== Check scene/layer structure for texture reuse ==="
rg -n 'new ThreeTexture\|\.texture\s*=' packages/effects-threejs/src/three-texture.ts
# 4. Check if components maintain texture across updates
echo -e "\n=== Component texture lifecycle ==="
rg -n -B5 -A5 'texture.*fromData\|fromData.*texture' packages/effects-threejs/srcRepository: galacean/effects-runtime
Length of output: 528
Dispose previous texture before reassigning in fromData.
At lines 132-133, this.texture is reassigned without disposing the old instance. Since updateSource() properly disposes before reassignment (line 77), fromData() should follow the same pattern to prevent GPU memory leaks on repeated calls.
🛠️ Suggested fix
override fromData (data: spec.EffectsObjectData): void {
super.fromData(data);
- this.texture = this.createTextureByType(data as unknown as TextureSourceOptions);
+ const nextTexture = this.createTextureByType(data as unknown as TextureSourceOptions);
+ this.texture?.dispose();
this.texture = nextTexture;
this.texture.needsUpdate = true;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/effects-threejs/src/three-texture.ts` around lines 129 - 133,
fromData currently reassigns this.texture via createTextureByType and sets
needsUpdate without disposing the previous THREE texture, causing GPU leaks;
modify fromData (the override method) to check for an existing this.texture and
call its dispose (matching the pattern used in updateSource) before assigning a
new texture from createTextureByType, then set this.texture.needsUpdate = true.
Comment on lines
+171
to
+194
| function ime_create (): void { | ||
| if (ime_input || typeof document === 'undefined') { | ||
| return; | ||
| } | ||
| ime_input = document.createElement('textarea'); | ||
| ime_input.style.cssText = | ||
| 'position:fixed;left:-9999px;top:-9999px;width:1px;height:1px;opacity:0;'; | ||
| document.body.appendChild(ime_input); | ||
|
|
||
| ime_input.addEventListener('compositionstart', ime_on_compositionstart); | ||
| ime_input.addEventListener('compositionend', ime_on_compositionend); | ||
| ime_input.addEventListener('input', ime_on_input); | ||
|
|
||
| ime_input.addEventListener('keydown', (e: KeyboardEvent) => { | ||
| if (!is_composing && !e.isComposing) { | ||
| canvas_on_keydown(e); | ||
| } | ||
| }); | ||
| ime_input.addEventListener('keyup', (e: KeyboardEvent) => { | ||
| if (!is_composing && !e.isComposing) { | ||
| canvas_on_keyup(e); | ||
| } | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Reset ImGui key state when the IME textarea blurs.
Once ime_input has focus, canvas_on_blur() is out of the loop. If the user alt-tabs away or clicks outside the app before keyup arrives, io.KeysDown and the modifier flags can stay latched until some later keyboard event happens. Reuse the same reset path from an ime_input/window blur handler as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-packages/imgui-demo/src/imgui/imgui_impl.ts` around lines 171 - 194, The
IME textarea can keep focus and leave ImGui key state latched when the window or
the textarea blurs; update ime_create to attach a blur handler on ime_input (and
also listen for window blur) that calls the same reset path as canvas_on_blur
(so it clears io.KeysDown and modifier flags) — e.g., add
ime_input.addEventListener('blur', ...) and window.addEventListener('blur', ...)
that invoke canvas_on_blur (or the existing key-reset routine) to ensure key
state is cleared when focus is lost.
Comment on lines
+204
to
+208
| function ime_focus (): void { | ||
| if (ime_input && document.activeElement !== ime_input) { | ||
| ime_input.focus(); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Don't steal focus back from unrelated DOM controls.
NewFrame() keeps calling ime_focus() while io.WantTextInput is true. If the user clicks another input outside the canvas, ImGui never sees that click, so this code immediately yanks focus back to the hidden textarea on the next frame.
🔧 Possible fix
function ime_focus (): void {
- if (ime_input && document.activeElement !== ime_input) {
- ime_input.focus();
- }
+ if (!ime_input || typeof document === 'undefined') {
+ return;
+ }
+ const activeElement = document.activeElement;
+
+ if (
+ activeElement !== null &&
+ activeElement !== document.body &&
+ activeElement !== canvas &&
+ activeElement !== ime_input
+ ) {
+ return;
+ }
+ if (activeElement !== ime_input) {
+ ime_input.focus();
+ }
}Also applies to: 455-460
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-packages/imgui-demo/src/imgui/imgui_impl.ts` around lines 204 - 208, The
ime_focus() routine currently forces focus back to ime_input whenever
io.WantTextInput is true, which yanks focus from unrelated DOM controls; change
ime_focus (and the same check at the other site around lines 455-460) to only
call ime_input.focus() when the current document.activeElement is either already
ime_input, the canvas element used by ImGui, or a non-focusable element (e.g.,
document.body/null) — do not focus when activeElement is some other
input/control; use the ImGui input intent flag io.WantTextInput and the existing
ime_input and NewFrame call sites to locate and apply this conditional guard so
unrelated inputs keep their focus.
Comment on lines
+81
to
+93
| private drawURLSection (): void { | ||
| ImGui.AlignTextToFramePadding(); | ||
| ImGui.Text('URL'); | ||
| ImGui.SameLine(); | ||
| ImGui.SetNextItemWidth(ImGui.GetContentRegionAvail().x); | ||
| if (ImGui.InputText( | ||
| '##CompositionURL', | ||
| (value = this.currentCompositionURL) => this.currentCompositionURL = value, | ||
| undefined, | ||
| ImGui.InputTextFlags.EnterReturnsTrue, | ||
| )) { | ||
| this.playURL(this.currentCompositionURL); | ||
| } |
Contributor
There was a problem hiding this comment.
Clear stale row state on manual URL launches.
playURL() only updates row state when a list index is supplied, so pressing Enter in the URL field keeps the previously clicked row marked as selected/playing even though a different URL is being loaded. If Selection.select(null) is intentional for composition switches, that decision should live in the same place too so both launch paths stay consistent.
🔧 Possible fix
if (ImGui.IsItemClicked()) {
- this.currentItem = idx;
this.currentCompositionURL = this.compositionURLs[idx];
Selection.select(null);
this.playURL(this.currentCompositionURL, idx);
}
...
private playURL (url: string, listIndex?: number): void {
if (!url) {
return;
}
- if (listIndex !== undefined) {
- this.playingIndex = listIndex;
- }
+ const resolvedIndex = listIndex ?? this.compositionURLs.indexOf(url);
+
+ this.currentItem = resolvedIndex;
+ this.playingIndex = resolvedIndex;
void GalaceanEffects.playURL(url);
}Also applies to: 165-169, 293-300
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-packages/imgui-demo/src/panels/composition.ts` around lines 81 - 93, The
URL-input Enter handler in drawURLSection triggers
playURL(this.currentCompositionURL) but doesn't clear any previously selected
row, leaving stale selection; update the manual-launch code path (the
ImGui.InputText Enter handlers in drawURLSection and the similar blocks at the
other mentioned spots) to call Selection.select(null) when launching a URL
without a list index, or alternatively modify playURL to accept an optional
index and perform Selection.select(null) when index is undefined; target the
ImGui.InputText Enter handlers and the playURL method (and ensure
currentCompositionURL is passed consistently) so both manual and list launches
clear/maintain selection state the same way.
| } | ||
| } | ||
|
|
||
| private drawItemTypeIcon (drawList: any, itemType: spec.ItemType, rowRectMin: ImGui.Vec2, rowHeight: number): void { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the local ImGui bindings expose a concrete draw-list type
# that can replace `any` in `drawItemTypeIcon`.
# Expected: a declaration for `GetWindowDrawList()` and/or an interface/class/type
# representing the returned draw-list object.
rg -n -C3 'GetWindowDrawList\s*\(|interface\s+\w*DrawList|class\s+\w*DrawList|type\s+\w*DrawList' --glob '*.ts' --glob '*.d.ts'Repository: galacean/effects-runtime
Length of output: 29649
🏁 Script executed:
sed -n '625,635p' web-packages/imgui-demo/src/panels/hierarchy.tsRepository: galacean/effects-runtime
Length of output: 466
🏁 Script executed:
rg -n 'drawItemTypeIcon' web-packages/imgui-demo/src/panels/hierarchy.ts -A 2 -B 2Repository: galacean/effects-runtime
Length of output: 558
🏁 Script executed:
sed -n '340,370p' web-packages/imgui-demo/src/panels/hierarchy.tsRepository: galacean/effects-runtime
Length of output: 1169
🏁 Script executed:
sed -n '630,680p' web-packages/imgui-demo/src/panels/hierarchy.tsRepository: galacean/effects-runtime
Length of output: 2335
🏁 Script executed:
rg -n 'interface DrawList|class DrawList|type DrawList|ImDrawList|ImGui\.DrawList' web-packages/imgui-demo/src/imgui --glob '*.ts' --glob '*.d.ts' -B 2 -A 2 | head -50Repository: galacean/effects-runtime
Length of output: 4868
🏁 Script executed:
fd -t f '\.d\.ts$' web-packages/imgui-demo/src/imgui | xargs grep -l 'DrawList' | head -5Repository: galacean/effects-runtime
Length of output: 50
🏁 Script executed:
rg -n 'export.*DrawList|declare.*DrawList' web-packages/imgui-demo/src --glob '*.ts' -B 2 -A 5Repository: galacean/effects-runtime
Length of output: 1160
🏁 Script executed:
rg -n 'ImGui\.(ImDrawList|DrawList)' web-packages/imgui-demo/src/panels/node-graph --glob '*.ts' | head -20Repository: galacean/effects-runtime
Length of output: 574
Replace any with ImGui.ImDrawList for the draw helper parameter.
Line 630 uses drawList: any, which disables type checking on every AddRectFilled, AddCircleFilled, and other draw-list method calls throughout this helper. The codebase already uses ImGui.ImDrawList elsewhere (e.g., bezier-math.ts:72, imgui-x.ts:648); use it here instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-packages/imgui-demo/src/panels/hierarchy.ts` at line 630, The
drawItemTypeIcon helper currently types its drawList parameter as any which
disables type checking for calls like AddRectFilled/AddCircleFilled; change the
parameter type to ImGui.ImDrawList in the function signature of drawItemTypeIcon
so all draw-list method calls are statically typed (update "private
drawItemTypeIcon (drawList: any, itemType: spec.ItemType, rowRectMin:
ImGui.Vec2, rowHeight: number): void" to use ImGui.ImDrawList) and ensure any
uses of drawItemTypeIcon pass an ImDrawList-compatible object.
Comment on lines
+75
to
+107
| // 计算所有关键帧位置 | ||
| const keyframePositions: { x: number, kf: KeyframeData, index: number }[] = []; | ||
|
|
||
| for (let kfIndex = 0; kfIndex < channel.keyframes.length; kfIndex++) { | ||
| const keyframe = channel.keyframes[kfIndex]; | ||
| const absoluteTime = clipStart + keyframe.time * clipDuration; | ||
| const pixelX = lineStartPos.x + LAYOUT.clipsAreaLeftPadding + timeToPixel(absoluteTime, state); | ||
|
|
||
| keyframePositions.push({ x: pixelX, kf: keyframe, index: kfIndex }); | ||
| } | ||
|
|
||
| // 重叠检测 | ||
| const overlapSet = new Set<number>(); | ||
|
|
||
| for (let i = 0; i < keyframePositions.length; i++) { | ||
| for (let j = i + 1; j < keyframePositions.length; j++) { | ||
| if (Math.abs(keyframePositions[i].x - keyframePositions[j].x) <= LAYOUT.keyOverlapThresholdPx) { | ||
| overlapSet.add(keyframePositions[i].index); | ||
| overlapSet.add(keyframePositions[j].index); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // KeyBar（使用分量颜色 + 首尾渐变） | ||
| if (keyframePositions.length > 1) { | ||
| const keyBarColor = channel.color; | ||
| const keyBarHeight = 2; | ||
| const keyBarHalfHeight = keyBarHeight / 2; | ||
|
|
||
| for (let i = 0; i < keyframePositions.length - 1; i++) { | ||
| const startX = keyframePositions[i].x; | ||
| const endX = keyframePositions[i + 1].x; | ||
| const barLength = endX - startX; |
Contributor
There was a problem hiding this comment.
Sort keyframePositions before using them.
web-packages/imgui-demo/src/panels/sequencer/data-extraction.ts, Lines 192-286, validates keyframe times but never sorts them. If the source array arrives out of order, barLength becomes negative here and the key bars either disappear or connect the wrong neighbors. Sorting once before overlap detection keeps the render order and hover precedence deterministic.
Suggested fix
for (let kfIndex = 0; kfIndex < channel.keyframes.length; kfIndex++) {
const keyframe = channel.keyframes[kfIndex];
const absoluteTime = clipStart + keyframe.time * clipDuration;
const pixelX = lineStartPos.x + LAYOUT.clipsAreaLeftPadding + timeToPixel(absoluteTime, state);
keyframePositions.push({ x: pixelX, kf: keyframe, index: kfIndex });
}
+
+ keyframePositions.sort((a, b) => a.x - b.x || a.index - b.index);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // 计算所有关键帧位置 | |
| const keyframePositions: { x: number, kf: KeyframeData, index: number }[] = []; | |
| for (let kfIndex = 0; kfIndex < channel.keyframes.length; kfIndex++) { | |
| const keyframe = channel.keyframes[kfIndex]; | |
| const absoluteTime = clipStart + keyframe.time * clipDuration; | |
| const pixelX = lineStartPos.x + LAYOUT.clipsAreaLeftPadding + timeToPixel(absoluteTime, state); | |
| keyframePositions.push({ x: pixelX, kf: keyframe, index: kfIndex }); | |
| } | |
| // 重叠检测 | |
| const overlapSet = new Set<number>(); | |
| for (let i = 0; i < keyframePositions.length; i++) { | |
| for (let j = i + 1; j < keyframePositions.length; j++) { | |
| if (Math.abs(keyframePositions[i].x - keyframePositions[j].x) <= LAYOUT.keyOverlapThresholdPx) { | |
| overlapSet.add(keyframePositions[i].index); | |
| overlapSet.add(keyframePositions[j].index); | |
| } | |
| } | |
| } | |
| // KeyBar（使用分量颜色 + 首尾渐变） | |
| if (keyframePositions.length > 1) { | |
| const keyBarColor = channel.color; | |
| const keyBarHeight = 2; | |
| const keyBarHalfHeight = keyBarHeight / 2; | |
| for (let i = 0; i < keyframePositions.length - 1; i++) { | |
| const startX = keyframePositions[i].x; | |
| const endX = keyframePositions[i + 1].x; | |
| const barLength = endX - startX; | |
| // 计算所有关键帧位置 | |
| const keyframePositions: { x: number, kf: KeyframeData, index: number }[] = []; | |
| for (let kfIndex = 0; kfIndex < channel.keyframes.length; kfIndex++) { | |
| const keyframe = channel.keyframes[kfIndex]; | |
| const absoluteTime = clipStart + keyframe.time * clipDuration; | |
| const pixelX = lineStartPos.x + LAYOUT.clipsAreaLeftPadding + timeToPixel(absoluteTime, state); | |
| keyframePositions.push({ x: pixelX, kf: keyframe, index: kfIndex }); | |
| } | |
| keyframePositions.sort((a, b) => a.x - b.x || a.index - b.index); | |
| // 重叠检测 | |
| const overlapSet = new Set<number>(); | |
| for (let i = 0; i < keyframePositions.length; i++) { | |
| for (let j = i + 1; j < keyframePositions.length; j++) { | |
| if (Math.abs(keyframePositions[i].x - keyframePositions[j].x) <= LAYOUT.keyOverlapThresholdPx) { | |
| overlapSet.add(keyframePositions[i].index); | |
| overlapSet.add(keyframePositions[j].index); | |
| } | |
| } | |
| } | |
| // KeyBar（使用分量颜色 + 首尾渐变） | |
| if (keyframePositions.length > 1) { | |
| const keyBarColor = channel.color; | |
| const keyBarHeight = 2; | |
| const keyBarHalfHeight = keyBarHeight / 2; | |
| for (let i = 0; i < keyframePositions.length - 1; i++) { | |
| const startX = keyframePositions[i].x; | |
| const endX = keyframePositions[i + 1].x; | |
| const barLength = endX - startX; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-packages/imgui-demo/src/panels/sequencer/keyframe-renderer.ts` around
lines 75 - 107, keyframePositions is built from channel.keyframes but never
sorted, so later code that assumes increasing x (e.g., the overlap detection
loop and the KeyBar loop computing barLength = endX - startX) can produce
negative lengths or wrong neighbor connections; fix by sorting keyframePositions
by their x (pixelX) or by absoluteTime after populating the array and before
creating overlapSet and drawing KeyBar (references: keyframePositions,
overlapSet, channel.keyframes, timeToPixel, KeyBar loop that computes barLength)
so rendering and hover precedence are deterministic even if source keyframes are
out of order.
Comment on lines
+12
to
+23
| for (const child of track.getChildTracks()) { | ||
| if (child.constructor.name === 'ActivationTrack' || child.constructor.name.includes('Activation')) { | ||
| const childClips = typeof child.getClips === 'function' ? child.getClips() : []; | ||
|
|
||
| if (childClips && childClips.length > 0) { | ||
| targetTrackToSelectClip = child; | ||
| clips = childClips; | ||
|
|
||
| break; | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
constructor.name check will fail in minified/production builds.
Using child.constructor.name === 'ActivationTrack' is fragile because constructor names get mangled during minification. This will cause the code to silently fail in production builds.
The codebase already uses instanceof ActivationTrack for this check (see packages/effects-core/src/plugins/timeline/tracks/object-binding-track.ts:26-28), and ActivationTrack is exported from @galacean/effects.
🔧 Proposed fix using instanceof
+import { ActivationTrack } from '@galacean/effects';
import type { TrackAsset } from '@galacean/effects';
import type { SequencerState } from './sequencer-state';
export function selectTrack (state: SequencerState, track: TrackAsset): void {
state.selectedTrack = track;
// Try to find the ActivationTrack or any track with clips to select its first clip
let targetTrackToSelectClip = track;
let clips = typeof track.getClips === 'function' ? track.getClips() : [];
if (!clips || clips.length === 0) {
for (const child of track.getChildTracks()) {
- if (child.constructor.name === 'ActivationTrack' || child.constructor.name.includes('Activation')) {
+ if (child instanceof ActivationTrack) {
const childClips = typeof child.getClips === 'function' ? child.getClips() : [];
if (childClips && childClips.length > 0) {
targetTrackToSelectClip = child;
clips = childClips;
break;
}
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const child of track.getChildTracks()) { | |
| if (child.constructor.name === 'ActivationTrack' || child.constructor.name.includes('Activation')) { | |
| const childClips = typeof child.getClips === 'function' ? child.getClips() : []; | |
| if (childClips && childClips.length > 0) { | |
| targetTrackToSelectClip = child; | |
| clips = childClips; | |
| break; | |
| } | |
| } | |
| } | |
| import { ActivationTrack } from '@galacean/effects'; | |
| import type { TrackAsset } from '@galacean/effects'; | |
| import type { SequencerState } from './sequencer-state'; | |
| export function selectTrack (state: SequencerState, track: TrackAsset): void { | |
| state.selectedTrack = track; | |
| // Try to find the ActivationTrack or any track with clips to select its first clip | |
| let targetTrackToSelectClip = track; | |
| let clips = typeof track.getClips === 'function' ? track.getClips() : []; | |
| if (!clips || clips.length === 0) { | |
| for (const child of track.getChildTracks()) { | |
| if (child instanceof ActivationTrack) { | |
| const childClips = typeof child.getClips === 'function' ? child.getClips() : []; | |
| if (childClips && childClips.length > 0) { | |
| targetTrackToSelectClip = child; | |
| clips = childClips; | |
| break; | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-packages/imgui-demo/src/panels/sequencer/selection.ts` around lines 12 -
23, The loop is using fragile constructor.name checks which break after
minification; replace those checks with a proper instanceof check against
ActivationTrack: import ActivationTrack from '@galacean/effects' (or the correct
exported symbol) and change the condition to use (child instanceof
ActivationTrack) while keeping the existing fallback for child.getClips (i.e.,
call typeof child.getClips === 'function' ? child.getClips() : []), then set
targetTrackToSelectClip and clips and break as before.
|
|
||
| // 选中状态 | ||
| selectedTrack: TrackAsset | null = null; | ||
| selectedClip: any = null; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the clip type used in `@galacean/effects`
rg -n "getClips\(\)" --type=ts -A2 packages/effects-core/src/plugins/timeline/ | head -30Repository: galacean/effects-runtime
Length of output: 1220
🏁 Script executed:
# Search for Clip type definitions in effects-core
rg -n "class.*Clip|interface.*Clip|type.*Clip" --type=ts packages/effects-core/src/plugins/timeline/ | head -20Repository: galacean/effects-runtime
Length of output: 728
🏁 Script executed:
# Find PlayableAsset type definition
rg -n "class PlayableAsset|interface PlayableAsset" --type=ts packages/effects-core/src/ | head -10Repository: galacean/effects-runtime
Length of output: 186
🏁 Script executed:
# Read the sequencer-state.ts file to see selectedClip usage
cat -n web-packages/imgui-demo/src/panels/sequencer/sequencer-state.tsRepository: galacean/effects-runtime
Length of output: 1935
🏁 Script executed:
# Check what types are exported from `@galacean/effects`
rg -n "export.*Clip|export.*PlayableAsset" --type=ts packages/effects-core/src/index.ts | head -20Repository: galacean/effects-runtime
Length of output: 50
🏁 Script executed:
# Find how selectedClip is used in the sequencer component
rg -n "selectedClip" --type=ts web-packages/imgui-demo/src/panels/sequencer/ -B1 -A1Repository: galacean/effects-runtime
Length of output: 3815
🏁 Script executed:
# Check the TimelineClip and RuntimeClip class definitions
sed -n '10,30p' packages/effects-core/src/plugins/timeline/track.ts
sed -n '170,190p' packages/effects-core/src/plugins/timeline/track.tsRepository: galacean/effects-runtime
Length of output: 1123
🏁 Script executed:
# Check main package exports for TimelineClip
cat packages/effects-core/src/index.ts | grep -i "timelineclip\|export.*from.*track"Repository: galacean/effects-runtime
Length of output: 50
🏁 Script executed:
# Check what's exported from the effects package main entry
cat packages/effects-core/package.json | grep -A5 '"main"'Repository: galacean/effects-runtime
Length of output: 172
🏁 Script executed:
# See the full imports in the sequencer files to understand the available types
head -10 web-packages/imgui-demo/src/panels/sequencer/clip-renderer.ts
head -10 web-packages/imgui-demo/src/panels/sequencer/selection.tsRepository: galacean/effects-runtime
Length of output: 1104
🏁 Script executed:
# Check TrackAsset interface definition
rg -n "interface TrackAsset|class TrackAsset" --type=ts packages/effects-core/src/ -A10 | head -40Repository: galacean/effects-runtime
Length of output: 902
🏁 Script executed:
# Check the getClips method signature in TrackAsset
rg -n "getClips" --type=ts packages/effects-core/src/plugins/timeline/track.ts -B2 -A2Repository: galacean/effects-runtime
Length of output: 151
🏁 Script executed:
# List all exports from effects-core index.ts
cat packages/effects-core/src/index.tsRepository: galacean/effects-runtime
Length of output: 1977
Use TimelineClip type instead of any for selectedClip.
The property loses type safety with any typing. The actual type should be TimelineClip | null based on how it's used throughout the codebase (accessing properties like endBehavior in properties-panel-renderer.ts). However, TimelineClip is not exported from the public @galacean/effects API, so you'll need to import it from the internal path:
import { TimelineClip } from '@galacean/effects/dist/plugins/timeline/track';Then update the declaration to:
selectedClip: TimelineClip | null = null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-packages/imgui-demo/src/panels/sequencer/sequencer-state.ts` at line 33,
Replace the loose any typing on the sequencer state property selectedClip with
the concrete TimelineClip type to restore type safety: import TimelineClip from
the internal path `@galacean/effects/dist/plugins/timeline/track` and change the
declaration of selectedClip to use TimelineClip | null (used elsewhere, e.g.
properties-panel-renderer.ts reads properties like endBehavior), ensuring the
new import is added to the top of sequencer-state.ts and the initialization
remains null.
Comment on lines
+50
to
+76
| state.currentComposition = GalaceanEffects.player.getCompositions()[0]; | ||
| const currentComposition = state.currentComposition; | ||
|
|
||
| // 清除轨道颜色缓存当 composition 变化时 | ||
| const compositionId = currentComposition.name ?? ''; | ||
|
|
||
| if (state.lastCompositionId !== compositionId) { | ||
| state.trackColorMap.clear(); | ||
| state.lastCompositionId = compositionId; | ||
| } | ||
|
|
||
| let compositionComponent = currentComposition.rootItem.getComponent(CompositionComponent); | ||
|
|
||
| const selectedObject = Selection.getSelectedObjects()[0]; | ||
|
|
||
| if (selectedObject instanceof VFXItem && selectedObject.getComponent(CompositionComponent)) { | ||
| compositionComponent = selectedObject.getComponent(CompositionComponent); | ||
| } | ||
|
|
||
| if (!compositionComponent) { | ||
| return; | ||
| } | ||
|
|
||
| // 使用当前 Composition 的时长作为时间轴最大时长 | ||
| if (typeof currentComposition.getDuration === 'function') { | ||
| state.timelineEndTime = currentComposition.getDuration(); | ||
| } |
Contributor
There was a problem hiding this comment.
Keep currentComposition aligned with the selected CompositionComponent.
After Lines 61-67 swap compositionComponent, the rest of the method still drives the first player composition captured at Lines 50-51. That lets the window render one composition's tracks while the ruler, scrubbing, and transport buttons operate on another. Update state.currentComposition / currentComposition at the same time you switch the component.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-packages/imgui-demo/src/panels/sequencer/sequencer.ts` around lines 50 -
76, When you detect and swap to the selected CompositionComponent (after the
block where compositionComponent is reassigned based on selectedObject
instanceof VFXItem and getComponent(CompositionComponent)), also update
state.currentComposition and the local currentComposition to match that
component's composition; then recompute compositionId (and clear
state.trackColorMap / set state.lastCompositionId if needed) and set
state.timelineEndTime from the new currentComposition.getDuration() if present.
In short: after the compositionComponent reassignment, set
state.currentComposition = compositionComponent.ownerComposition (or derive the
Composition instance from compositionComponent), update the compositionId
handling and timelineEndTime using that updated currentComposition so the
tracks, ruler, scrubbing and transport use the same composition.
Comment on lines
+369
to
+380
| if (isHovered) { | ||
| const r = ((color >> 24) & 0xFF) / 255; | ||
| const g = ((color >> 16) & 0xFF) / 255; | ||
| const b = ((color >> 8) & 0xFF) / 255; | ||
| const a = (color & 0xFF) / 255; | ||
|
|
||
| const brightR = Math.min(1.0, r + 0.3); | ||
| const brightG = Math.min(1.0, g + 0.3); | ||
| const brightB = Math.min(1.0, b + 0.3); | ||
|
|
||
| drawColor = ImGui.GetColorU32(new ImGui.Vec4(brightR, brightG, brightB, a)); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the byte order of ImGui GetColorU32 return value - is it RGBA or ABGR?
💡 Result:
The byte order of ImGui::GetColorU32 return value is ABGR. That is, in the returned ImU32 value, the bytes are ordered as A (highest byte), B, G, R (lowest byte) on little-endian platforms, which is the standard for ImGui colors. This follows from the IM_COL32(R,G,B,A) macro defined in imgui.h as (((ImU32)(A)<<24) | ((ImU32)(B)<<16) | ((ImU32)(G)<<8) | ((ImU32)(R))), packing R in the least significant byte and A in the most significant byte. GetColorU32 returns values in this same packed format, as confirmed by multiple sources including Rust bindings documentation explicitly stating "ImU32 ABGR order" and GitHub issues where ImGui maintainer ocornut recommends using IM_COL32 to avoid ambiguity between common RGBA/ARGB hex notations and ImGui's ABGR storage.
Citations:
- 1: Strange endian behaviour causing ImU32 colors to be flipped ocornut/imgui#7525
- 2: https://docs.rs/dear-imgui-rs/latest/src/dear_imgui_rs/colors.rs.html
- 3: https://gitea.dresselhaus.cloud/Drezil/imgui/commit/d5fd87e80613e3d21197cbacb1eb43033f2deb23?files=imgui.h
- 4: SDL2: unexpected color rgba mismatch? ocornut/imgui#8201
Fix incorrect color byte order in hover brightening.
The color extraction incorrectly assumes RGBA byte order (R at bits 24-31, G at 16-23, B at 8-15, A at 0-7). ImGui::GetColorU32 returns colors in ABGR format (A at bits 24-31, B at 16-23, G at 8-15, R at 0-7). This causes incorrect colors during the hover brightening effect.
Fix: Extract colors in correct ABGR order
if (isHovered) {
- const r = ((color >> 24) & 0xFF) / 255;
- const g = ((color >> 16) & 0xFF) / 255;
- const b = ((color >> 8) & 0xFF) / 255;
- const a = (color & 0xFF) / 255;
+ const r = (color & 0xFF) / 255;
+ const g = ((color >> 8) & 0xFF) / 255;
+ const b = ((color >> 16) & 0xFF) / 255;
+ const a = ((color >> 24) & 0xFF) / 255;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-packages/imgui-demo/src/panels/sequencer/track-label-renderer.ts` around
lines 369 - 380, The hover brightening currently extracts RGBA components from
the 32-bit color with R at bits 24-31 etc., but ImGui uses ABGR ordering for
GetColorU32; update the extraction in the isHovered block to read A = (color >>
24) & 0xFF, B = (color >> 16) & 0xFF, G = (color >> 8) & 0xFF, R = color & 0xFF,
normalize each to 0..1, brighten the RGB channels (brightR/brightG/brightB) and
then call ImGui.GetColorU32(new ImGui.Vec4(brightR, brightG, brightB, a)) to
compute drawColor so hovering brightens the correct channels for the variables
color, drawColor, and the ImGui.GetColorU32/ImGui.Vec4 usage in
track-label-renderer.ts.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Refactor