Add scale_around_center method to BoundingVolume trait

[chompaa]

Objective

Add a scale_around_center method to the BoundingVolume trait, as per #12130.

Solution

Added scale_around_center to the BoundingVolume trait, implemented in Aabb2d, Aabb3d, BoundingCircle, and BoundingSphere (with tests).

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[alice-i-cecile]

Nice docs and tests :) Thanks!

[Jondolf]

This looks good implementation-wise, but we might need to think more about what the scaling should actually do.

This scales bounding volumes in each direction relative to their centers, right? So the center remains the same, but just the size changes. The Parry collision detection library actually scales the min and max of AABBs directly, which also affects the center (this is consistent with how rotation for bounding volumes works, see #11681).

I believe one reason for doing scaling like that is that you can e.g. scale entire Bounding Volume Hierarchies without having to separately scale the AABB sizes and center coordinates. BVHs can even be used to accelerate colliders like polylines and trimeshes, and scaling them requires scaling the BVH, which in turn requires scaling the center coordinates of the bounding volumes.

The kind of scaling in this PR isn't generally as useful in my opinion; if you want to expand AABBs for e.g. speculative contacts or CCD, you'd typically want to expand them by a specific amount, and we have grow for that.

[Jondolf]

Jolt also scales min and max separately: https://github.com/jrouwe/JoltPhysics/blob/17d727a8d02b5e82ab8cc89b7cd0967f4a9fe61d/Jolt/Geometry/AABox.h#L214

[unknownue]

The meaning of "scale" is controversial.
I would like to use another name like scale_around_center instead of scale to allow user scale the BoundingVolume relative to center.

[Jondolf]

For scaling with respect to the center, you can also just do this: aabb.grow(aabb.half_size() * 0.1) (this expands the AABB by 10%). Not exactly the same as a separate scaling method, but can accomplish the same thing in a pretty simple way.

I haven't seen any libraries with scaling wrt the center yet, but if we do want a separate method for it, some naming ideas:

• scale_around_center (as suggested by @unknownue)
• scale_wrt_center (explicit, but wrt might be unfamiliar for some people, and abbreviations aren't ideal in names)
• scale_local (i.e. in the local space of the volume, which means the center of the volume is considered to be at the origin)
• scale_size (clearly implies changing the size only)
• resize_scale (similar to previous)

[alice-i-cecile]

scale_around_center is my preferred name here :)

[chompaa]

I agree the naming isn't precise. Renaming scale to scale_around_center seems most appropriate. Alternatively, what about keeping scale but having the point to scale around be chosen, something like:

fn scale(&self, scale: Self::HalfSize, point: Self::Position) {
    let b = Self {
        min: (self.min - point) * scale + point,
        max: (self.max - point) * scale + point,
    };
    debug_assert!(b.min.x <= b.max.x && b.min.y <= b.max.y);
    b
}

would work. In this way, we could also have the component-wise scaling mentioned by @Jondolf, by choosing to scale around zero. My only issue is that I'm not really sure this makes sense for BoundingCircle and BoundingSphere.

[Jondolf]

A few suggestions, otherwise everything looks good in terms of code.

Personally, "scaling around X" sounds kind of weird to me, since "around" often implies some kind of rotational or orbiting motion like in the case of Transform::translate_around and Transform::rotate_around. This is why I'm not 100% sold on the name scale_around_center. But I don't have a clear favorite in terms of naming, so I probably wouldn't block over it

[Jondolf]

This would be cleaner and what the BoundingCircle version already does

Suggested change

	Self {
	center: self.center,
	sphere: Sphere {
	radius: self.radius() * scale,
	},
	}
	Self::new(self.center, self.radius() * scale)

[chompaa]

Ah, I think I missed this since grow and shrink don't use Self::new.

[Jondolf]

"the center" could be confused with the global origin, i.e. (0, 0, 0). It should explicitly refer to the center of the bounding volume.

Suggested change

	/// Scale the size of the bounding volume around the center by the given amount
	/// Scale the size of the bounding volume around its center by the given amount

[Jondolf]

Maybe a matter of preference, but you can use splat when every element is the same

Suggested change

	assert!((scaled.min - Vec2::new(-2., -2.)).length() < std::f32::EPSILON);
	assert!((scaled.max - Vec2::new(2., 2.)).length() < std::f32::EPSILON);
	assert!((scaled.min - Vec2::splat(-2.)).length() < std::f32::EPSILON);
	assert!((scaled.max - Vec2::splat(2.)).length() < std::f32::EPSILON);

same for 3D

[chompaa]

I followed the style choices for the above tests here, since they choose to not use splat. There's a mix of both though overall, and I think I also prefer splat.

[Jondolf]

Similarly to the previous comment, there are constants for this:

Suggested change

	min: Vec2::new(-1., -1.),
	max: Vec2::new(1., 1.),
	min: Vec2::NEG_ONE,
	max: Vec2::ONE,

[Jondolf]

I'm not fully sold on the name scale_around_center (see my comment #12142 (review)), but I don't have a clearly better suggestion right now and the code looks good, so I'm content with this. We can always rename later if a better name pops up.

Note: You should probably update the PR title and description to match the new name :)

[NthTensor]

Code looks good but I'd like to understand the motivation before I approve. What's the use-case for this?

[unknownue]

@NthTensor In my use case, I need to scale the y-direction of a 2D bullet asset's Transform to simulate a laser effect, and the scaling value is dynamically calculated during game. scale_around_center can directly adjust the bounding box size of the laser, while using the grow/shrink method requires more steps, including:

1. Comparing the original bounding box size and the new bounding box size to determine whether to call grow or shrink in each direction (xyz).
2. Manually calculating the parameters for grow/shrink based on the scaling value.

[NthTensor]

Alright, I think this is worth adding.scale_around_center name seems pretty clear. Nice tests!

[alice-i-cecile]

@chompaa once you've resolved merge conflicts I'll merge this in :)

[chompaa]

@alice-i-cecile All should be good now :)

[alice-i-cecile]

Awesome thanks. I tried doing this on the web client and git was very confused about the #[test] annotations.