From 3ba7c2578523da97d69f8e4083185a840e83dd27 Mon Sep 17 00:00:00 2001 From: steban Date: Wed, 29 Jul 2026 04:02:55 +0200 Subject: [PATCH] Guard remaining level_colliders lookups against missing keys PR #1396 (ssarg / mynameisgrass) fixed the non-unwinding panic on the rigid_bodies registry (the reported getPose crash), but a second registry, level_colliders, was left fully unguarded. On a production NeoForge 1.21.1 server this reproduced the exact same failure mode (SIGABRT from a Rust panic across an extern "system" JNI boundary) one call further down the same code path, in setCenterOfMass: thread '' panicked at rapier/src/lib.rs:557:14: called `Option::unwrap()` on a `None` value thread caused non-unwinding panic. aborting. This commit audits every JNI entry point that indexes level_colliders (or a rigid_bodies handle later resolved through it) by an id supplied from Java, and replaces unwrap()/expect()/direct indexing with Option-based guards that no-op (or fall back to an unswapped/zero default where the lookup only affects an optimization) when the body has already been removed - mirroring the pattern #1396 applied to rigid_bodies. Sites fixed, none of which were touched by #1396: - lib.rs: setCenterOfMass, setLocalBounds, addChunk - contraptions.rs: get_kinematic_collider_info (now returns Option) and its two callers; createKinematicContraption's mount lookup; removeKinematicContraption against a double-remove - rope.rs: tick() start/end attachment anchor updates, for a rope still attached to a sub-level that has since unloaded - dispatcher.rs: the collision-pair swap heuristic, and both world_vs_world contact-manifold paths - hooks.rs: fake-velocity lookups in both solver-contact hooks Where a guarded lookup only feeds a performance heuristic (the dispatcher swap order) or a purely cosmetic velocity nudge, the fallback is the pre-existing default behavior rather than skipping the tick, since skipping there is not required for correctness. Verified against the production crash: applying #1396 alone traded the original getPose panic for this setCenterOfMass panic on the same server within the same physics tick chain (onStatsChanged calls setCenterOfMass then setLocalBounds back to back). With this commit on top, both crashes are gone under the same reproduction (repeated sub-level load/unload near a player). This patch was written and applied by Claude (Anthropic) at the repository owner's direction - i.e. it is vibecoded: the owner described the crash and asked for a fix, Claude read #1396, found the gap, wrote the guards, cross-compiled sable_rapier for x86_64-unknown-linux-gnu.2.17 to match upstream's release target, and verified the fix in place on the affected server before this commit was prepared. It has not been reviewed by a Rust engineer beyond that runtime verification - please review the unwrap/expect removals accordingly, in particular whether any of the now-silent no-op paths should instead log or clean up related state (e.g. dangling rope attachments after a level_colliders entry disappears from under it). --- .../src/main/rust/rapier/src/contraptions.rs | 34 ++++----- .../src/main/rust/rapier/src/dispatcher.rs | 69 ++++++++++++------- .../src/main/rust/rapier/src/hooks.rs | 14 ++-- sable_rapier/src/main/rust/rapier/src/lib.rs | 19 +++-- sable_rapier/src/main/rust/rapier/src/rope.rs | 61 +++++++++------- 5 files changed, 117 insertions(+), 80 deletions(-) diff --git a/sable_rapier/src/main/rust/rapier/src/contraptions.rs b/sable_rapier/src/main/rust/rapier/src/contraptions.rs index 480cab37..9da410af 100644 --- a/sable_rapier/src/main/rust/rapier/src/contraptions.rs +++ b/sable_rapier/src/main/rust/rapier/src/contraptions.rs @@ -36,11 +36,8 @@ macro_rules! extract_jint_array { fn get_kinematic_collider_info( sable: &mut SableSceneData, id: jint, -) -> &mut ActiveLevelColliderInfo { - sable - .level_colliders - .get_mut(&(id as LevelColliderID)) - .expect("No kinematic contraption with given ID!") +) -> Option<&mut ActiveLevelColliderInfo> { + sable.level_colliders.get_mut(&(id as LevelColliderID)) } #[unsafe(no_mangle)] @@ -66,18 +63,17 @@ pub extern "system" fn Java_dev_ryanhcode_sable_physics_impl_rapier_Rapier3D_cre .insert(RigidBodyBuilder::kinematic_position_based()); Some(new_body) } else { - Some( - *sable_data - .rigid_bodies - .get(&(mount_id as LevelColliderID)) - .unwrap(), - ) + sable_data + .rigid_bodies + .get(&(mount_id as LevelColliderID)) + .copied() }; let mount_rigid_body: RigidBodyHandle = if let Some(body) = mount_rigid_body { body } else { - panic!("woops!") + // The mount body was unloaded before this call reached us; nothing to attach to. + return; }; let level_collider = LevelCollider::new(Some(id as LevelColliderID), false); @@ -140,7 +136,9 @@ pub extern "system" fn Java_dev_ryanhcode_sable_physics_impl_rapier_Rapier3D_set let mut sim_data = scene.sim_data.write().unwrap(); let mut sable_data = scene.sable_data.write().unwrap(); - let info = get_kinematic_collider_info(&mut sable_data, id); + let Some(info) = get_kinematic_collider_info(&mut sable_data, id) else { + return; + }; let collider_handle = info.collider; let collider = sim_data.collider_set.get_mut(collider_handle); @@ -218,7 +216,9 @@ pub extern "system" fn Java_dev_ryanhcode_sable_physics_impl_rapier_Rapier3D_add with_handle(handle, |scene| { let mut sable_data = scene.sable_data.write().unwrap(); - let info = get_kinematic_collider_info(&mut sable_data, id); + let Some(info) = get_kinematic_collider_info(&mut sable_data, id) else { + return; + }; if let Some(chunk_map) = &mut info.chunk_map { chunk_map.insert(crate::scene::pack_section_pos(x, y, z), chunk); } @@ -240,8 +240,10 @@ pub extern "system" fn Java_dev_ryanhcode_sable_physics_impl_rapier_Rapier3D_rem let sim_data = &mut *sim_data; let mut sable_data = scene.sable_data.write().unwrap(); - let info = sable_data.level_colliders.remove(&(id as LevelColliderID)); - let info = info.unwrap(); + // Already removed (e.g. a duplicate/late removal after an unload) - nothing to do. + let Some(info) = sable_data.level_colliders.remove(&(id as LevelColliderID)) else { + return; + }; sim_data.collider_set.remove( info.collider, diff --git a/sable_rapier/src/main/rust/rapier/src/dispatcher.rs b/sable_rapier/src/main/rust/rapier/src/dispatcher.rs index 8fdbe03a..428756ff 100644 --- a/sable_rapier/src/main/rust/rapier/src/dispatcher.rs +++ b/sable_rapier/src/main/rust/rapier/src/dispatcher.rs @@ -184,25 +184,35 @@ where let sable_data = self.sable_data.read().unwrap(); let body_1 = g1 .id - .map(|id| &sable_data.level_colliders[&(id as LevelColliderID)]) - .unwrap(); + .and_then(|id| sable_data.level_colliders.get(&(id as LevelColliderID))); let body_2 = g2 .id - .map(|id| &sable_data.level_colliders[&(id as LevelColliderID)]) - .unwrap(); - - let extents_1 = body_1.local_bounds_max.unwrap() - - body_1.local_bounds_min.unwrap() - + IVec3::ONE; - let extents_2 = body_2.local_bounds_max.unwrap() - - body_2.local_bounds_min.unwrap() - + IVec3::ONE; - - let volume_1 = extents_1.x * extents_1.y * extents_1.z; - let volume_2 = extents_2.x * extents_2.y * extents_2.z; - - // Swap the bodies so we're always doing the least amount of work possible for collision detection - volume_1 < volume_2 + .and_then(|id| sable_data.level_colliders.get(&(id as LevelColliderID))); + + // A body can be unloaded mid-step, or its bounds may not be set yet. + // Swapping is only a performance choice, so fall back to the unswapped + // order rather than aborting the process. + match (body_1, body_2) { + (Some(body_1), Some(body_2)) => match ( + body_1.local_bounds_max, + body_1.local_bounds_min, + body_2.local_bounds_max, + body_2.local_bounds_min, + ) { + (Some(max_1), Some(min_1), Some(max_2), Some(min_2)) => { + let extents_1 = max_1 - min_1 + IVec3::ONE; + let extents_2 = max_2 - min_2 + IVec3::ONE; + + let volume_1 = extents_1.x * extents_1.y * extents_1.z; + let volume_2 = extents_2.x * extents_2.y * extents_2.z; + + // Swap the bodies so we're always doing the least amount of work possible for collision detection + volume_1 < volume_2 + } + _ => false, + }, + _ => false, + } }; if swap { @@ -258,8 +268,10 @@ impl SableDispatcher { let collider_info = g1 .id - .map(|id| &sable_data.level_colliders[&(id as LevelColliderID)]); - let center_of_mass_1 = collider_info.map_or(DVec3::ZERO, |b| b.center_of_mass.unwrap()); + .and_then(|id| sable_data.level_colliders.get(&(id as LevelColliderID))); + let center_of_mass_1 = collider_info + .and_then(|b| b.center_of_mass) + .unwrap_or(DVec3::ZERO); let mut local_aabb = g2.compute_aabb(pos12); @@ -443,10 +455,21 @@ impl SableDispatcher { let collider_info_1 = g1 .id - .map(|id| &sable_data.level_colliders[&(id as LevelColliderID)]); - let collider_info_2 = &sable_data.level_colliders[&(g2.id.unwrap() as LevelColliderID)]; - let center_of_mass_1 = collider_info_1.map_or(DVec3::ZERO, |b| b.center_of_mass.unwrap()); - let center_of_mass_2 = collider_info_2.center_of_mass.unwrap(); + .and_then(|id| sable_data.level_colliders.get(&(id as LevelColliderID))); + // g2 is required below; if it was unloaded mid-step, skip contact generation for + // this pair instead of aborting the process. + let Some(collider_info_2) = g2 + .id + .and_then(|id| sable_data.level_colliders.get(&(id as LevelColliderID))) + else { + return; + }; + let center_of_mass_1 = collider_info_1 + .and_then(|b| b.center_of_mass) + .unwrap_or(DVec3::ZERO); + let Some(center_of_mass_2) = collider_info_2.center_of_mass else { + return; + }; let chunk_access_1: &dyn ChunkAccess = if let Some(info) = collider_info_1 && info.has_own_chunks() diff --git a/sable_rapier/src/main/rust/rapier/src/hooks.rs b/sable_rapier/src/main/rust/rapier/src/hooks.rs index 47cdd957..05b4ee9e 100644 --- a/sable_rapier/src/main/rust/rapier/src/hooks.rs +++ b/sable_rapier/src/main/rust/rapier/src/hooks.rs @@ -128,12 +128,15 @@ impl SablePhysicsHooks { level_collider_a: Option<&LevelCollider>, ) -> Vec3 { if let Some(level_collider_a) = level_collider_a - && level_collider_a.id.is_some() + && let Some(id) = level_collider_a.id { let sable_data = self.sable_data.read().unwrap(); - let collider_info = - &sable_data.level_colliders[&(level_collider_a.id.unwrap() as LevelColliderID)]; + // The body may have been unloaded mid-step; treat it as having no fake velocity. + let Some(collider_info) = sable_data.level_colliders.get(&(id as LevelColliderID)) + else { + return Vec3::ZERO; + }; if let Some(fake_velo) = collider_info.fake_velocities { let transform = collider_a.position(); @@ -160,8 +163,9 @@ impl SablePhysicsHooks { let (tangent_velo, center_of_mass, skip_contact_events) = { let sable_data = self.sable_data.read().unwrap(); - let collider_info = - level_collider.and_then(|lc| lc.id.map(|id| &sable_data.level_colliders[&(id)])); + let collider_info = level_collider + .and_then(|lc| lc.id) + .and_then(|id| sable_data.level_colliders.get(&(id))); let mut tangent_velo = Vec3::ZERO; if let Some(fake_velo) = collider_info.and_then(|info| info.fake_velocities) { diff --git a/sable_rapier/src/main/rust/rapier/src/lib.rs b/sable_rapier/src/main/rust/rapier/src/lib.rs index bca60aef..21729adc 100644 --- a/sable_rapier/src/main/rust/rapier/src/lib.rs +++ b/sable_rapier/src/main/rust/rapier/src/lib.rs @@ -551,10 +551,9 @@ pub extern "system" fn Java_dev_ryanhcode_sable_physics_impl_rapier_Rapier3D_set ) { with_handle(handle, |scene| { let mut sable_data = scene.sable_data.write().unwrap(); - let info = sable_data - .level_colliders - .get_mut(&(id as LevelColliderID)) - .unwrap(); + let Some(info) = sable_data.level_colliders.get_mut(&(id as LevelColliderID)) else { + return; + }; info.center_of_mass = Some(DVec3::new(x, y, z)); let mut sim_data = scene.sim_data.write().unwrap(); update_collider_aabb(&mut sim_data, info); @@ -586,7 +585,9 @@ pub extern "system" fn Java_dev_ryanhcode_sable_physics_impl_rapier_Rapier3D_set .. } = &mut *sable_data; - let info = level_colliders.get_mut(&(id as LevelColliderID)).unwrap(); + let Some(info) = level_colliders.get_mut(&(id as LevelColliderID)) else { + return; + }; info.set_local_bounds( IVec3::new(min_x, min_y, min_z), IVec3::new(max_x, max_y, max_z), @@ -796,11 +797,9 @@ pub extern "system" fn Java_dev_ryanhcode_sable_physics_impl_rapier_Rapier3D_add let chunk = main_level_chunks.get(&pack_section_pos(x, y, z)).unwrap(); if global == 0 { if object_id != -1 { - let body = level_colliders - .get_mut(&(object_id as LevelColliderID)) - .unwrap(); - - body.insert_chunk(chunk, x, y, z, collider_map); + if let Some(body) = level_colliders.get_mut(&(object_id as LevelColliderID)) { + body.insert_chunk(chunk, x, y, z, collider_map); + } } } else { for bx in 0..16 { diff --git a/sable_rapier/src/main/rust/rapier/src/rope.rs b/sable_rapier/src/main/rust/rapier/src/rope.rs index 84a31010..8fd7cac1 100644 --- a/sable_rapier/src/main/rust/rapier/src/rope.rs +++ b/sable_rapier/src/main/rust/rapier/src/rope.rs @@ -54,19 +54,24 @@ pub fn tick(scene: &PhysicsScene) { if !sim.impulse_joint_set.contains(attachment.joint) { dead_start_attachments.push(id.clone()); } else { - let local_anchor = attachment.location - - if let Some(id_b) = attachment.sub_level_id { - let rb_b = &sable_data.level_colliders[&id_b]; - rb_b.center_of_mass.unwrap() - } else { - DVec3::ZERO - }; - - let impulse_joint = sim - .impulse_joint_set - .get_mut(attachment.joint, false) - .unwrap(); - impulse_joint.data.set_local_anchor1(local_anchor.as_vec3()); + // The attached sub-level may have been unloaded while the rope is still + // alive; in that case leave the anchor untouched instead of aborting. + let offset = match attachment.sub_level_id { + Some(id_b) => sable_data + .level_colliders + .get(&id_b) + .and_then(|rb_b| rb_b.center_of_mass), + None => Some(DVec3::ZERO), + }; + + if let Some(offset) = offset { + let local_anchor = attachment.location - offset; + if let Some(impulse_joint) = + sim.impulse_joint_set.get_mut(attachment.joint, false) + { + impulse_joint.data.set_local_anchor1(local_anchor.as_vec3()); + } + } } } @@ -74,19 +79,23 @@ pub fn tick(scene: &PhysicsScene) { if !sim.impulse_joint_set.contains(attachment.joint) { dead_end_attachments.push(id.clone()); } else { - let local_anchor = attachment.location - - if let Some(id_b) = attachment.sub_level_id { - let rb_b = &sable_data.level_colliders[&id_b]; - rb_b.center_of_mass.unwrap() - } else { - DVec3::ZERO - }; - - let impulse_joint = sim - .impulse_joint_set - .get_mut(attachment.joint, false) - .unwrap(); - impulse_joint.data.set_local_anchor1(local_anchor.as_vec3()); + // Same guard as the start attachment above. + let offset = match attachment.sub_level_id { + Some(id_b) => sable_data + .level_colliders + .get(&id_b) + .and_then(|rb_b| rb_b.center_of_mass), + None => Some(DVec3::ZERO), + }; + + if let Some(offset) = offset { + let local_anchor = attachment.location - offset; + if let Some(impulse_joint) = + sim.impulse_joint_set.get_mut(attachment.joint, false) + { + impulse_joint.data.set_local_anchor1(local_anchor.as_vec3()); + } + } } } }