Serialization - #124
Merged
Merged
Conversation
motiongfx::ease only used bevy_math::ops for four f32 functions, but pulled in bevy_math's whole glam/itertools chain to get them - itertools' `either` dependency doesn't support no_std, breaking motiongfx_scene's no_std build. Replaced with a small local ops module dispatching to std's intrinsics or libm directly. Workspace-level default-features = false added for motiongfx/motiongfx_scene/bevy_motiongfx so downstream consumers' default features aren't silently forced on transitively; each now opts back in explicitly.
A backend now defines its own struct of named sparse_map::SparseMap<T> columns instead of one opaque Value type, via the new ValueColumn<T> trait. ActionCmd/Subject reference values by ValueId (a sparse_map::Key) instead of holding them inline - no wrapper types, no enum, and still plain, context-free Serialize/Deserialize since every column is just a regular Vec-backed struct field. Deletion is safe by construction: ValueId's generation counter invalidates stale references instead of leaking or shifting other actions' slots. register_op's closure also simplifies to Fn(&T) -> Box<dyn Action<T>>, since the concrete value is now resolved from the pool before op resolution runs, rather than being extracted from an opaque blob inside the closure. sparse_map gained Clone/PartialEq/serde support (patched in via [patch.crates-io] pending a crates.io release) to make this possible. TypeName is now #[serde(transparent)] for cleaner RON output.
- SceneBackend gains its own ValueId associated type, so motiongfx_scene no longer forces sparse_map on every backend. - bevy_motiongfx::scene: Backend, ValuePool, SceneId/SceneEntityMap materialization, MotionGfxScene asset + loader + plugin, spawn_scene, and MotionGfxScene::compile wiring a loaded scene into a Timeline. - scene_demo example loads a real .mgx.ron file through the asset server end to end (spawn -> compile -> play). - Trim noisy empty-vec/None serialization in ActionCmd and ValuePool. - Point the sparse_map patch at the pushed branch instead of a local path.
Add guarantees and insurance to the additional functions
EntityUid is now attached as a Component to every materialized entity, so SceneUidMap no longer needs manual insert/remove or a reverse Entity -> EntityUid map - two observers (on Add/Remove of EntityUid) keep it in sync automatically. spawn_scene returns the spawned (EntityUid, Entity) pairs directly since the map isn't queryable until commands are applied. Also fixes cube.mgx.ron, which predated the TypePath-derived field names and field_path's leading "::" convention.
SceneBackend::Id can now be an enum distinguishing subject kinds (entity vs asset, etc), converted per-field into whatever key its SubjectSource impl needs via IntoSubjectId, with a real CompileError on mismatch instead of a silent no-op. bevy_motiongfx's Backend::Id is now SceneUid (just Entity(EntityUid) for now) - an Asset variant can be added later with no other changes.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis change adds the ChangesScene pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BevyApp
participant SceneAssetLoader
participant MotionGfxScene
participant SceneRegistry
participant MotionGfxManager
BevyApp->>SceneAssetLoader: load .mgx.ron asset
SceneAssetLoader->>MotionGfxScene: deserialize scene
MotionGfxScene->>SceneRegistry: resolve fields and actions
SceneRegistry->>MotionGfxManager: register compiled timeline
BevyApp->>MotionGfxScene: stage initial values
BevyApp->>MotionGfxManager: start playback
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 5
🧹 Nitpick comments (1)
crates/bevy_motiongfx/Cargo.toml (1)
43-43: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winKeep
motiongfx_sceneoptional when enablingstd.Because
motiongfx_sceneis optional,"motiongfx_scene/std"strongly enables that dependency wheneverstdis enabled—even ifsceneis not. Use"motiongfx_scene?/std"here sostdis propagated only when another feature has already enabled the dependency. Cargo documents this distinction explicitly. (doc.rust-lang.org)-std = ["motiongfx/std", "motiongfx_scene/std", "bevy_ecs/std", "bevy_app/std", "bevy_platform/std", "bevy_time/std"] +std = ["motiongfx/std", "motiongfx_scene?/std", "bevy_ecs/std", "bevy_app/std", "bevy_platform/std", "bevy_time/std"]🤖 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 `@crates/bevy_motiongfx/Cargo.toml` at line 43, Update the std feature definition to use the optional dependency syntax for motiongfx_scene, changing its feature propagation to motiongfx_scene?/std while preserving all other feature entries unchanged.
🤖 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 `@Cargo.toml`:
- Around line 52-54: Update the [patch.crates-io] sparse_map override to use the
immutable revision 4a34aa3ccd903440ffe361a1d19933d060cc17d2 instead of branch
nixon/serialization, and retain the temporary patch until the serde feature is
published.
In `@crates/bevy_motiongfx/src/scene/id.rs`:
- Around line 120-133: Update the EntityUid insertion and removal handlers
around on_remove_entity_uid so duplicate UIDs are rejected rather than
overwriting an existing mapping, while preserving the original entity’s
association. During removal, only delete the UID entry when the stored entity
matches trigger.entity.
In `@crates/motiongfx_scene/src/compile.rs`:
- Around line 45-47: Add a dedicated CompileError::BuildFailed variant for
failures returned by builder.try_compile(), and map that failure directly to the
new variant instead of constructing an empty UnknownField. Update the
CompileError Display implementation in error.rs to provide an appropriate
message for BuildFailed.
In `@crates/motiongfx_scene/src/duration.rs`:
- Around line 25-30: Update the duration representation logic around Repr and
the duration serialization path to avoid truncating or saturating values that
exceed u64 millisecond or nanosecond limits. Add an overflow-safe
seconds-and-nanoseconds representation for oversized durations, preserve
existing Ms/Ns decoding compatibility, and add coverage for both overflow
boundaries while retaining exact round-tripping.
In `@examples/vello_winit_example/Cargo.toml`:
- Line 10: Remove the unsupported default-features override from the inherited
motiongfx dependency in the package manifest. Preserve workspace inheritance by
using only supported workspace dependency options, or define motiongfx directly
with the required defaults if explicitly enabling default features is necessary.
---
Nitpick comments:
In `@crates/bevy_motiongfx/Cargo.toml`:
- Line 43: Update the std feature definition to use the optional dependency
syntax for motiongfx_scene, changing its feature propagation to
motiongfx_scene?/std while preserving all other feature entries unchanged.
🪄 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: 60cfd5ca-131a-4026-af81-3ddaebb4fa44
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
Cargo.tomlcrates/bevy_motiongfx/Cargo.tomlcrates/bevy_motiongfx/src/lib.rscrates/bevy_motiongfx/src/manager.rscrates/bevy_motiongfx/src/scene.rscrates/bevy_motiongfx/src/scene/asset.rscrates/bevy_motiongfx/src/scene/backend.rscrates/bevy_motiongfx/src/scene/id.rscrates/bevy_motiongfx/src/scene/value_pool.rscrates/bevy_motiongfx/src/world.rscrates/motiongfx/Cargo.tomlcrates/motiongfx/src/ease.rscrates/motiongfx/src/lib.rscrates/motiongfx/src/ops.rscrates/motiongfx/src/pipeline.rscrates/motiongfx/src/timeline.rscrates/motiongfx_scene/Cargo.tomlcrates/motiongfx_scene/examples/dummy_scene.rscrates/motiongfx_scene/src/backend.rscrates/motiongfx_scene/src/block.rscrates/motiongfx_scene/src/compile.rscrates/motiongfx_scene/src/duration.rscrates/motiongfx_scene/src/error.rscrates/motiongfx_scene/src/lib.rscrates/motiongfx_scene/src/refs.rscrates/motiongfx_scene/src/registry.rscrates/motiongfx_scene/src/scene.rscrates/motiongfx_scene/src/value.rscrates/motiongfx_scene/tests/compile.rscrates/motiongfx_scene/tests/roundtrip.rseditor/motiongfx_editor/Cargo.tomleditor/motiongfx_editor_ui/Cargo.tomlexamples/bevy_examples/Cargo.tomlexamples/bevy_examples/assets/scenes/cube.mgx.ronexamples/bevy_examples/assets/scenes/cube.scn.ronexamples/bevy_examples/examples/scene_demo.rsexamples/vello_winit_example/Cargo.toml
Comment on lines
+25
to
+30
| let repr = if duration.subsec_nanos().is_multiple_of(1_000_000) { | ||
| Repr::Ms(duration.as_millis() as u64) | ||
| } else { | ||
| Repr::Ns( | ||
| u64::try_from(duration.as_nanos()).unwrap_or(u64::MAX), | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching duration.rs:"
fd -a 'duration\.rs$' . || true
echo
echo "Target file outline:"
ast-grep outline crates/motiongfx_scene/src/duration.rs --view expanded || true
echo
echo "Target file contents:"
cat -n crates/motiongfx_scene/src/duration.rs
echo
echo "Search for Repr serialization/deserialization/context:"
rg -n "enum Repr|Repr|as_millis|as_nanos|subsec_nanos|serde" crates/motiongfx_scene/src/duration.rs .Repository: voxell-tech/motiongfx
Length of output: 12678
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Duration and chrono usage around blocks:"
cat -n crates/motiongfx_scene/src/block.rs | sed -n '1,120p'
echo
echo "Roundtrip test duration values:"
rg -n "duration|Duration|Duration::|chrono::" crates/motiongfx_scene/tests/roundtrip.rs -C 3
echo
echo "Behavioral probe for oversized Duration serialization boundaries around u64:"
python3 - <<'PY'
from math import isclose
def rust_u64_max():
return 2**64 - 1
def current_serialize_sub_second_whole_ms_boundary():
# Rust std Duration::MAX is Duration(secs=2**55, nanos=1) roughly.
# as_nanos saturates to u128::MAX when out of u128, but we only need
# boundary where as_nanos exceeds u64::MAX and as_millis truncated.
# Use exact u128 Duration boundaries conceptually and Python ints.
pass
def as_nanos_secs(secs, nans):
# Mirrors Duration::as_nanos for non-negative components as u128
return (secs * 1_000_000_000) + nans # can exceed u64
def current_encode_ms_or_ns(secs, nans):
total_nanos = as_nanos_secs(secs, nans)
subsec_nanos = nans
if subsec_nanos % 1_000_000 == 0:
# Rust as_millis is u128, then cast to u64
total_millis = total_nanos // 1_000_000
return ("Ms", 0xFFFFFFFF_FFFFFFFF & total_millis)
else:
try_ns = total_nanos
return ("Ns", 0xFFFFFFFF_FFFFFFFF if try_ns > 0xFFFFFFFF_FFFFFFFF else try_ns)
def decode(encoded):
typ, value = encoded
if typ == "Ms":
return value * 1_000_000
return value
cases = [
("u64 nanos boundary, whole ms", 2**62, 0),
("u64 millis boundary", 2**62 // 1_000_000, 0),
("u64 millis+1 whole ms", 2**62 // 1_000_000 + 1, 0),
("u64 nanos overflow by one whole ms", 2**50 + 1_000_000, 0),
("near u64 nanos overflow with sub-millisecond precision", 2**62, 999_999),
]
for name, secs, nans in cases:
encoded = current_encode_ms_or_ns(secs, nans)
dec = decode(encoded)
orig = as_nanos_secs(secs, nans)
same = dec == orig
print(f"{name}: orig_ns={orig} encoded={encoded} decoded_ns={dec} same={same}")
u64 = 2**64 - 1
secs_at_u64_ms = u64 // 1_000_000 + 1
nans_at_u64_ns = u64 + 1
for name, secs, nans in [
("just above u64_ms", secs_at_u64_ms, 0),
("just above u64_ns", 0, nans_at_u64_ns),
]:
orig = as_nanos_secs(secs, nans)
enc = current_encode_ms_or_ns(secs, nans)
print(f"{name}: secs={secs} nans={nans} orig_ns={orig} encoded={enc} decoded_ns={decode(enc)}")
PY
echo
echo "Rust Duration::MAX constants from docs if rustfmt unavailable?"
rg -n "Duration::MAX|u128::MAX|as_nanos|as_millis|subsec_nanos" crates/motiongfx_scene/src/duration.rsRepository: voxell-tech/motiongfx
Length of output: 5690
Preserve or reject oversized durations instead of silently changing them.
as_millis() as u64 truncates durations above u64::MAX milliseconds, and as_nanos() with unwrap_or(u64::MAX) saturates durations above u64::MAX nanoseconds to a different duration. Since this module documents exact round-tripping, add an overflow-safe representation such as { secs, nanos } while keeping existing Ms/Ns decoding compatible, and cover the boundary cases.
🤖 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 `@crates/motiongfx_scene/src/duration.rs` around lines 25 - 30, Update the
duration representation logic around Repr and the duration serialization path to
avoid truncating or saturating values that exceed u64 millisecond or nanosecond
limits. Add an overflow-safe seconds-and-nanoseconds representation for
oversized durations, preserve existing Ms/Ns decoding compatibility, and add
coverage for both overflow boundaries while retaining exact round-tripping.
Add `apply_stage` to seed the fields
A serialized scene could not reproduce its own opening frame, since
nothing wrote the stage into the world. `Scene::stage` does, through a
new `FieldResolver::seed` that reuses `Scene::compile`'s name-to-type
resolution but assigns through the accessor. `MotionGfxScene::stage`
wraps it for Bevy, called from `scene_demo`.
Also moves `compile`/`stage` onto `Scene` as inherent methods, replaces
the fabricated `UnknownField(FieldRef::new("", ""))` fallback with
`CompileError::EmptyTimeline`, and renames `tests/compile.rs` to
`tests/toy.rs` to stop it clashing with `src/compile.rs`.
nixonyh
marked this pull request as ready for review
July 31, 2026 07:13
There was a problem hiding this comment.
🧹 Nitpick comments (2)
examples/bevy_examples/examples/scene_demo.rs (1)
111-131: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the existing registry into the closure instead of rebuilding it.
compilefinishes withregistryat line 114, soregistryis free to move into the deferred closure. The example then builds the registry once. Readers copy example code, so the single-build form teaches the cheaper pattern.♻️ Proposed change
let handle = pending.0.clone(); commands.queue(move |world: &mut World| { - let registry = default_scene_registry(); world.resource_scope::<Assets<MotionGfxScene>, _>(🤖 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 `@examples/bevy_examples/examples/scene_demo.rs` around lines 111 - 131, Move the existing registry created for scene compilation into the deferred closure queued by commands.queue, capturing it with the closure, and remove the second default_scene_registry() call inside the closure. Keep the compile and stage operations using this same registry instance.crates/motiongfx_scene/src/compile.rs (1)
45-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument that
stageapplies seeds partially on error.
stagewrites each seed directly intoworldand returns on the first error. If seed number n fails, seeds 1..n-1 stay applied. The world then holds a half-staged opening frame, and the caller has no way to tell how far the write got. The example inexamples/bevy_examples/examples/scene_demo.rspanics on this error, so the app dies with an inconsistent world.State this contract in the doc comment, or validate every seed before the first write.
♻️ Proposed doc note
/// Writes the [`Stage`](crate::scene::Stage)'s initial values into /// `world`. Run after the subjects are materialized, before /// `bake_actions`. + /// + /// Seeds are written in order. On error, the seeds written before + /// the failing one stay applied, so `world` can hold a partially + /// staged opening frame. pub fn stage(🤖 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 `@crates/motiongfx_scene/src/compile.rs` around lines 45 - 65, Update the doc comment for Compile::stage to explicitly state that seeds are applied directly in order and may be partially written when a seed fails, with earlier seeds remaining in world; preserve the existing early-return 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.
Nitpick comments:
In `@crates/motiongfx_scene/src/compile.rs`:
- Around line 45-65: Update the doc comment for Compile::stage to explicitly
state that seeds are applied directly in order and may be partially written when
a seed fails, with earlier seeds remaining in world; preserve the existing
early-return behavior.
In `@examples/bevy_examples/examples/scene_demo.rs`:
- Around line 111-131: Move the existing registry created for scene compilation
into the deferred closure queued by commands.queue, capturing it with the
closure, and remove the second default_scene_registry() call inside the closure.
Keep the compile and stage operations using this same registry instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 48514f27-b7a8-41b7-bebe-7791ff360b70
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
Cargo.tomlcrates/bevy_motiongfx/Cargo.tomlcrates/bevy_motiongfx/src/controller.rscrates/bevy_motiongfx/src/scene.rscrates/bevy_motiongfx/src/scene/asset.rscrates/bevy_motiongfx/src/scene/backend.rscrates/bevy_motiongfx/src/scene/id.rscrates/bevy_motiongfx/src/scene/value_pool.rscrates/motiongfx/examples/custom_world.rscrates/motiongfx_scene/Cargo.tomlcrates/motiongfx_scene/examples/dummy_scene.rscrates/motiongfx_scene/src/block.rscrates/motiongfx_scene/src/compile.rscrates/motiongfx_scene/src/duration.rscrates/motiongfx_scene/src/error.rscrates/motiongfx_scene/src/lib.rscrates/motiongfx_scene/src/registry.rscrates/motiongfx_scene/src/scene.rscrates/motiongfx_scene/src/value.rscrates/motiongfx_scene/tests/roundtrip.rscrates/motiongfx_scene/tests/toy.rsexamples/bevy_examples/assets/scenes/cube.mgx.ronexamples/bevy_examples/examples/scene_demo.rsexamples/vello_winit_example/examples/lissajous.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- crates/motiongfx_scene/Cargo.toml
- crates/bevy_motiongfx/src/scene.rs
- examples/bevy_examples/assets/scenes/cube.mgx.ron
- crates/motiongfx_scene/tests/roundtrip.rs
- crates/motiongfx_scene/src/value.rs
- crates/motiongfx_scene/src/lib.rs
- crates/bevy_motiongfx/src/scene/id.rs
- crates/bevy_motiongfx/Cargo.toml
Custom serialization introduces potential problems with nano seconds.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/bevy_motiongfx/tests/asset.rs (1)
19-30: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winValidate all value references covered by the test.
The test checks only
scene.stage.subjects[0]. A later subject can reference a missing pool entry and the test still passes. TheScenecontract also includes values referenced by animation data. Iterate over all subjects and traverse animation references, or narrow the comment at Line 23.Proposed minimum fix
- let subject = &scene.stage.subjects[0]; - assert_eq!(subject.fields.len(), 1); + assert_eq!(scene.stage.subjects.len(), 1); + assert_eq!(scene.stage.subjects[0].fields.len(), 1); - // Every seeded value resolves in the pool it points at. - for seed in &subject.fields { + // Every stage seed resolves in the pool it points at. + for subject in &scene.stage.subjects { + for seed in &subject.fields { assert!( scene.values.vec3.contains_key(&seed.value), "seed {:?} missing from the vec3 column", seed.value ); + } }🤖 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 `@crates/bevy_motiongfx/tests/asset.rs` around lines 19 - 30, Expand the asset validation test beyond the first subject: iterate through every subject in scene.stage.subjects and validate each seed’s value against scene.values.vec3, then also traverse the animation data and validate its referenced values using the appropriate pool. Keep the existing missing-reference assertion context for each checked reference.
🤖 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.
Nitpick comments:
In `@crates/bevy_motiongfx/tests/asset.rs`:
- Around line 19-30: Expand the asset validation test beyond the first subject:
iterate through every subject in scene.stage.subjects and validate each seed’s
value against scene.values.vec3, then also traverse the animation data and
validate its referenced values using the appropriate pool. Keep the existing
missing-reference assertion context for each checked reference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a03c4a0f-a405-4ba8-ae93-61118893a145
📒 Files selected for processing (5)
crates/bevy_motiongfx/Cargo.tomlcrates/bevy_motiongfx/tests/asset.rscrates/motiongfx_scene/src/block.rscrates/motiongfx_scene/src/lib.rsexamples/bevy_examples/assets/scenes/cube.mgx.ron
💤 Files with no reviewable changes (1)
- crates/motiongfx_scene/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- examples/bevy_examples/assets/scenes/cube.mgx.ron
- crates/motiongfx_scene/src/block.rs
- crates/bevy_motiongfx/Cargo.toml
Closed
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.
Support serialization of
motiongfxviamotiongfx_scene!motiongfx_sceneis backend agnostic, which means it's not just bevy, it can work with any backend world/renderer/app you like!This PR also made double confirmation on
no_stdsupport formotiongfx&motiongfx_sceneby compiling it using thethumbv7em-none-eabihftarget.