Skip to content

Commit da07cc2

Browse files
authored
Remove unsafe ColliderAabb initialization (#669)
# Objective #527 used a read-only `UnsafeWorldCell` for the initialization of `ColliderAabb` in an `on_add` hook for `Collider`. However, it uses methods that provide mutable access, which is unsafe and panics with debug assertions. ## Solution Remove the whole AABB initialization logic and use of `unsafe`. Having it didn't make sense since the initial positions are likely wrong anyway. `ColliderAabb` is now explicitly specified to be `ColliderAabb::INVALID` until the first physics update.
1 parent 9f8ce98 commit da07cc2

File tree

2 files changed

+18
-54
lines changed

2 files changed

+18
-54
lines changed

src/collision/collider/backend.rs

Lines changed: 4 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
//!
33
//! See [`ColliderBackendPlugin`].
44
5-
use core::{any::type_name, marker::PhantomData};
5+
use core::marker::PhantomData;
66

77
use crate::{broad_phase::BroadPhaseSet, prelude::*, prepare::PrepareSet, sync::SyncConfig};
88
#[cfg(all(feature = "bevy_scene", feature = "default-collider"))]
@@ -11,8 +11,7 @@ use bevy::{
1111
ecs::{
1212
intern::Interned,
1313
schedule::ScheduleLabel,
14-
system::{StaticSystemParam, SystemId, SystemState},
15-
world::WorldEntityFetch,
14+
system::{StaticSystemParam, SystemId},
1615
},
1716
prelude::*,
1817
};
@@ -88,9 +87,6 @@ impl<C: ScalableCollider> Default for ColliderBackendPlugin<C> {
8887
}
8988
}
9089

91-
#[derive(Resource)]
92-
struct ContextState<C: ScalableCollider>(SystemState<C::Context>);
93-
9490
impl<C: ScalableCollider> Plugin for ColliderBackendPlugin<C> {
9591
fn build(&self, app: &mut App) {
9692
// Register required components for the collider type.
@@ -105,13 +101,10 @@ impl<C: ScalableCollider> Plugin for ColliderBackendPlugin<C> {
105101
app.insert_resource(ColliderRemovalSystem(collider_removed_id));
106102
}
107103

108-
let context_state = SystemState::new(app.world_mut());
109-
app.insert_resource(ContextState::<C>(context_state));
110-
111104
let hooks = app.world_mut().register_component_hooks::<C>();
112105

113106
// Initialize missing components for colliders.
114-
hooks.on_add(|world, entity, _| {
107+
hooks.on_add(|mut world, entity, _| {
115108
let existing_global_transform = world
116109
.entity(entity)
117110
.get::<GlobalTransform>()
@@ -147,8 +140,7 @@ impl<C: ScalableCollider> Plugin for ColliderBackendPlugin<C> {
147140
#[cfg(feature = "2d")]
148141
let scale = scale.xy();
149142

150-
let cell = world.as_unsafe_world_cell_readonly();
151-
let mut entity_mut = unsafe { entity.fetch_deferred_mut(cell).unwrap() };
143+
let mut entity_mut = world.entity_mut(entity);
152144

153145
// Make sure the collider is initialized with the correct scale.
154146
// This overwrites the scale set by the constructor, but that one is
@@ -160,33 +152,6 @@ impl<C: ScalableCollider> Plugin for ColliderBackendPlugin<C> {
160152

161153
let collider = entity_mut.get::<C>().unwrap();
162154

163-
let aabb = {
164-
let mut context_state = {
165-
// SAFETY: No other code takes a ref to this resource,
166-
// and `ContextState` is not publicly visible,
167-
// so `C::Context` is unable to borrow this resource.
168-
// This does not perform any structural world changes,
169-
// so reading mutably through a read-only cell is OK.
170-
// (We can't get a non-readonly cell from a DeferredWorld)
171-
unsafe { cell.get_resource_mut::<ContextState<C>>() }
172-
}
173-
.unwrap_or_else(|| {
174-
panic!(
175-
"context state for `{}` was removed",
176-
type_name::<C::Context>()
177-
)
178-
});
179-
let collider_context = context_state.0.get(&world);
180-
let context = AabbContext::new(entity, &collider_context);
181-
entity_mut
182-
.get::<ColliderAabb>()
183-
.copied()
184-
.unwrap_or(collider.aabb_with_context(
185-
Vector::ZERO,
186-
Rotation::default(),
187-
context,
188-
))
189-
};
190155
let density = entity_mut
191156
.get::<ColliderDensity>()
192157
.copied()
@@ -198,10 +163,6 @@ impl<C: ScalableCollider> Plugin for ColliderBackendPlugin<C> {
198163
collider.mass_properties(density.0)
199164
};
200165

201-
if let Some(mut collider_aabb) = entity_mut.get_mut::<ColliderAabb>() {
202-
*collider_aabb = aabb;
203-
}
204-
205166
if let Some(mut collider_mass_properties) =
206167
entity_mut.get_mut::<ColliderMassProperties>()
207168
{

src/collision/collider/mod.rs

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -529,9 +529,9 @@ impl From<Transform> for ColliderTransform {
529529
#[reflect(Debug, Component, Default, PartialEq)]
530530
pub struct Sensor;
531531

532-
/// The Axis-Aligned Bounding Box of a [collider](Collider).
532+
/// The Axis-Aligned Bounding Box of a [collider](Collider) in world space.
533533
///
534-
/// The coordinates are in world space (global coordinates).
534+
/// Note that the AABB will be [`ColliderAabb::INVALID`] until the first physics update.
535535
#[derive(Reflect, Clone, Copy, Component, Debug, PartialEq)]
536536
#[cfg_attr(feature = "serialize", derive(serde::Serialize, serde::Deserialize))]
537537
#[cfg_attr(feature = "serialize", reflect(Serialize, Deserialize))]
@@ -543,7 +543,19 @@ pub struct ColliderAabb {
543543
pub max: Vector,
544544
}
545545

546+
impl Default for ColliderAabb {
547+
fn default() -> Self {
548+
ColliderAabb::INVALID
549+
}
550+
}
551+
546552
impl ColliderAabb {
553+
/// An invalid [`ColliderAabb`] that represents an empty AABB.
554+
pub const INVALID: Self = Self {
555+
min: Vector::INFINITY,
556+
max: Vector::NEG_INFINITY,
557+
};
558+
547559
/// Creates a new [`ColliderAabb`] from the given `center` and `half_size`.
548560
pub fn new(center: Vector, half_size: Vector) -> Self {
549561
Self {
@@ -633,15 +645,6 @@ impl ColliderAabb {
633645
}
634646
}
635647

636-
impl Default for ColliderAabb {
637-
fn default() -> Self {
638-
ColliderAabb {
639-
min: Vector::INFINITY,
640-
max: Vector::NEG_INFINITY,
641-
}
642-
}
643-
}
644-
645648
/// A component that adds an extra margin or "skin" around [`Collider`] shapes to help maintain
646649
/// additional separation to other objects. This added thickness can help improve
647650
/// stability and performance in some cases, especially for thin shapes such as trimeshes.

0 commit comments

Comments
 (0)