Dock-able motiongfx_editor - #121
Merged
Merged
Conversation
Port jackdaw_panels' generic docking engine into `ui/dock`: a pure-data `DockTree` (splits + tabbed leaves, unit-tested), a reconciler that materializes it into UI, resizable split handles, tab bars with close buttons and an add-window popup, and pointer-drag tab reorder / merge / edge-split with drop overlays. Editor integration: Viewport, Timeline and Settings are dockable windows; the scene renders to an offscreen preview image that letterboxes into its dock area. Settings is a generic reflect inspector (`ui/inspector`) over `EditorSettings` with an explicit Save. `ui` stays UI-only so it can split into its own crate later; editor wiring lives in `EditorUiPlugin` in lib.rs.
New `ui/glass`: a frosted-glass `UiMaterial` (SDF rounded-rect mask with optional feathered edge, backdrop frost blur, thin near-opaque border rim, cursor glow on hovered interactables) applied across the dock chrome via a declarative `Glass` preset component, plus a `glass_button` widget replacing the feathers-themed buttons. Frost activates when a node is tagged `GlassBackdrop`; no source is tagged right now, so panes render as tint + rim + glow. New `ui/theme`: `EditorTheme` resource carrying the Monokai Pro palette (mirrors assets/typst/monokai_pro.typ) and semantic slots; glass presets and dock text colors derive from it. Tabs gain a hover pill (`Glass::TabHover`), tile panes are squared, and plugins now sit at the top of their modules.
The drag ghost now reuses the shared tab-tile builder (identical square pill, padding, font, active material) instead of a distinct rounded card, and the source tab is hidden while dragging. Fix `drop_on_edge` losing the tab when anchored to its own leaf's edge: split before removing so the target leaf stays valid.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds reusable reactive UI, docking, glass styling, and inspector crates, then rebuilds the MotionGfx editor around them. It also updates workspace wiring, adds a docking example, adjusts timeline composition, removes the previous editor implementation, and normalizes repository formatting. ChangesEditor platform
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant App
participant MotionGfxEditorPlugin
participant KernelPlugin
participant EditorScene
participant DockTree
participant Playback
App->>MotionGfxEditorPlugin: register editor plugins
MotionGfxEditorPlugin->>KernelPlugin: install reactive UI root
EditorScene->>DockTree: build dock layout and windows
EditorScene->>Playback: bind timeline and playback state
Playback->>EditorScene: update timeline UI and preview
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/rust.yml:
- Around line 1-7: Update the top-level workflow configuration around the name
and on declarations to explicitly set GITHUB_TOKEN permissions to read-only
using the workflow’s permissions setting. Keep the existing pull_request and
push triggers unchanged.
In `@editor/motiongfx_editor_ui/src/dock.rs`:
- Around line 7-16: Update the module-level documentation in dock.rs to reflect
the current integration: remove the claim that DockPlugin, WindowRegistry,
DockTreeHost, and DockTree still need to be wired or registered, and remove “add
window” popup from the out-of-scope list because AddWindowPopupPlugin is
registered. Preserve the remaining documented scope accurately.
In `@editor/motiongfx_editor_ui/src/dock/add_popup.rs`:
- Around line 80-86: Update the AddWindowPopupBackdrop Pickable configuration so
outside-click dismissal does not propagate to controls behind the popup, while
preserving the backdrop’s ability to detect the click and close the popup. Keep
the change scoped to should_block_lower and retain the existing hover behavior
unless required by the dismissal flow.
In `@editor/motiongfx_editor_ui/src/dock/drag.rs`:
- Around line 250-263: Update the child-position calculation in the
drag-selection loop to use each child’s transform from node_query.get, rather
than reusing the row-level ui_transform. Derive child_center from
_child_transform while preserving the existing scale, size, distance, and
nearest-tab selection logic.
In `@editor/motiongfx_editor_ui/src/dock/reconcile.rs`:
- Around line 55-65: Update reconcile_tree and reconcile_at to propagate a
registry_changed/force-rebuild flag when WindowRegistry changes, rather than
only reacting to DockTree changes. Include that flag in each leaf’s
needs_rebuild decision so unregistering or replacing a descriptor rebuilds
existing content, labels, and build output even when tab IDs are unchanged.
In `@editor/motiongfx_editor_ui/src/dock/registry.rs`:
- Around line 26-44: Update DockRegistry::register to handle an existing
descriptor ID before appending: replace the descriptor at its current index, or
reject the duplicate according to the intended registry semantics. Ensure
windows contains at most one descriptor per ID and keep index consistent;
preserve unregister and iter behavior for unique registrations.
In `@editor/motiongfx_editor_ui/src/dock/tree.rs`:
- Around line 582-604: Adjust the insertion index in the tab-moving logic around
the source removal and destination insertion: when from and to refer to the same
leaf and the dragged tab’s original position is before the requested index,
decrement the index after removal before clamping and inserting. Preserve
cross-leaf and backward moves, and add regression coverage for moving the first
tab before and after later tabs.
In `@editor/motiongfx_editor_ui/src/glass/backdrop.rs`:
- Around line 24-43: Update the cached state in the backdrop update system
around the last rect check to include the current GlassBackdrop.0 handle or
otherwise detect GlassBackdrop changes. Only return early when both the
rectangle and image handle are unchanged, and propagate a newly changed backdrop
image to every material even when rect remains the same.
In `@editor/motiongfx_editor_ui/src/inspector.rs`:
- Around line 119-124: Update the generic inspector conversion and edit handling
around the u32/u64 branches and the associated event path (including the code
covering the referenced later range) to preserve concrete integer types instead
of routing them through f64 or i64. Use checked conversions for unsigned values,
reject values outside the target type’s valid range, and ensure edits write back
the original integer type without precision loss.
In `@editor/motiongfx_editor_ui/src/lib.rs`:
- Around line 148-150: Remove the manually inserted Hovered component from the
Button node construction, leaving bevy_picking to manage hover state through
pointer events while preserving the surrounding button setup.
In `@editor/motiongfx_editor/src/layout.rs`:
- Around line 69-75: Update editor/motiongfx_editor/src/layout.rs:69-75 to build
and tear down generated content independently for each TimelineContent instead
of using a singleton query. Update
editor/motiongfx_editor/src/hierarchy.rs:47-49 to populate every HierarchyPanel,
editor/motiongfx_editor/src/playback.rs:233-237 to synchronize every
PlayPauseLabel, and editor/motiongfx_editor/src/scene.rs:431-435 to resolve the
NamePanel associated with the dragged divider; preserve each instance’s entity
relationships when iterating duplicate docked windows.
In `@editor/motiongfx_editor/src/playback.rs`:
- Around line 38-47: Update the player toggle loop in the playback control
function to compute one target play/pause state before iterating, then assign
that state to every player’s is_playing field instead of individually inverting
each value. Preserve resetting time_scale to 1.0 and the existing rewind logic
based on the resulting shared state.
- Around line 101-123: Update the timeline press/drag handling around
on_track_press to resolve press.entity or drag.entity to its owning
TimelineContent before using q_track or changing Scrubbing. Apply Scrubbing to
the resolved owner rather than descendant clips or groups. Add Pointer<Release>
handling that resolves the same owner and removes Scrubbing, including releases
that occur before dragging begins.
In `@editor/motiongfx_editor/src/view.rs`:
- Around line 61-68: Clamp the editable composition dimensions from
settings.physical_size to the same valid minimums used when creating the preview
target before computing aspect. Update the sizing logic around comp and aspect
so zero-valued width or height cannot produce infinity or NaN, while preserving
the existing available-area fitting behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ad36a2e4-3a2e-4881-9fb1-fef9f948b07c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (45)
.github/workflows/rust.yml.gitignoreCargo.tomlLICENSE-APACHELICENSE-MITassets/themes/Monokai Pro.tmThemecrates/bevy_motiongfx/Cargo.tomlcrates/motiongfx/Cargo.tomlcrates/motiongfx/src/lib.rscrates/motiongfx/src/track.rscrates/motiongfx/src/track/meta.rscrates/motiongfx_editor/src/lib.rscrates/motiongfx_editor/src/ui.rseditor/motiongfx_editor/Cargo.tomleditor/motiongfx_editor/docs/HANDOFF.mdeditor/motiongfx_editor/src/hierarchy.rseditor/motiongfx_editor/src/layout.rseditor/motiongfx_editor/src/lib.rseditor/motiongfx_editor/src/main.rseditor/motiongfx_editor/src/playback.rseditor/motiongfx_editor/src/scene.rseditor/motiongfx_editor/src/view.rseditor/motiongfx_editor_ui/Cargo.tomleditor/motiongfx_editor_ui/src/dock.rseditor/motiongfx_editor_ui/src/dock/add_popup.rseditor/motiongfx_editor_ui/src/dock/area.rseditor/motiongfx_editor_ui/src/dock/drag.rseditor/motiongfx_editor_ui/src/dock/reconcile.rseditor/motiongfx_editor_ui/src/dock/registry.rseditor/motiongfx_editor_ui/src/dock/split.rseditor/motiongfx_editor_ui/src/dock/tabs.rseditor/motiongfx_editor_ui/src/dock/tree.rseditor/motiongfx_editor_ui/src/glass.rseditor/motiongfx_editor_ui/src/glass.wgsleditor/motiongfx_editor_ui/src/glass/backdrop.rseditor/motiongfx_editor_ui/src/glass/glow.rseditor/motiongfx_editor_ui/src/glass/material.rseditor/motiongfx_editor_ui/src/glass/preset.rseditor/motiongfx_editor_ui/src/glass/widget.rseditor/motiongfx_editor_ui/src/inspector.rseditor/motiongfx_editor_ui/src/lib.rseditor/motiongfx_editor_ui/src/theme.rsexamples/bevy_examples/Cargo.tomlexamples/bevy_examples/examples/dock_demo.rsrustfmt.toml
💤 Files with no reviewable changes (2)
- crates/motiongfx_editor/src/lib.rs
- crates/motiongfx_editor/src/ui.rs
Comment on lines
+1
to
+7
| name: CI | ||
| on: | ||
| pull_request: | ||
| push: | ||
| branches: | ||
| - main | ||
| jobs: |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Restrict default permissions for GITHUB_TOKEN.
By default, GitHub Actions may grant broad permissions to the GITHUB_TOKEN depending on repository settings. To follow the principle of least privilege and prevent potential abuse by untrusted code executing during builds or tests, explicitly restrict top-level permissions.
🔒️ Proposed fix to apply read-only permissions
name: CI
on:
pull_request:
push:
branches:
- main
+
+permissions:
+ contents: read
+
jobs:📝 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
| name: CI | |
| on: | |
| pull_request: | |
| push: | |
| branches: | |
| - main | |
| jobs: | |
| name: CI | |
| on: | |
| pull_request: | |
| push: | |
| branches: | |
| - main | |
| permissions: | |
| contents: read | |
| jobs: |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 1-91: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/rust.yml around lines 1 - 7, Update the top-level workflow
configuration around the name and on declarations to explicitly set GITHUB_TOKEN
permissions to read-only using the workflow’s permissions setting. Keep the
existing pull_request and push triggers unchanged.
Source: Linters/SAST tools
Comment on lines
+80
to
+86
| AddWindowPopupBackdrop, | ||
| // Catch the outside-click to close, but let hover/clicks | ||
| // pass through to the UI beneath instead of freezing it. | ||
| Pickable { | ||
| should_block_lower: false, | ||
| is_hoverable: true, | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Block dismissal clicks from reaching controls behind the popup.
With should_block_lower: false, clicking outside closes the popup and also activates the underlying editor target. This can unintentionally trigger buttons or other actions.
Proposed fix
Pickable {
- should_block_lower: false,
+ should_block_lower: true,
is_hoverable: true,
},📝 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
| AddWindowPopupBackdrop, | |
| // Catch the outside-click to close, but let hover/clicks | |
| // pass through to the UI beneath instead of freezing it. | |
| Pickable { | |
| should_block_lower: false, | |
| is_hoverable: true, | |
| }, | |
| AddWindowPopupBackdrop, | |
| // Catch the outside-click to close, but let hover/clicks | |
| // pass through to the UI beneath instead of freezing it. | |
| Pickable { | |
| should_block_lower: true, | |
| is_hoverable: true, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@editor/motiongfx_editor_ui/src/dock/add_popup.rs` around lines 80 - 86,
Update the AddWindowPopupBackdrop Pickable configuration so outside-click
dismissal does not propagate to controls behind the popup, while preserving the
backdrop’s ability to detect the click and close the popup. Keep the change
scoped to should_block_lower and retain the existing hover behavior unless
required by the dismissal flow.
Comment on lines
+55
to
+65
| fn reconcile_tree(world: &mut World) { | ||
| if !world.is_resource_changed::<DockTree>() { | ||
| return; | ||
| } | ||
| let Some(root) = world.resource::<DockTree>().root else { | ||
| return; | ||
| }; | ||
| let Some(host) = find_dock_tree_host(world) else { | ||
| return; | ||
| }; | ||
| reconcile_at(world, host, root); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reconcile changes to the window registry.
WindowRegistry::unregister or descriptor replacement leaves existing content, labels, and build output materialized because reconciliation only observes DockTree. Even a forced tree pass would skip rebuilding when the tab IDs are unchanged.
Propagate a registry_changed/force-rebuild flag through reconcile_at and include it in each leaf’s needs_rebuild.
Also applies to: 101-103
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@editor/motiongfx_editor_ui/src/dock/reconcile.rs` around lines 55 - 65,
Update reconcile_tree and reconcile_at to propagate a
registry_changed/force-rebuild flag when WindowRegistry changes, rather than
only reacting to DockTree changes. Include that flag in each leaf’s
needs_rebuild decision so unregistering or replacing a descriptor rebuilds
existing content, labels, and build output even when tab IDs are unchanged.
Comment on lines
+101
to
+123
| pub(crate) fn on_track_press( | ||
| mut press: On<Pointer<Press>>, | ||
| state: Res<EditorState>, | ||
| ui_scale: Res<UiScale>, | ||
| q_track: Query< | ||
| (&ComputedNode, &UiGlobalTransform), | ||
| With<TimelineContent>, | ||
| >, | ||
| mut manager: ResMut<MotionGfxManager>, | ||
| mut q_players: Query<&mut RealtimePlayer>, | ||
| mut commands: Commands, | ||
| ) { | ||
| let track = press.entity; | ||
| let Ok((computed, transform)) = q_track.get(track) else { | ||
| return; | ||
| }; | ||
| press.propagate(false); | ||
| commands.entity(track).insert(Scrubbing); | ||
|
|
||
| let cursor = press.pointer_location.position / ui_scale.0; | ||
| let time = | ||
| time_at_cursor(cursor, computed, transform, state.duration); | ||
| scrub_to(time, &state, &mut manager, &mut q_players); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -a 'Cargo.toml' -x rg -n -C2 'bevy\s*=|bevy_motiongfx' {}
rg -n -C4 'on_track_(press|drag|release|cancel)|original_event_target|trigger\(\)\.target' editor/motiongfx_editor/srcRepository: voxell-tech/motiongfx
Length of output: 5666
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,240p' editor/motiongfx_editor/src/playback.rs
printf '\n--- scene excerpt ---\n'
sed -n '180,230p' editor/motiongfx_editor/src/scene.rs
printf '\n--- scrub-related symbols ---\n'
rg -n -C 2 'Scrubbing|on_track_(press|drag|release|cancel)|Pointer<Press>|Pointer<DragEnd>|Pointer<Cancel>|TimelineContent|timeline_track' editor/motiongfx_editor/srcRepository: voxell-tech/motiongfx
Length of output: 16434
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect Bevy picking APIs referenced by the concern.
rg -n -C 3 'original_target|bubble|propagate\(false\)|Pointer<Press>|Pointer<DragEnd>|Pointer<Cancel>|On<Pointer<Press>>|On<Pointer<DragEnd>>' ~/.cargo/registry/src 2>/dev/null | head -n 200Repository: voxell-tech/motiongfx
Length of output: 159
🌐 Web query:
Bevy 0.19 picking Pointer<Press> Pointer<Release> Pointer<DragEnd> original_target bubbling DragEnd release semantics
💡 Result:
In Bevy 0.19, pointer events are managed by the picking system, which follows a strict interaction state machine [1][2][3]. Regarding your specific query on semantics and event propagation: 1. Pointer Interaction Events: - Pointer and Pointer are fundamental button events [1][2]. These are emitted by the state machine when a pointer button is interacted with [2]. - Pointer is a high-level event that triggers when a pointer, which was previously dragging a target entity, receives a Pointer event [4][5]. 2. Bubbling and Target Semantics: - Events like Pointer and Pointer are defined as bubbling, meaning they propagate up through an entity's ancestors via the ChildOf relationship [6][7]. - With the introduction of Pointer and Pointer in 0.19 (replacing certain uses of Over/Out for hierarchy-aware logic), these new events do not bubble [8]. They are sent specifically to entities based on changes in the hover state, providing a more predictable alternative for UI and interaction logic [8][9]. 3. Propagation Control: - Bevy allows you to control propagation using the event.propagate(false) method within an observer [10]. This is commonly used in On<Pointer<DragEnd>> or other event observers to stop the event from continuing its upward travel through the entity hierarchy [10]. 4. Event Targeting: - The system generally tracks the original_target of an interaction [6]. For events like Pointer, they often target the entity hovered in the previous frame, which is significant when dealing with fast movements or touch pointers that may not be hovering anything at the moment of release [2][3]. For detailed implementation, consult the pointer_events system documentation in the Bevy prelude, as it governs the dispatch order (typically: Press/Click -> Release -> DragDrop -> DragEnd -> DragLeave) [1][2][3].
Citations:
- 1: https://docs.rs/bevy/latest/bevy/picking/events/fn.pointer_events.html
- 2: https://docs.rs/bevy/latest/bevy/prelude/fn.pointer_events.html
- 3: https://docs.rs/bevy/latest/bevy/picking/prelude/fn.pointer_events.html
- 4: https://docs.rs/bevy/latest/bevy/picking/events/struct.DragEnd.html
- 5: https://docs.rs/bevy/latest/bevy/prelude/struct.DragEnd.html
- 6: https://docs.rs/bevy/latest/bevy/picking/events/index.html
- 7: Pointer<Over> is fired multiple times while moving pointer inside a button with text bevyengine/bevy#23636
- 8: Adds
Pointer<Enter>andPointer<Leave>Events - Hierarchy aware versions ofPointer<Over>andPointer<Out>bevyengine/bevy#22991 - 9: Pointer<Enter> and Pointer<Leave> bevyengine/bevy#16289
- 10: https://docs.rs/bevy/latest/src/dragdrop_picking/dragdrop_picking.rs.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n editor/motiongfx_editor/src/playback.rs | sed -n '1,220p'Repository: voxell-tech/motiongfx
Length of output: 8504
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 2 '\bClip\b|\bGroup\b|timeline_track\(|TimelineContent|Children \[' editor/motiongfx_editor/srcRepository: voxell-tech/motiongfx
Length of output: 7518
🌐 Web query:
Bevy Pointer<Press> On event entity original_target fields docs.rs 0.19
💡 Result:
In Bevy 0.19, the Pointer struct is a generic container used for pointer events (such as Press, Click, or Drag) [1][2]. It does not contain a field named original_target [1][2]. The Pointer struct consists of the following fields [1][2]: - entity: The Entity this pointer event happened for [1][2]. - pointer_id: The unique identifier for the pointer [1][2]. - pointer_location: The location of the pointer during the event [1][2]. - event: The event-specific data (e.g., the Press struct) [1][3][2]. The Press struct itself contains only the button (PointerButton) and hit (HitData) information [3]. If you are looking for target information, the Pointer struct implements EntityEvent, which provides the event_target method to retrieve the entity targeted by the event [1][2]. The picking system determines the target based on the pointer's interaction state, and some events (like Click and Release) are specifically noted to target the entity hovered in the previous frame to maintain consistency during release actions [4].
Citations:
- 1: https://docs.rs/bevy/latest/bevy/prelude/struct.Pointer.html
- 2: https://docs.rs/bevy/latest/bevy/picking/events/struct.Pointer.html
- 3: https://docs.rs/bevy/latest/bevy/prelude/struct.Press.html
- 4: https://docs.rs/bevy/latest/bevy/picking/events/fn.pointer_events.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect local Bevy sources if present to resolve Pointer<Press>/DragEnd field semantics.
BEVY_SRC=$(fd -a 'events.rs' ~/.cargo/registry/src 2>/dev/null | rg '/bevy[^/]*/.+/picking/.+events\.rs$' | head -n 5 || true)
printf '%s\n' "$BEVY_SRC"
if [ -n "$BEVY_SRC" ]; then
for f in $BEVY_SRC; do
echo "--- $f ---"
rg -n -C 2 'struct Pointer<Press>|struct Pointer<DragEnd>|original_target|target:|entity:' "$f"
done
fiRepository: voxell-tech/motiongfx
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import os, glob
paths = glob.glob(os.path.expanduser("~/.cargo/registry/src/*/bevy-*/crates/bevy_picking/src/**/*.rs"), recursive=True)
for p in paths:
if p.endswith("events.rs") or p.endswith("pointer_events.rs") or p.endswith("mod.rs"):
print(p)
PYRepository: voxell-tech/motiongfx
Length of output: 159
Resolve the timeline owner before scrubbing, and clear scrubbing on plain release.
press.entity/drag.entitycan be a clip or group descendant; resolve the owningTimelineContententity before querying or inserting/removingScrubbing.- Handle
Pointer<Release>too, so a press released before dragging doesn’t leaveScrubbingset.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@editor/motiongfx_editor/src/playback.rs` around lines 101 - 123, Update the
timeline press/drag handling around on_track_press to resolve press.entity or
drag.entity to its owning TimelineContent before using q_track or changing
Scrubbing. Apply Scrubbing to the resolved owner rather than descendant clips or
groups. Add Pointer<Release> handling that resolves the same owner and removes
Scrubbing, including releases that occur before dragging begins.
The editor visualized the composition tree by walking `Track::meta()`, which forced `motiongfx` to carry a shadow copy of authoring structure through its runtime types. The upcoming document format owns that structure instead, so the runtime doesn't need it. - delete `layout.rs` (clip/group/toggle placement, group collapse) - drop `clip_box`/`group_box`/`group_toggle` and their markers from `motiongfx_editor_ui` - restore `motiongfx`/`bevy_motiongfx` to `main`; the `metadata` feature was purely additive on this branch, so this is an exact revert rather than a hand-edit - add `playback::sync_timeline_state` for the timeline id, duration and track width that `layout.rs` used to provide Control bar, name column, divider, scroll viewport, playhead and scrubbing are unchanged; the track is now an empty strip sized to the composition length. Claude-Session: https://claude.ai/code/session_01Asj7E3ebcvYUrQ192wtaop
Add motiongfx_editor_ui_kernel: a no_std, backend-agnostic kernel of watchers (rebuild a subtree when a predicate fires) and bindings (write one field in place), with a Host trait for the backend. reactive.rs is the bevy adapter. The dock, tab bar, inspector, hierarchy, add-popup and timeline panel are now built through it. Structure rides on watchers keyed to a projection that omits volatile fields, so a splitter drag or a tab switch updates through bindings instead of rebuilding.
Move `watch` from `Ui` onto `NodeMut`, so a builder reads as "make this node, then say how it reacts" instead of making a watcher and giving it a body afterwards. Add `BevyUiExt::bsn`, which spawns a node from a `bsn!` scene, and drop the free `widget` function every call site had to wrap with.
Drop Ui::bind_raw and the Ui-level typed binds, so `Ui` only ever spawns and every watcher or binding is declared on the node it belongs to. Callers that want an invisible node to scope a binding use `group()`.
Also refresh the README's Host trait and builder sections, which still described the removed Widget associated type and the two-phase build.
ChangedFn, ApplyFn and BuildFn become traits with a blanket impl, so signatures read `impl ChangedFn<H>` instead of spelling out the FnMut bound plus Send + Sync + 'static every time. The boxed forms the kernel stores stay as private aliases.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
editor/motiongfx_editor/src/playback.rs (1)
33-56: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStill inverting per-player state instead of computing one shared play/pause target.
If
RealtimePlayers ever disagree onis_playing, inverting each independently swaps which players run rather than toggling play/pause as a whole; the rewind guard below also still checksq_players.iter().any(|p| p.is_playing)instead of the newly-computednow_playing. This matches a still-unresolved finding from a previous review round.Proposed fix
- let mut now_playing = false; - for mut player in &mut q_players { - player.is_playing = !player.is_playing; - player.time_scale = 1.0; - now_playing |= player.is_playing; - } - state.is_playing = now_playing; + let should_play = !q_players.iter().any(|p| p.is_playing); + for mut player in &mut q_players { + player.is_playing = should_play; + player.time_scale = 1.0; + } + state.is_playing = should_play; // Rewind if starting playback from the very end. - if let Some(timeline_id) = state.timeline - && q_players.iter().any(|p| p.is_playing) + if should_play + && let Some(timeline_id) = state.timeline && let Some(timeline) = manager.get_timeline_mut(&timeline_id) && timeline.target_time() >= state.duration🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/motiongfx_editor/src/playback.rs` around lines 33 - 56, Update on_toggle_playback to compute one shared play/pause target from the current player state, then assign that target to every RealtimePlayer instead of independently inverting each player. Use the resulting now_playing value for the rewind guard rather than querying q_players again, while preserving the existing time-scale reset and rewind behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@editor/motiongfx_editor_ui/src/glass/backdrop.rs`:
- Around line 23-47: Update the cached state in the binding closure around
backdrops and seen so it stores the GlassBackdrop image asset ID alongside the
rectangle. Compare both the current rectangle and handle ID when computing
changed, and update both cached values so a handle change triggers propagation
even when the rectangle is unchanged.
In `@editor/motiongfx_editor/src/view.rs`:
- Around line 39-60: Clamp EditorSettings.physical_size to at least UVec2::ONE
before converting it to a Vec2 and calculating aspect in preview_fit. Preserve
the existing sizing and early-return behavior while ensuring zero-valued
dimensions cannot produce inf or NaN.
---
Duplicate comments:
In `@editor/motiongfx_editor/src/playback.rs`:
- Around line 33-56: Update on_toggle_playback to compute one shared play/pause
target from the current player state, then assign that target to every
RealtimePlayer instead of independently inverting each player. Use the resulting
now_playing value for the rewind guard rather than querying q_players again,
while preserving the existing time-scale reset and rewind behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bd3c8685-a8d1-4767-9325-ea9ec313e826
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
Cargo.tomlcrates/motiongfx/Cargo.tomleditor/motiongfx_editor/Cargo.tomleditor/motiongfx_editor/docs/HANDOFF.mdeditor/motiongfx_editor/src/hierarchy.rseditor/motiongfx_editor/src/lib.rseditor/motiongfx_editor/src/main.rseditor/motiongfx_editor/src/playback.rseditor/motiongfx_editor/src/scene.rseditor/motiongfx_editor/src/view.rseditor/motiongfx_editor_ui/Cargo.tomleditor/motiongfx_editor_ui/src/dock.rseditor/motiongfx_editor_ui/src/dock/add_popup.rseditor/motiongfx_editor_ui/src/dock/area.rseditor/motiongfx_editor_ui/src/dock/drag.rseditor/motiongfx_editor_ui/src/dock/reconcile.rseditor/motiongfx_editor_ui/src/dock/registry.rseditor/motiongfx_editor_ui/src/dock/split.rseditor/motiongfx_editor_ui/src/dock/tabs.rseditor/motiongfx_editor_ui/src/dock/tree.rseditor/motiongfx_editor_ui/src/glass.rseditor/motiongfx_editor_ui/src/glass/backdrop.rseditor/motiongfx_editor_ui/src/glass/material.rseditor/motiongfx_editor_ui/src/glass/preset.rseditor/motiongfx_editor_ui/src/inspector.rseditor/motiongfx_editor_ui/src/lib.rseditor/motiongfx_editor_ui/src/reactive.rseditor/motiongfx_editor_ui_kernel/Cargo.tomleditor/motiongfx_editor_ui_kernel/README.mdeditor/motiongfx_editor_ui_kernel/src/host.rseditor/motiongfx_editor_ui_kernel/src/lib.rseditor/motiongfx_editor_ui_kernel/src/ui.rsexamples/bevy_examples/examples/dock_demo.rs
💤 Files with no reviewable changes (1)
- editor/motiongfx_editor/src/main.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- editor/motiongfx_editor_ui/src/glass.rs
- editor/motiongfx_editor_ui/src/dock.rs
- editor/motiongfx_editor_ui/src/glass/preset.rs
- editor/motiongfx_editor_ui/src/glass/material.rs
- editor/motiongfx_editor_ui/src/dock/split.rs
- editor/motiongfx_editor_ui/src/inspector.rs
- editor/motiongfx_editor/src/scene.rs
- editor/motiongfx_editor_ui/src/dock/tree.rs
- editor/motiongfx_editor_ui/src/dock/drag.rs
Three were stale after the kernel refactor (Ui::watch, BevyUiExt::bind_field, view::fit_preview_image); the rest linked to private modules, which rustdoc reports either way.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@editor/motiongfx_editor/src/scene.rs`:
- Around line 249-258: Update the track box container in the timeline UI to use
normal flex-column layout instead of PositionType::Absolute, preserving the top
padding through standard layout properties. In build_track_boxes, remove
absolute-position offsets and calculate each track’s vertical placement through
bottom margins or equivalent spacing so generated boxes remain stacked visually
while contributing to TimelineContent’s height and enabling TrackViewport and
NamePanel scrolling.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4fe7c881-f19a-4921-ade1-6551900c0976
📒 Files selected for processing (6)
editor/motiongfx_editor/src/lib.rseditor/motiongfx_editor/src/main.rseditor/motiongfx_editor/src/scene.rseditor/motiongfx_editor_ui/src/glass.rseditor/motiongfx_editor_ui/src/glass/preset.rseditor/motiongfx_editor_ui/src/reactive.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- editor/motiongfx_editor_ui/src/glass.rs
- editor/motiongfx_editor/src/main.rs
- editor/motiongfx_editor_ui/src/glass/preset.rs
- editor/motiongfx_editor/src/lib.rs
- editor/motiongfx_editor_ui/src/reactive.rs
- registry: `register` replaces in place on a duplicate id instead of leaving a stale descriptor that `iter` still yields (+tests) - tree: compensate the insertion index for same-leaf forward reorders, which the pre-removal index otherwise overshoots by one (+tests) - playback: toggle one shared play/pause state rather than inverting each player, and clear `Scrubbing` on a plain `Release` since a click without a drag never fires `DragEnd` - view: floor `physical_size` at 1px so the aspect ratio can't divide by zero - glass: cache the backdrop image id in the binding predicate so a new handle repropagates even when the rect is unchanged
This was referenced Aug 4, 2026
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
motiongfx_editor_uimotiongfx_editor_ui_kernelmotiongfx_editorintoeditor/folder, and make it a binary instead of a lib.editor.rsexample and move its content tomotiongfx_editor/src/main.rsas a stub content until we have proper serialization implementation.Relates to #70 & #89