diff --git a/crates/benilla-app/src/crash.rs b/crates/benilla-app/src/crash.rs index eb17c7ff..723d6579 100644 --- a/crates/benilla-app/src/crash.rs +++ b/crates/benilla-app/src/crash.rs @@ -14,6 +14,8 @@ use std::path::{Path, PathBuf}; use std::sync::atomic::{AtomicBool, Ordering}; use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; +use bevy::ecs::error::{BevyError, ErrorContext}; + use crate::BuildId; /// Chains the crash-report hook after the current one. Called once from [`crate::run`], before @@ -27,6 +29,20 @@ pub(crate) fn install(build: BuildId) { })); } +/// The client's default error handler, installed by [`crate::run`] on its `App`: a command that +/// fails (an entity a same-frame despawn already removed) is logged at `warn` with the command and +/// the entity, and the session goes on. Every other error stays a panic, as Bevy's default, and +/// reaches the crash report: a missing resource or a failed system is a bug to see, not to skip. +/// Only the real launch installs it. Tests build their own `App`, which keeps Bevy's panicking +/// default, so a command that fails in a test still fails the test. +pub(crate) fn command_error_handler(error: BevyError, ctx: ErrorContext) { + if matches!(ctx, ErrorContext::Command { .. }) { + bevy::log::warn!("{ctx}: {error}"); + } else { + bevy::ecs::error::panic(error, ctx); + } +} + // The injected test panic (`WOW_CRASH_INJECT=`) lives in `perf::crash_inject`, behind // the `dev` feature. @@ -180,4 +196,70 @@ mod tests { assert_eq!(std::fs::read_to_string(&path).unwrap(), text); let _ = std::fs::remove_dir_all(&dir); } + + /// A failed command is logged with the command and the entity and the frame goes on; any + /// other error is still a panic. + #[test] + fn a_failed_command_logs_and_other_errors_still_panic() { + use std::sync::{Arc, Mutex}; + + use bevy::prelude::{App, Name}; + + use bevy::log::tracing::field::{Field, Visit}; + use bevy::log::tracing::{Event, Subscriber}; + use bevy::log::tracing_subscriber::layer::{Context, SubscriberExt}; + use bevy::log::tracing_subscriber::Layer; + + struct Capture(Arc>>); + struct Message(String); + impl Visit for Message { + fn record_debug(&mut self, field: &Field, value: &dyn std::fmt::Debug) { + if field.name() == "message" { + self.0 = format!("{value:?}"); + } + } + } + impl Layer for Capture { + fn on_event(&self, event: &Event<'_>, _: Context<'_, S>) { + let mut m = Message(String::new()); + event.record(&mut m); + self.0 + .lock() + .unwrap() + .push(format!("{} {}", event.metadata().level(), m.0)); + } + } + + let mut app = App::new(); + app.set_error_handler(command_error_handler); + let gone = app.world_mut().spawn_empty().id(); + app.world_mut().despawn(gone); + + let lines = Arc::new(Mutex::new(Vec::new())); + let subscriber = bevy::log::tracing_subscriber::registry().with(Capture(lines.clone())); + bevy::log::tracing::subscriber::with_default(subscriber, || { + app.world_mut() + .commands() + .entity(gone) + .insert(Name::new("x")); + app.world_mut().flush(); + }); + let lines = lines.lock().unwrap(); + assert_eq!(lines.len(), 1, "{lines:?}"); + assert!(lines[0].starts_with("WARN "), "{lines:?}"); + assert!(lines[0].contains("Command"), "names the command: {lines:?}"); + assert!( + lines[0].contains(&gone.to_string()) + || lines[0].contains(&format!("{}v", gone.index())), + "names the entity: {lines:?}" + ); + + let system = ErrorContext::System { + name: bevy::utils::prelude::DebugName::borrowed("s"), + last_run: bevy::ecs::change_detection::Tick::new(0), + }; + let panicked = + std::panic::catch_unwind(|| command_error_handler(BevyError::from("boom"), system)); + assert!(panicked.is_err(), "a system error stays a panic"); + } } diff --git a/crates/benilla-app/src/entities/attach/mod.rs b/crates/benilla-app/src/entities/attach/mod.rs index 16f4ddbc..e041e74b 100644 --- a/crates/benilla-app/src/entities/attach/mod.rs +++ b/crates/benilla-app/src/entities/attach/mod.rs @@ -295,7 +295,14 @@ pub(super) fn attach_entity_visuals( mut anim_table, ) = skin_build; let now = time.elapsed_secs(); + // Mount children this pass has despawned for a dismounted rider. The query was read before + // the despawn applies, so a child visited after its rider is still in it: skipping it here + // keeps every write off an entity the pass itself removed, whatever the iteration order. + let mut dropped: Vec = Vec::new(); for (entity, net, equipment, reattached, mount_child, mount_body, anchored) in &pending { + if dropped.contains(&entity) { + continue; + } // A player attaches as soon as its display model is ready. 1.12.1 withholds a unit only // until its first composite (`0x477860`) is done, never until every `PLAYER_VISIBLE_ITEM` // template is answered: the compositor paints the armour it knows and re-dresses when a @@ -360,6 +367,7 @@ pub(super) fn attach_entity_visuals( if mount_display != 0 { match super::mount::seat_or_spawn_mount( &mut commands, + &mut dropped, &mount_children, &mut built_poses, creatures.as_deref(), @@ -376,9 +384,8 @@ pub(super) fn attach_entity_visuals( } } else if let Some(&super::mount::MountChild(child)) = mount_child { // Dismounted while pending: drop the mount it ordered. - if let Ok(mut ec) = commands.get_entity(child) { - ec.despawn(); - } + commands.entity(child).try_despawn(); + dropped.push(child); commands .entity(entity) .try_remove::(); @@ -1041,4 +1048,104 @@ mod tests { ); world.insert_resource(palettes); } + + /// A rider that drops its still-pending mount child, dismounted or mounted on another display, + /// has the pass skip that child wherever the iteration reaches it: its writes would otherwise + /// apply to an entity the same pass despawned, which the default command error handler + /// panics on. + #[test] + fn a_dropped_pending_mount_child_is_skipped_whatever_the_visit_order() { + use super::super::mount::{MountBody, MountChild}; + + for (child_table_first, stale_display) in + [(true, false), (false, false), (true, true), (false, true)] + { + let mut app = App::new(); + app.add_plugins((MinimalPlugins, AssetPlugin::default())) + .init_asset::() + .init_asset::() + .init_asset::() + .init_resource::() + .init_resource::() + .init_resource::() + .init_resource::() + .init_resource::() + .init_resource::() + .init_resource::() + .init_resource::() + .init_resource::() + .insert_resource(CubeAssets { + mesh: Handle::default(), + player_mesh: Handle::default(), + player_mat: Handle::default(), + npc_mat: Handle::default(), + }) + // A loaded model that names a file and draws nothing: the build runs, so the + // rider's dismount branch and the child's writes are reached. + .insert_resource(Creatures { + catalog: Default::default(), + models: std::collections::HashMap::from([( + 1u32, + DisplayModel { + handle: ModelHandle::M2(Handle::default()), + parts: Some(Vec::new()), + ..super::super::display::empty_shell() + }, + )]), + }); + let net = || NetEntity { + kind: EntityKind::Unit, + display_id: Some(1), + scale: 1.0, + }; + let world = app.world_mut(); + // The mount display the rider's field names: none (dismounted), or one the child was + // not built for. + let store = ObjectStore(benilla_protocol::ObjectFields::from_pairs(&[( + benilla_protocol::field::FIELD_UNIT_MOUNTDISPLAYID, + if stale_display { 2 } else { 0 }, + )])); + // The two sit in different tables, and which one the query walks first follows the + // order they were created in, so each layout is run: the rider is visited before its + // child in one of them (measured, not assumed). + let (rider, child) = if child_table_first { + let child = world + .spawn(( + net(), + MountBody { + host: Entity::PLACEHOLDER, + }, + )) + .id(); + let rider = world.spawn((net(), MountChild(child), store)).id(); + world.entity_mut(child).insert(MountBody { host: rider }); + (rider, child) + } else { + let child = world.spawn_empty().id(); + let rider = world.spawn((net(), MountChild(child), store)).id(); + world.entity_mut(child).insert(( + net(), + MountBody { host: rider }, + Transform::default(), + )); + (rider, child) + }; + + let mut build = IntoSystem::into_system(attach_entity_visuals); + build.initialize(world); + build + .run_without_applying_deferred((), world) + .expect("the build system runs"); + build.apply_deferred(world); + + assert!( + world.get_entity(child).is_err(), + "the pending mount is gone (child_table_first={child_table_first}, stale={stale_display})" + ); + let rider = world.entity(rider); + assert!(!rider.contains::()); + // A stale mount waits for the next pass to spawn the right one; a dismounted unit builds. + assert_eq!(rider.contains::(), !stale_display); + } + } } diff --git a/crates/benilla-app/src/entities/mount.rs b/crates/benilla-app/src/entities/mount.rs index b016f0a9..b913b9d4 100644 --- a/crates/benilla-app/src/entities/mount.rs +++ b/crates/benilla-app/src/entities/mount.rs @@ -110,9 +110,12 @@ pub(super) type MountChildren<'w, 's> = Query< >; /// The seat leg of the first build and the transition: spawn the mount child when missing, drop -/// one built for a display the field has left, and resolve the seat once its rig is up. +/// one built for a display the field has left, and resolve the seat once its rig is up. A child it +/// despawns is pushed on `dropped`, which a caller still iterating children must skip: its query +/// predates the despawn. pub(super) fn seat_or_spawn_mount( commands: &mut Commands, + dropped: &mut Vec, children: &MountChildren, poses: &mut Query<&mut RigPose>, creatures: Option<&super::Creatures>, @@ -132,9 +135,8 @@ pub(super) fn seat_or_spawn_mount( // Built for a display the field has left, or gone: drop it and start over next pass. let built = children.get(child).map(|(_, _, n)| n.display_id).ok(); if built != Some(Some(mount_display)) { - if let Ok(mut ec) = commands.get_entity(child) { - ec.despawn(); - } + commands.entity(child).try_despawn(); + dropped.push(child); commands.entity(unit).remove::(); return Seat::Wait; } @@ -207,9 +209,7 @@ fn reseat_rig(commands: &mut Commands, rig: &mut RigPose, frame: Entity, conform rig.pose_dirty = true; // A conform node the rig just left is empty, so it goes; a seat anchor dies with its mount. if conform { - if let Ok(mut ec) = commands.get_entity(old) { - ec.despawn(); - } + commands.entity(old).try_despawn(); } } @@ -251,9 +251,7 @@ pub(super) fn reseat_mounts( // sweep any child, which the settled test would otherwise re-fire on every frame. let Ok(seated_root) = poses.get(entity).map(|r| r.joints_root) else { if let Some(&MountChild(child)) = child { - if let Ok(mut ec) = commands.get_entity(child) { - ec.despawn(); - } + commands.entity(child).try_despawn(); commands.entity(entity).remove::(); } commands.entity(entity).insert(AppliedMount(live)); @@ -273,9 +271,7 @@ pub(super) fn reseat_mounts( } } if let Some(child) = child { - if let Ok(mut ec) = commands.get_entity(child) { - ec.despawn(); - } + commands.entity(child).try_despawn(); } commands .entity(entity) @@ -286,6 +282,8 @@ pub(super) fn reseat_mounts( // ── Leg 2 · `0x607a00`: build the mount model and attach the body to its seat. match seat_or_spawn_mount( &mut commands, + // This system walks riders, never children, so nothing here revisits a despawned one. + &mut Vec::new(), &children, &mut poses, creatures.as_deref(), diff --git a/crates/benilla-app/src/lib.rs b/crates/benilla-app/src/lib.rs index b46e56df..39cc4cb2 100644 --- a/crates/benilla-app/src/lib.rs +++ b/crates/benilla-app/src/lib.rs @@ -258,6 +258,8 @@ pub fn run(build: BuildId) -> AppExit { }; let mut app = App::new(); + // A command on an entity a same-frame despawn removed logs instead of aborting the session. + app.set_error_handler(crash::command_error_handler); // The panel footer and the preflight banner read the stamp back. app.insert_resource(build); // Static-scene transform tracking pinned on: the default threshold re-decides every frame with