From d8d8f4fc590019cd2d3a169349843408cfeba590 Mon Sep 17 00:00:00 2001 From: grom Date: Mon, 19 Oct 2026 17:22:59 -0600 Subject: [PATCH] physics: retire avian's collider mass-property pass, 181 ms off a world load MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `WorldPlugins` has disabled `MassPropertyPlugin` for as long as the world has had colliders, with the reason written beside it: there are no dynamic bodies, so no mass is ever read. The pass kept running anyway. `ColliderBackendPlugin`, which we keep for `update_collider_scale`, registers `update_collider_mass_properties` itself (`collision/collider/backend.rs:232`), and that system's query filters `Without` and nothing about the body type — so every collider that *changed* paid `Collider::mass_properties`, a full parry volume integral over the trimesh, for a value with no reader. Terrain streams its colliders as trimeshes by the tile, so the cost lands in a world load. Measured on a cold Elwynn vista with `WOW_SYSTEM_TIMING=1`, before and after, same viewpoint: before run_fixed_main_schedule worst 183.0 ms 200.9 ms/s after run_fixed_main_schedule worst 1.3 ms 25.9 ms/s `update_collider_mass_properties` no longer appears in the report at all, and its 181.3 ms worst run was the whole of that schedule's spike — the nesting is what identified it. A run condition rather than another `.disable()`, because the system cannot be removed individually and shares a `.chain()` with `update_collider_scale`. `MassPropertySystems` is `pub` but not in avian's prelude, hence the full path. The invariant the whole physics trim rests on now has a wall of its own: `tests/no_dynamic_bodies_wall.rs` fails if `RigidBody::Dynamic` appears in the workspace. This bug was exactly an invariant that lived only in prose — the `.disable()` comment stated it correctly while a system slipped through the gap between two plugins — so the assert is the point. Verified by planting a dynamic body and watching it fail, then reverting. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn --- crates/benilla-world/src/world_plugins.rs | 20 +++ .../tests/no_dynamic_bodies_wall.rs | 124 ++++++++++++++++++ 2 files changed, 144 insertions(+) create mode 100644 crates/benilla-world/tests/no_dynamic_bodies_wall.rs diff --git a/crates/benilla-world/src/world_plugins.rs b/crates/benilla-world/src/world_plugins.rs index 2886de92..de0feea4 100644 --- a/crates/benilla-world/src/world_plugins.rs +++ b/crates/benilla-world/src/world_plugins.rs @@ -154,6 +154,26 @@ impl Plugin for WorldFoundation { .init_resource::() .init_resource::() .add_systems(First, crate::collision::track_collider_removals); + // Disabling `MassPropertyPlugin` above does **not** retire every mass-property system: + // `ColliderBackendPlugin`, which we keep for `update_collider_scale`, registers + // `update_collider_mass_properties` itself (`collision/collider/backend.rs:232`) into this + // set. Its query filters `Without` and nothing else — not the body type — so every + // collider that *changes* pays `Collider::mass_properties`, a full parry volume integral + // over the trimesh, for a mass no dynamic body will ever read. Measured on a cold Elwynn + // vista with `WOW_SYSTEM_TIMING=1`: **one run of 181 ms**, which is also the whole of that + // second's `run_fixed_main_schedule` worst (183 ms). Terrain streams trimesh colliders by + // the tile, so the spike lands squarely in the world-load hitches. + // + // Retiring the set is safe for the same reason `MassPropertyPlugin` is off: the workspace + // has 23 `RigidBody::Static`, 4 `Kinematic` and **0 `Dynamic`**, and reads no mass + // component anywhere (`ColliderMassProperties`, `ComputedMass`, `ColliderDensity`, + // `CenterOfMass`, `AngularInertia`: no consumers). Re-enable with the plugin when a dynamic + // body enters the world — the comment on `.disable::()` says the same. + app.configure_sets( + FixedPostUpdate, + avian3d::dynamics::rigid_body::mass_properties::MassPropertySystems::UpdateColliderMassProperties + .run_if(|| false), + ); // No pre-step transform propagation: movers write `Position`/`Rotation` directly, so a // second whole-world sweep buys nothing; an opening door's hull reaches physics a frame // late. `WOW_PHYS_PREPROP=1` restores avian's default. diff --git a/crates/benilla-world/tests/no_dynamic_bodies_wall.rs b/crates/benilla-world/tests/no_dynamic_bodies_wall.rs new file mode 100644 index 00000000..f59eb7c3 --- /dev/null +++ b/crates/benilla-world/tests/no_dynamic_bodies_wall.rs @@ -0,0 +1,124 @@ +//! The dynamic-body wall: the world has **no `RigidBody::Dynamic`**, and most of the physics +//! trimming in `world_plugins.rs` is correct only while that holds. +//! +//! `WorldPlugins` disables `MassPropertyPlugin`, `ForcePlugin`, `IntegratorPlugin`, `SolverPlugin`, +//! `XpbdSolverPlugin`, `SolverBodyPlugin`, `CcdPlugin`, `IslandPlugin`, `JointPlugin` and +//! `PhysicsInterpolationPlugin`, runs `SubstepCount(1)` instead of avian's 6, and retires +//! `MassPropertySystems::UpdateColliderMassProperties`. Every one of those is justified by the same +//! sentence — *"the world has no dynamic bodies"* — and each would fail **silently**, as wrong +//! motion rather than a compile error, the day one appears. +//! +//! **Why this is a test and not a comment.** It already went wrong once in the other direction: +//! `.disable::()` was in place with exactly the right reasoning written beside +//! it, while `update_collider_mass_properties` kept running, because `ColliderBackendPlugin` — kept +//! for `update_collider_scale` — registers that system itself. Nothing caught it until a +//! `WOW_SYSTEM_TIMING=1` run showed one run of **181 ms** integrating trimesh volumes for a mass no +//! body reads. An invariant asserted only in prose is an invariant nothing checks. +//! +//! If you are here because this test failed: you added a dynamic body, which is a legitimate thing +//! to do. Re-enable the plugins that body needs, drop the run condition on +//! `MassPropertySystems::UpdateColliderMassProperties`, reconsider `SubstepCount`, and add the +//! kind to [`ALLOWED`] with the reason. +//! +//! Scope: this guards the *body kind* only. That no code reads a mass property today was checked by +//! hand when the run condition landed (`ColliderMassProperties`, `ComputedMass`, `CenterOfMass`, +//! `AngularInertia`: no readers anywhere); it is not asserted here, because a grep for those names +//! fires on prose and on unrelated geometry code, and a brittle wall is worse than none. + +use std::path::{Path, PathBuf}; + +/// This test's own file name, the one file excluded from the scan. +const SELF: &str = "no_dynamic_bodies_wall.rs"; + +/// Body kinds the world may name, and why. `Dynamic` is deliberately absent. +const ALLOWED: &[(&str, &str)] = &[ + ( + "Static", + "terrain, WMOs, doodads and one-sided collision: never integrated", + ), + ( + "Kinematic", + "transports and attached hulls: they write Position/Rotation directly", + ), +]; + +#[test] +fn the_world_names_no_dynamic_rigid_body() { + let root = workspace_root(); + let mut offenders: Vec = Vec::new(); + for path in workspace_sources(&root) { + // This file names the forbidden kind itself — in the header, in the scan below and in the + // failure message — so it is the one file the wall cannot read. + if path.file_name().is_some_and(|f| f == SELF) { + continue; + } + let Ok(text) = std::fs::read_to_string(&path) else { + continue; + }; + for (n, line) in text.lines().enumerate() { + // Prose may discuss the kind — the `.disable()` comments in `world_plugins.rs` exist to + // explain exactly why there is none — and commented-out code is not active. Only code + // counts. + if line.trim_start().starts_with("//") { + continue; + } + if line.contains("RigidBody::Dynamic") { + let rel = path.strip_prefix(&root).unwrap_or(&path); + offenders.push(format!("{}:{}", rel.display(), n + 1)); + } + } + } + assert!( + offenders.is_empty(), + "RigidBody::Dynamic appears in the workspace, but `WorldPlugins` is trimmed for a world \ + without one (no mass properties, no solver, no integrator, no CCD, one substep). Read \ + this file's header before deleting the assert.\nFound at:\n {}\nAllowed kinds: {}", + offenders.join("\n "), + ALLOWED + .iter() + .map(|(k, why)| format!("{k} ({why})")) + .collect::>() + .join(", "), + ); +} + +fn workspace_root() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .parent() + .and_then(Path::parent) + .expect("crates/benilla-world → the workspace root") + .to_path_buf() +} + +/// Every `.rs` under each crate's `src/`, `examples/` and `tests/`. +fn workspace_sources(root: &Path) -> Vec { + let mut out = Vec::new(); + for crate_dir in std::fs::read_dir(root.join("crates")) + .into_iter() + .flatten() + .flatten() + { + for sub in ["src", "examples", "tests"] { + let dir = crate_dir.path().join(sub); + if dir.is_dir() { + collect_rs(&dir, &mut out); + } + } + } + out.sort(); + out +} + +fn collect_rs(dir: &Path, out: &mut Vec) { + let mut stack = vec![dir.to_path_buf()]; + while let Some(d) = stack.pop() { + for e in std::fs::read_dir(&d).into_iter().flatten().flatten() { + let p = e.path(); + if p.is_dir() { + stack.push(p); + } else if p.extension().is_some_and(|x| x == "rs") { + out.push(p); + } + } + } +}