Open
Conversation
- Move `BakeFnPtr`/`SampleFnPtr` into `pipeline/func_pointers` submodule - Move all registry into a new `registry.rs` file
nixonyh
added
documentation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Member
Author
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughUnifies runtime around a type‑erased Registry and world‑generic timelines (Timeline). Introduces SubjectSource for world read/write, replaces closure pipelines with function‑pointer pipelines, moves Bevy integration from MotionGfxWorld to MotionGfxManager, updates builders to use FieldAccessor/path! and updates docs/examples. Workspace dependency bumped. ChangesRegistry & Bevy manager rework
Sequence Diagram(s)sequenceDiagram
participant Builder as TimelineBuilder<'_, W>
participant Registry as Registry
participant PipelineReg as PipelineRegistry (untyped)
participant Pipeline as PipelineUntyped
participant BakeFn as BakeFnPtr
participant World as SubjectSource (W)
Builder->>Registry: register::<W,I,S,T>(FieldAccessor)
Registry->>PipelineReg: ensure PipelineKey::<W,I,S,T>
Builder->>PipelineReg: request pipeline by key
PipelineReg-->>Pipeline: return PipelineUntyped
Pipeline->>BakeFn: invoke .bake::<W>(BakeCtx{ world, accessor_registry, ... })
BakeFn->>World: get_source(id) / apply_source(id, f)
World-->>BakeFn: source read / target written
BakeFn-->>Pipeline: bake/sample complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/motiongfx/src/timeline.rs (1)
508-534:⚠️ Potential issue | 🟠 Major
try_compile()evaluatescompile()eagerly even when tracks are empty, allowing release builds to construct an invalid Timeline.
then_some(self.compile())eagerly evaluatesself.compile()beforethen_some()executes. Whenself.tracks.is_empty()is true:
- Debug builds: hits
debug_assert!as expected- Release builds:
debug_assert!is a no-op;compile()constructs and returns aTimeline<W>with empty tracks (later discarded), violating the documented invariant that other methods likecurr_track()andset_target_time()depend onThe documented panic contract is not enforced in release builds.
Suggested fix
pub fn compile(self) -> Timeline<W> { - // TODO(nixon): What happens when track is empty? - debug_assert!( + assert!( !self.tracks.is_empty(), "Track cannot be empty!" ); Timeline { action_world: self.action_world, @@ /// Similar to [`Self::compile()`] but return `None` instead of /// panicking. pub fn try_compile(self) -> Option<Timeline<W>> { - (!self.tracks.is_empty()).then_some(self.compile()) + if self.tracks.is_empty() { + None + } else { + Some(self.compile()) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/motiongfx/src/timeline.rs` around lines 508 - 534, try_compile currently calls compile() eagerly via then_some(self.compile()), which constructs an invalid Timeline in release builds because debug_assert! is a no-op; replace the eager call with a lazy one so compile() is only invoked when tracks is non-empty—i.e. change the return expression in try_compile from (!self.tracks.is_empty()).then_some(self.compile()) to a lazy variant such as (!self.tracks.is_empty()).then(|| self.compile()) or an explicit if self.tracks.is_empty() { None } else { Some(self.compile()) }, referencing try_compile, compile, self.tracks and debug_assert! to locate the code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/motiongfx/src/action.rs`:
- Around line 124-127: The crate currently uses const TypeId::of::<I>() inside
const fn new<I: SubjectId> in action.rs which requires Rust 1.91.0; update this
crate's Cargo.toml to declare the minimum supported Rust version by adding
rust-version = "1.91.0" (or the workspace Cargo.toml if you manage MSRV at
workspace level) so consumers using older compilers won't fail to build; ensure
the change is added to the same crate that defines Action::new (the const fn
using TypeId::of::<I>()).
In `@crates/motiongfx/src/pipeline.rs`:
- Around line 153-163: PipelineUntyped::bake and ::sample unsafely cast erased
function pointers based solely on the caller-provided generic W (unsafe contract
unenforced); fix by enforcing a runtime type check or changing API safety:
either store the original TypeId when creating PipelineUntyped and validate it
against TypeId::of::<W>() before doing the unsafe typed_unchecked call, or mark
bake/sample as unsafe and document the caller responsibility, or restrict
visibility so callers obtain a typed Pipeline<W> (e.g., make
PipelineRegistry::get private or return Pipeline<W>), and update
PipelineRegistry::get, PipelineUntyped, and the creation code to include the
chosen approach.
---
Outside diff comments:
In `@crates/motiongfx/src/timeline.rs`:
- Around line 508-534: try_compile currently calls compile() eagerly via
then_some(self.compile()), which constructs an invalid Timeline in release
builds because debug_assert! is a no-op; replace the eager call with a lazy one
so compile() is only invoked when tracks is non-empty—i.e. change the return
expression in try_compile from
(!self.tracks.is_empty()).then_some(self.compile()) to a lazy variant such as
(!self.tracks.is_empty()).then(|| self.compile()) or an explicit if
self.tracks.is_empty() { None } else { Some(self.compile()) }, referencing
try_compile, compile, self.tracks and debug_assert! to locate the code to
change.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8f6710ce-a886-4cae-ab3d-ad2793ac1b69
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
Cargo.tomlcrates/bevy_motiongfx/README.mdcrates/bevy_motiongfx/src/interpolation.rscrates/bevy_motiongfx/src/lib.rscrates/bevy_motiongfx/src/pipeline.rscrates/bevy_motiongfx/src/registry.rscrates/bevy_motiongfx/src/world.rscrates/motiongfx/README.mdcrates/motiongfx/examples/custom_world.rscrates/motiongfx/src/action.rscrates/motiongfx/src/lib.rscrates/motiongfx/src/pipeline.rscrates/motiongfx/src/pipeline/func_pointers.rscrates/motiongfx/src/registry.rscrates/motiongfx/src/timeline.rscrates/motiongfx/src/track.rsexamples/bevy_examples/examples/custom_ease.rsexamples/bevy_examples/examples/custom_interp.rsexamples/bevy_examples/examples/easings.rsexamples/bevy_examples/examples/hello_world.rsexamples/bevy_examples/examples/minimal.rsexamples/bevy_examples/examples/recording.rsexamples/bevy_examples/examples/slide_basic.rs
💤 Files with no reviewable changes (1)
- crates/bevy_motiongfx/src/registry.rs
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/motiongfx/src/timeline.rs (1)
509-513:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
compile()does not uphold its own panic contract in release builds.Line 510 uses
debug_assert!, socompile()accepts empty tracks in release despite docs saying it panics. That creates an invalidTimelinestate and shifts failure to later indexing paths.Suggested fix
- // TODO(nixon): What happens when track is empty? - debug_assert!( + assert!( !self.tracks.is_empty(), - "Track cannot be empty!" + "Track cannot be empty! Use try_compile() when empty tracks are possible." );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/motiongfx/src/timeline.rs` around lines 509 - 513, The compile() function currently uses debug_assert! to check for empty tracks, so in release builds it can accept empty tracks contrary to its documented panic contract; update the check in Timeline::compile (the debug_assert!(!self.tracks.is_empty(), "Track cannot be empty!")) to enforce the invariant in all builds (replace with assert! or an explicit panic/early return that triggers on self.tracks.is_empty()) so an empty tracks vector cannot produce an invalid Timeline state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/motiongfx/src/timeline.rs`:
- Around line 50-52: The code currently silently skips when
registry.pipeline.get(key) returns None; instead enforce the registry invariant
by failing fast: replace the `continue` with an explicit panic or error return
(e.g., panic! or Err) that includes the missing key and context (e.g., "missing
pipeline for key {:?}"), and do the same for the other occurrence where
registry.pipeline.get(key) is used; ensure the error message references
`registry.pipeline.get(key)` / the pipeline key so diagnostics point to the
exact missing registry entry.
---
Outside diff comments:
In `@crates/motiongfx/src/timeline.rs`:
- Around line 509-513: The compile() function currently uses debug_assert! to
check for empty tracks, so in release builds it can accept empty tracks contrary
to its documented panic contract; update the check in Timeline::compile (the
debug_assert!(!self.tracks.is_empty(), "Track cannot be empty!")) to enforce the
invariant in all builds (replace with assert! or an explicit panic/early return
that triggers on self.tracks.is_empty()) so an empty tracks vector cannot
produce an invalid Timeline state.
🪄 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: 15d97e65-73c4-4a1e-b43b-b66e7082e19a
📒 Files selected for processing (1)
crates/motiongfx/src/timeline.rs
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
examples/bevy_examples/examples/minimal.rs (1)
4-4:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUpdate the
TimelineCompleteimport path.
TimelineCompleteno longer exists inbevy_motiongfx::world; it is defined in the new manager module, so this example does not compile as written.Suggested fix
-use bevy_motiongfx::world::TimelineComplete; +use bevy_motiongfx::manager::TimelineComplete;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/bevy_examples/examples/minimal.rs` at line 4, The example imports TimelineComplete from the old module; update the use statement to import TimelineComplete from the new manager module (replace bevy_motiongfx::world::TimelineComplete with bevy_motiongfx::manager::TimelineComplete) so the example compiles with the relocated symbol.examples/bevy_examples/examples/recording.rs (1)
8-8:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix the stale
TimelineCompletemodule path.After this refactor,
TimelineCompleteis no longer exported frombevy_motiongfx::world, so this import breaks the example build.Suggested fix
-use bevy_motiongfx::world::TimelineComplete; +use bevy_motiongfx::manager::TimelineComplete;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/bevy_examples/examples/recording.rs` at line 8, The import use bevy_motiongfx::world::TimelineComplete is stale and should be updated: locate where TimelineComplete is now exported in the crate (search the codebase for the TimelineComplete type) and replace the broken import with the correct module path (or remove the import and reference the type with its new fully-qualified path where used). Ensure the example references the TimelineComplete symbol from its current export location instead of bevy_motiongfx::world.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/bevy_motiongfx/src/manager.rs`:
- Around line 97-111: The add_timeline method accepts any BevyTimeline which
allows timelines from a different Registry to be inserted and later used in
load_pending_timelines()/sample_timelines(), violating registry affinity; make
MotionGfxManager the only producer of insertable timelines by either (a)
changing add_timeline to accept a builder-produced wrapper/token type (e.g., a
ManagerOwnedBevyTimeline or TimelineHandle returned by
create_builder()/create_bevy_builder) instead of raw BevyTimeline, or (b) attach
and verify a Registry token on BevyTimeline and check that token inside
add_timeline (compare against self.registry’s token) and reject/move/convert
mismatched timelines; update create_builder/create_bevy_builder to produce the
token-wrapped timeline type or provide a creation API that returns the
manager-bound timeline so cross-registry timelines cannot be passed into
add_timeline.
In `@crates/motiongfx/src/pipeline.rs`:
- Around line 168-175: The types BakeCtx and SampleCtx are declared with a
lifetime parameter (BakeCtx<'a, W> / SampleCtx<'a, W>) but are used without the
lifetime in function signatures; update all usages (e.g., the bake function
signature `pub fn bake<W, I, S, T>(ctx: BakeCtx<W>)` and any `SampleCtx<W>`
usages around line 223) to include the appropriate lifetime parameter (e.g.,
`BakeCtx<'a, W>` / `SampleCtx<'a, W>` or introduce a matching generic lifetime
like `<'a, W>` on the function) so the declared structs and their uses match and
compile under stable Rust.
---
Outside diff comments:
In `@examples/bevy_examples/examples/minimal.rs`:
- Line 4: The example imports TimelineComplete from the old module; update the
use statement to import TimelineComplete from the new manager module (replace
bevy_motiongfx::world::TimelineComplete with
bevy_motiongfx::manager::TimelineComplete) so the example compiles with the
relocated symbol.
In `@examples/bevy_examples/examples/recording.rs`:
- Line 8: The import use bevy_motiongfx::world::TimelineComplete is stale and
should be updated: locate where TimelineComplete is now exported in the crate
(search the codebase for the TimelineComplete type) and replace the broken
import with the correct module path (or remove the import and reference the type
with its new fully-qualified path where used). Ensure the example references the
TimelineComplete symbol from its current export location instead of
bevy_motiongfx::world.
🪄 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: 01e529a1-3cfd-493f-8cb1-f51aebf720f0
📒 Files selected for processing (14)
crates/bevy_motiongfx/README.mdcrates/bevy_motiongfx/src/controller.rscrates/bevy_motiongfx/src/lib.rscrates/bevy_motiongfx/src/manager.rscrates/bevy_motiongfx/src/pipeline.rscrates/bevy_motiongfx/src/world.rscrates/motiongfx/src/pipeline.rsexamples/bevy_examples/examples/custom_ease.rsexamples/bevy_examples/examples/custom_interp.rsexamples/bevy_examples/examples/easings.rsexamples/bevy_examples/examples/hello_world.rsexamples/bevy_examples/examples/minimal.rsexamples/bevy_examples/examples/recording.rsexamples/bevy_examples/examples/slide_basic.rs
💤 Files with no reviewable changes (1)
- crates/bevy_motiongfx/src/pipeline.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- examples/bevy_examples/examples/custom_ease.rs
- crates/bevy_motiongfx/README.md
- examples/bevy_examples/examples/slide_basic.rs
- crates/bevy_motiongfx/src/lib.rs
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
✅ Created PR with unit tests: #102 |
Jaghov
reviewed
May 2, 2026
Jaghov
reviewed
May 2, 2026
Jaghov
reviewed
May 2, 2026
Jaghov
reviewed
May 2, 2026
Jaghov
reviewed
May 2, 2026
Jaghov
reviewed
May 2, 2026
nixonyh
force-pushed
the
nixon/lazy-registration
branch
2 times, most recently
from
8c65031 to
d699aa0
Compare
nixonyh
force-pushed
the
nixon/lazy-registration
branch
from
d699aa0 to
f95e9a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/motiongfx/src/timeline.rs (1)
514-526:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce the non-empty-track invariant in release builds too.
Line 523 uses
debug_assert!, socompile()can still produce an emptyTimeline<W>in release builds even though the rest of the type assumes at least one track. That turns the documented failure into later panics in methods likelast_track_index(),set_target_time(), andis_complete().Proposed fix
- debug_assert!( + assert!( !self.tracks.is_empty(), "Track cannot be empty!" );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/motiongfx/src/timeline.rs` around lines 514 - 526, The compile() method currently uses debug_assert! which is disabled in release builds, allowing an empty Timeline to be created; change the check to enforce the invariant in all builds by replacing debug_assert! with a hard assertion or explicit panic (e.g., assert!(!self.tracks.is_empty(), "Track cannot be empty!") or if self.tracks.is_empty() { panic!("Track cannot be empty!") }) inside Timeline::compile so it panics as documented when tracks is empty; keep mention of Self::try_compile in the docs if you want a non-panicking alternative.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/bevy_motiongfx/src/manager.rs`:
- Around line 102-107: The add_timeline() path currently wraps the incoming
BevyTimeline in MutDetect::new which clears its mutation flag and causes
preconfigured timelines to be skipped by sample_timelines(); change
add_timeline() so the MutDetect wrapper is marked mutated on insert (e.g.,
construct or update the MutDetect for the timeline in pending_timelines so its
mutated() returns true) so load_pending_timelines() will bake the timeline and
sample_timelines() will process it; reference pending_timelines, add_timeline,
MutDetect, load_pending_timelines, and sample_timelines when making the change.
In `@crates/motiongfx/README.md`:
- Around line 333-334: The README uses absolute-root links "/LICENSE-MIT" and
"/LICENSE-APACHE" which can break in rendered docs; update the license links to
be repository-relative (e.g., "LICENSE-MIT" and "LICENSE-APACHE" or
"./LICENSE-MIT" and "./LICENSE-APACHE") so the link targets "LICENSE-MIT" and
"LICENSE-APACHE" resolve correctly in all contexts; change the two occurrences
that reference "LICENSE-MIT" and "LICENSE-APACHE" in the README header lines
accordingly.
In `@crates/motiongfx/src/pipeline.rs`:
- Around line 61-108: PipelineKey currently omits the orphan-rule marker `M`
used by the SubjectSource trait, causing different marker impls to collide; fix
this by adding a marker_id: TypeId field to PipelineKey and populate it from the
marker type in constructors: update struct PipelineKey to include marker_id:
TypeId, change PipelineKey::new to accept an M: 'static generic and set
marker_id = TypeId::of::<M>(), update PipelineKey::from_action_key to be generic
over M: 'static and set marker_id = TypeId::of::<M>(), and add a pub(crate) fn
marker_id(&self) -> TypeId accessor; this ensures register::<W,I,S,T,M1>() and
register::<W,I,S,T,M2>() produce distinct keys.
In `@examples/bevy_examples/examples/hello_world.rs`:
- Around line 58-62: The index calculation for the grid is using the wrong
stride: replace the expression c = w * WIDTH + h with c = w * HEIGHT + h so the
computed index into cube_ids uses the correct row stride; update the loop that
iterates w and h (variables w, h and constants WIDTH, HEIGHT, and the cube_ids
access/cube_id variable) accordingly to ensure correct indexing when WIDTH !=
HEIGHT.
---
Outside diff comments:
In `@crates/motiongfx/src/timeline.rs`:
- Around line 514-526: The compile() method currently uses debug_assert! which
is disabled in release builds, allowing an empty Timeline to be created; change
the check to enforce the invariant in all builds by replacing debug_assert! with
a hard assertion or explicit panic (e.g., assert!(!self.tracks.is_empty(),
"Track cannot be empty!") or if self.tracks.is_empty() { panic!("Track cannot be
empty!") }) inside Timeline::compile so it panics as documented when tracks is
empty; keep mention of Self::try_compile in the docs if you want a non-panicking
alternative.
🪄 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: a72279ea-68d3-4c84-9488-b0b77e73b94f
📒 Files selected for processing (17)
crates/bevy_motiongfx/README.mdcrates/bevy_motiongfx/src/interpolation.rscrates/bevy_motiongfx/src/manager.rscrates/bevy_motiongfx/src/world.rscrates/motiongfx/README.mdcrates/motiongfx/docs/world.rscrates/motiongfx/examples/custom_world.rscrates/motiongfx/src/pipeline.rscrates/motiongfx/src/registry.rscrates/motiongfx/src/timeline.rsexamples/bevy_examples/examples/custom_ease.rsexamples/bevy_examples/examples/custom_interp.rsexamples/bevy_examples/examples/easings.rsexamples/bevy_examples/examples/hello_world.rsexamples/bevy_examples/examples/minimal.rsexamples/bevy_examples/examples/recording.rsexamples/bevy_examples/examples/slide_basic.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/bevy_motiongfx/README.md
- examples/bevy_examples/examples/easings.rs
- crates/bevy_motiongfx/src/interpolation.rs
Comment on lines
+102
to
+107
| pub fn add_timeline( | ||
| &mut self, | ||
| timeline: BevyTimeline, | ||
| ) -> TimelineId { | ||
| let id = self.id; | ||
| self.pending_timelines.insert(id, MutDetect::new(timeline)); |
There was a problem hiding this comment.
Mark inserted timelines as dirty.
Line 107 wraps the incoming timeline in MutDetect::new, which resets the mutation flag even if the caller already changed the timeline before add_timeline(). A timeline preconfigured with set_target_time or set_target_track will be baked in load_pending_timelines() and then skipped by sample_timelines() because only mutated() entries are sampled.
Proposed fix
pub fn add_timeline(
&mut self,
timeline: BevyTimeline,
) -> TimelineId {
let id = self.id;
- self.pending_timelines.insert(id, MutDetect::new(timeline));
+ let mut timeline = MutDetect::new(timeline);
+ timeline.mutated = true;
+ self.pending_timelines.insert(id, timeline);
self.id.0 = self.id.0.wrapping_add(1);
id
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/bevy_motiongfx/src/manager.rs` around lines 102 - 107, The
add_timeline() path currently wraps the incoming BevyTimeline in MutDetect::new
which clears its mutation flag and causes preconfigured timelines to be skipped
by sample_timelines(); change add_timeline() so the MutDetect wrapper is marked
mutated on insert (e.g., construct or update the MutDetect for the timeline in
pending_timelines so its mutated() returns true) so load_pending_timelines()
will bake the timeline and sample_timelines() will process it; reference
pending_timelines, add_timeline, MutDetect, load_pending_timelines, and
sample_timelines when making the change.
Comment on lines
+333
to
+334
| - MIT License ([LICENSE-MIT](/LICENSE-MIT) or [http://opensource.org/licenses/MIT](http://opensource.org/licenses/MIT)) | ||
| - Apache License, Version 2.0 ([LICENSE-APACHE](/LICENSE-APACHE) or [http://www.apache.org/licenses/LICENSE-2.0](http://www.apache.org/licenses/LICENSE-2.0)) |
There was a problem hiding this comment.
License links are brittle due absolute-root paths.
Line 333 and Line 334 use /LICENSE-*, which can resolve incorrectly in rendered docs. Prefer repo-relative links.
Proposed fix
-- MIT License ([LICENSE-MIT](/LICENSE-MIT) or [http://opensource.org/licenses/MIT](http://opensource.org/licenses/MIT))
-- Apache License, Version 2.0 ([LICENSE-APACHE](/LICENSE-APACHE) or [http://www.apache.org/licenses/LICENSE-2.0](http://www.apache.org/licenses/LICENSE-2.0))
+- MIT License ([LICENSE-MIT](LICENSE-MIT) or [http://opensource.org/licenses/MIT](http://opensource.org/licenses/MIT))
+- Apache License, Version 2.0 ([LICENSE-APACHE](LICENSE-APACHE) or [http://www.apache.org/licenses/LICENSE-2.0](http://www.apache.org/licenses/LICENSE-2.0))📝 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
| - MIT License ([LICENSE-MIT](/LICENSE-MIT) or [http://opensource.org/licenses/MIT](http://opensource.org/licenses/MIT)) | |
| - Apache License, Version 2.0 ([LICENSE-APACHE](/LICENSE-APACHE) or [http://www.apache.org/licenses/LICENSE-2.0](http://www.apache.org/licenses/LICENSE-2.0)) | |
| - MIT License ([LICENSE-MIT](LICENSE-MIT) or [http://opensource.org/licenses/MIT](http://opensource.org/licenses/MIT)) | |
| - Apache License, Version 2.0 ([LICENSE-APACHE](LICENSE-APACHE) or [http://www.apache.org/licenses/LICENSE-2.0](http://www.apache.org/licenses/LICENSE-2.0)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/motiongfx/README.md` around lines 333 - 334, The README uses
absolute-root links "/LICENSE-MIT" and "/LICENSE-APACHE" which can break in
rendered docs; update the license links to be repository-relative (e.g.,
"LICENSE-MIT" and "LICENSE-APACHE" or "./LICENSE-MIT" and "./LICENSE-APACHE") so
the link targets "LICENSE-MIT" and "LICENSE-APACHE" resolve correctly in all
contexts; change the two occurrences that reference "LICENSE-MIT" and
"LICENSE-APACHE" in the README header lines accordingly.
Comment on lines
58
to
62
| for w in 0..WIDTH { | ||
| for h in 0..HEIGHT { | ||
| let c = w * WIDTH + h; | ||
| let cube = cubes[c]; | ||
| let cube_id = cube_ids[c]; | ||
|
|
There was a problem hiding this comment.
Fix grid index calculation for non-square dimensions.
Line 60 uses w * WIDTH + h; this should use the row stride (HEIGHT) to stay correct if dimensions diverge.
Proposed fix
- let c = w * WIDTH + h;
+ let c = w * HEIGHT + h;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/bevy_examples/examples/hello_world.rs` around lines 58 - 62, The
index calculation for the grid is using the wrong stride: replace the expression
c = w * WIDTH + h with c = w * HEIGHT + h so the computed index into cube_ids
uses the correct row stride; update the loop that iterates w and h (variables w,
h and constants WIDTH, HEIGHT, and the cube_ids access/cube_id variable)
accordingly to ensure correct indexing when WIDTH != HEIGHT.
There was a problem hiding this comment.
♻️ Duplicate comments (4)
crates/motiongfx/src/timeline.rs (2)
517-525:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce the non-empty-track invariant in release builds.
compile()documents a panic here, butdebug_assert!disappears in release and still builds aTimeline<W>that later indexesself.tracks[0]. This should be an unconditionalassert!or atry_compile().expect(...).Suggested fix
- debug_assert!( - !self.tracks.is_empty(), - "Track cannot be empty!" - ); + assert!( + !self.tracks.is_empty(), + "Track cannot be empty! Use `try_compile()` if empty tracks are expected." + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/motiongfx/src/timeline.rs` around lines 517 - 525, The compile() function currently uses debug_assert! to check !self.tracks.is_empty(), which is removed in release builds and leads to indexing self.tracks[0] later; change this to enforce the invariant unconditionally by either replacing debug_assert! with assert!(!self.tracks.is_empty(), "Track cannot be empty!") or by implementing compile() as self.try_compile().expect("Track cannot be empty!") so the empty-track case always panics in release and debug; update the compile() body around the existing check and the call sites to use the chosen approach (functions: compile, try_compile, and access to self.tracks).
50-62:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast on missing pipelines instead of only
debug_assert!ing.In release builds this becomes a silent no-op, so a registry mismatch just drops baking/sampling work. Given this API now depends on registry affinity, this should be a hard failure or an error return, not a debug-only check.
Suggested fix
- let ok = registry.pipeline.bake( + let ok = registry.pipeline.bake( key, BakeCtx { world: subject_world, track, action_world: &mut self.action_world, accessor_registry: ®istry.accessor, }, ); - debug_assert!( - ok, - "pipeline not found for key {key:?}" - ); + assert!(ok, "pipeline not found for key {key:?}");- let ok = registry.pipeline.sample( + let ok = registry.pipeline.sample( key, SampleCtx { world: subject_world, action_world: &self.action_world, accessor_registry: ®istry.accessor, }, ); - debug_assert!(ok, "pipeline not found for key {key:?}"); + assert!(ok, "pipeline not found for key {key:?}");Also applies to: 237-245
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/motiongfx/src/timeline.rs` around lines 50 - 62, The code currently calls registry.pipeline.bake(...) and only does a debug_assert!(ok, ...) which becomes a no-op in release; change this to a hard failure or proper error propagation: if the surrounding function can return a Result, change its signature to return Result<..., Error> and propagate the failure from registry.pipeline.bake by returning an Err with a clear message including key (replace the debug_assert! after the bake call with return Err(format!("pipeline not found for key {:?}", key).into())); if the surrounding API cannot return a Result, replace the debug_assert! with an unconditional panic!(...) that includes the same message. Apply the same change to the other occurrence around lines 237-245 where registry.pipeline.bake / debug_assert! is used so missing pipelines never silently drop work.crates/bevy_motiongfx/src/manager.rs (1)
102-107:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMark newly added timelines as mutated.
Wrapping the incoming timeline with
MutDetect::new(...)clears its dirty flag, so preconfigured timelines can be baked and then skipped bysample_timelines()on their first pass.Suggested fix
pub fn add_timeline( &mut self, timeline: BevyTimeline, ) -> TimelineId { let id = self.id; - self.pending_timelines.insert(id, MutDetect::new(timeline)); + let mut timeline = MutDetect::new(timeline); + timeline.mutated = true; + self.pending_timelines.insert(id, timeline); self.id.0 = self.id.0.wrapping_add(1); id }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/bevy_motiongfx/src/manager.rs` around lines 102 - 107, The new timeline is wrapped with MutDetect::new(...) which leaves it non-dirty and causes sample_timelines() to skip it; update add_timeline (and the pending_timelines insertion) to mark the inserted MutDetect as mutated (e.g., create it with the MutDetect variant/constructor that marks dirty or call its mark_mutated()/set_dirty() method immediately after inserting) so the freshly added timeline is recognized as changed by sample_timelines(); touch the MutDetect instance produced for timeline in add_timeline (function name: add_timeline, collection: pending_timelines, type: MutDetect) to set its dirty flag.crates/motiongfx/README.md (1)
299-300:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse repo-relative license links.
These absolute-root paths can break in rendered docs. Relative links are safer here.
Suggested fix
-- MIT License ([LICENSE-MIT](/LICENSE-MIT) or [http://opensource.org/licenses/MIT](http://opensource.org/licenses/MIT)) -- Apache License, Version 2.0 ([LICENSE-APACHE](/LICENSE-APACHE) or [http://www.apache.org/licenses/LICENSE-2.0](http://www.apache.org/licenses/LICENSE-2.0)) +- MIT License ([LICENSE-MIT](LICENSE-MIT) or [http://opensource.org/licenses/MIT](http://opensource.org/licenses/MIT)) +- Apache License, Version 2.0 ([LICENSE-APACHE](LICENSE-APACHE) or [http://www.apache.org/licenses/LICENSE-2.0](http://www.apache.org/licenses/LICENSE-2.0))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/motiongfx/README.md` around lines 299 - 300, Update the two license markdown links so they use repo-relative paths instead of absolute-root paths: replace the link targets in the lines containing "MIT License ([LICENSE-MIT](/LICENSE-MIT)..." and "Apache License, Version 2.0 ([LICENSE-APACHE](/LICENSE-APACHE)..." with relative targets (e.g. "./LICENSE-MIT" or "LICENSE-MIT" and "./LICENSE-APACHE" or "LICENSE-APACHE") so the README uses repo-relative links that won’t break in rendered docs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/bevy_motiongfx/src/manager.rs`:
- Around line 102-107: The new timeline is wrapped with MutDetect::new(...)
which leaves it non-dirty and causes sample_timelines() to skip it; update
add_timeline (and the pending_timelines insertion) to mark the inserted
MutDetect as mutated (e.g., create it with the MutDetect variant/constructor
that marks dirty or call its mark_mutated()/set_dirty() method immediately after
inserting) so the freshly added timeline is recognized as changed by
sample_timelines(); touch the MutDetect instance produced for timeline in
add_timeline (function name: add_timeline, collection: pending_timelines, type:
MutDetect) to set its dirty flag.
In `@crates/motiongfx/README.md`:
- Around line 299-300: Update the two license markdown links so they use
repo-relative paths instead of absolute-root paths: replace the link targets in
the lines containing "MIT License ([LICENSE-MIT](/LICENSE-MIT)..." and "Apache
License, Version 2.0 ([LICENSE-APACHE](/LICENSE-APACHE)..." with relative
targets (e.g. "./LICENSE-MIT" or "LICENSE-MIT" and "./LICENSE-APACHE" or
"LICENSE-APACHE") so the README uses repo-relative links that won’t break in
rendered docs.
In `@crates/motiongfx/src/timeline.rs`:
- Around line 517-525: The compile() function currently uses debug_assert! to
check !self.tracks.is_empty(), which is removed in release builds and leads to
indexing self.tracks[0] later; change this to enforce the invariant
unconditionally by either replacing debug_assert! with
assert!(!self.tracks.is_empty(), "Track cannot be empty!") or by implementing
compile() as self.try_compile().expect("Track cannot be empty!") so the
empty-track case always panics in release and debug; update the compile() body
around the existing check and the call sites to use the chosen approach
(functions: compile, try_compile, and access to self.tracks).
- Around line 50-62: The code currently calls registry.pipeline.bake(...) and
only does a debug_assert!(ok, ...) which becomes a no-op in release; change this
to a hard failure or proper error propagation: if the surrounding function can
return a Result, change its signature to return Result<..., Error> and propagate
the failure from registry.pipeline.bake by returning an Err with a clear message
including key (replace the debug_assert! after the bake call with return
Err(format!("pipeline not found for key {:?}", key).into())); if the surrounding
API cannot return a Result, replace the debug_assert! with an unconditional
panic!(...) that includes the same message. Apply the same change to the other
occurrence around lines 237-245 where registry.pipeline.bake / debug_assert! is
used so missing pipelines never silently drop work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1382d8e2-770e-4cd2-a56a-2f6a52a9f09d
📒 Files selected for processing (11)
crates/bevy_motiongfx/src/interpolation.rscrates/bevy_motiongfx/src/manager.rscrates/bevy_motiongfx/src/world.rscrates/motiongfx/README.mdcrates/motiongfx/docs/world.rscrates/motiongfx/examples/custom_world.rscrates/motiongfx/src/lib.rscrates/motiongfx/src/pipeline.rscrates/motiongfx/src/registry.rscrates/motiongfx/src/timeline.rscrates/motiongfx/src/world.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/bevy_motiongfx/src/interpolation.rs
- crates/motiongfx/src/registry.rs
- crates/motiongfx/examples/custom_world.rs
- crates/motiongfx/src/lib.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
examples/bevy_examples/examples/custom_interp.rs (1)
76-76: Track thearc_lerp_3doptimization TODO.This is fine to land as-is, but the inline TODO is easy to lose. If you want, I can help turn it into a tracked follow-up or sketch a tighter implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/bevy_examples/examples/custom_interp.rs` at line 76, Replace the loose "// TODO(nixon): Optimize this." comment for arc_lerp_3d with a tracked follow-up: open a short issue describing the performance goal and proposed approach for optimizing arc_lerp_3d, then update the comment to reference that issue number or URL (e.g., "TODO(nixon): optimize arc_lerp_3d — tracked in issue `#NNN`") so the work isn't lost; optionally add the issue to the project board or changelog to ensure follow-up.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@examples/bevy_examples/examples/custom_interp.rs`:
- Line 76: Replace the loose "// TODO(nixon): Optimize this." comment for
arc_lerp_3d with a tracked follow-up: open a short issue describing the
performance goal and proposed approach for optimizing arc_lerp_3d, then update
the comment to reference that issue number or URL (e.g., "TODO(nixon): optimize
arc_lerp_3d — tracked in issue `#NNN`") so the work isn't lost; optionally add the
issue to the project board or changelog to ensure follow-up.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0313531a-140c-4bc7-84f5-9133eba23bc4
📒 Files selected for processing (1)
examples/bevy_examples/examples/custom_interp.rs
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
2 participants
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 used to require you to register all fields and pipelines before creating any animations.
Closes #98 (This PR aims to address it too!)
Now it's GONE!
This PR introduces fully autonomous
Pipeline/FieldAccessorregistration (or shud I say lazy registration) by lazily registering them during eachTimelineBuilder::act(..)calls.This is achieved through a few changes:
PipelineRegistryis nowWworld agnostic.Registrytype is added to unify bothAccessorRegistry&PipelineRegistry.PipelineRegistry::register()now does not take in any value, instead the entire creation of thePipelineKey&Pipelineis automated via theSubjectSourcetrait.SubjectSourcetrait allows thePipelineto create theBakerFn&SampleFnparameter-less.TimelineBuildernow takes in a compulsory mutable reference of a&mut Registryfor lazy registration. It is crucial that the sameRegistryis later used to bake & sample the compiledTimeline. (It is advised to just keep a globalRegistryat all times to prevent bugs / confusion)TimelineBuilder::act(..)will attempt to register the given path. If it is already registered before, we simply skip registering it.Timeline<W>&TimelineBuilder<W>is nowWbound. This ensure better correctness and preventTimelineto be multi-world, making it hard to determine which world to bake / sample from.