Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CollisionLayers modifiers look like in-place but are not. #322

Closed
spectria-limina opened this issue Feb 12, 2024 · 0 comments · Fixed by #313
Closed

CollisionLayers modifiers look like in-place but are not. #322

spectria-limina opened this issue Feb 12, 2024 · 0 comments · Fixed by #313

Comments

@spectria-limina
Copy link

The functions on CollisionLayers to modify the layer sets look like they are in-place, but are not:

// WRONG
layers.add_mask(Layer::Walls);
// RIGHT
*layers = layers.add_mask(Layer::Walls);

One option would be to add a second set of mutators that work in-place, but a simpler fix (especially if naming is a problem) would be to add #[must_use] to the mutators so that discarded return values are flagged.

@Jondolf Jondolf mentioned this issue Feb 17, 2024
Jondolf added a commit that referenced this issue Feb 17, 2024
# Objective

Fixes #322 (among other things).

Improve and simplify the API used for `CollisionLayers` and `SpatialQueryFilter`.

## Solution

Add a `LayerMask` struct that stores a bitmask used for collision layers. The `groups` and `masks` properties have also been renamed to `memberships` and `filters`, because "groups" and "layers" sound like the same thing, and "masks" might be confusing since it's a single bitmask (just like groups/memberships). The new naming is also what Rapier uses.

A `LayerMask` can be created from a `u32`, a type implementing `PhysicsLayer`, or an array of layers. `CollisionLayers::new` now takes `impl Into<LayerMask>`. All of the following work:

```rust
let layers = CollisionLayers::new(0b00010, 0b0111);
let layers = CollisionLayers::new(GameLayer::Player, [GameLayer::Enemy, GameLayer::Ground]);
let layers = CollisionLayers::new(LayerMask(0b0001), LayerMask::ALL);
```

`LayerMask` also allows us to reduce the number of `CollisionLayers` methods. `contains_group`/`contains_mask`, `add_group`/`add_mask` and so on have been removed in favor of simply calling methods like `add` or `remove` on the actual properties.

```rust
layers.memberships.remove(GameLayer::Environment);
layers.filters.add([GameLayer::Environment, GameLayer::Tree]);

// Bitwise ops also work since we're accessing the bitmasks/layermasks directly
layers.memberships |= GameLayer::Player; // or bitmask directly, e.g. 0b0010
```

The methods mutate in place instead of returning a copy, which fixes #322.

`SpatialQueryFilter` also now uses `LayerMask` for its masks, and its constructors have been improved. `new` has been removed for redundancy, `with_mask_from_bits` has been removed, and instead it has the methods `from_mask`, `from_excluded_entities`, `with_mask`, and `with_excluded_entities`.

---

## Changelog

### Added

- `LayerMask` struct
- `SpatialQueryFilter::from_mask`
- `SpatialQueryFilter::from_excluded_entities`

### Changed

#### `CollisionLayers`

- `groups` and `masks` of `CollisionLayers` are now called `memberships` and `filters`
- `CollisionLayers` stores `LayerMask`s for memberships and filters
- Methods that took `impl IntoIterator<Item = impl PhysicsLayer>` now take `impl Into<LayerMask>`
- Change `CollisionLayers::all()` to constant `CollisionLayers::ALL`
- Change `CollisionLayers::none()` to constant `CollisionLayers::NONE`
- Change `CollisionLayers::all_groups()` to constant `CollisionLayers::ALL_MEMBERSHIPS`
- Change `CollisionLayers::all_masks()` to constant `CollisionLayers::ALL_FILTERS`

#### `SpatialQueryFilter`

- `SpatialQueryFilter` stores a `LayerMask` for collision masks
- `SpatialQueryFilter::with_mask` now takes `impl Into<LayerMask>`
- Rename `SpatialQueryFilter::without_entities` to `with_excluded_entities` for consistency

### Removed

- `CollisionLayers::add_group` and `CollisionLayers::add_mask`
- `CollisionLayers::remove_group` and `CollisionLayers::remove_mask`
- `CollisionLayers::contains_group` and `CollisionLayers::contains_mask`
- `CollisionLayers::groups_bits` and `CollisionLayers::masks_bits` (you could get e.g. `layers.groups.0` instead)
- `SpatialQueryFilter::new`
- `SpatialQueryFilter::with_masks_from_bits`

## Migration Guide

- Replace e.g. every `layers.add_group(...)` with `layers.memberships.add(...)` and so on, and similarly for masks/filters
- Change `CollisionLayers::all()` to `CollisionLayers::ALL`
- Change `CollisionLayers::none()` to `CollisionLayers::NONE`
- Change `CollisionLayers::all_groups()` to `CollisionLayers::ALL_MEMBERSHIPS`
- Change `CollisionLayers::all_masks()` to `CollisionLayers::ALL_FILTERS`
- Change `SpatialQueryFilter::new()` to `SpatialQueryFilter::default()` or use the `SpatialQueryFilter::from_mask` or `SpatialQueryFilter::from_excluded_entities` constructors
- Change `SpatialQueryFilter::without_entities` to `with_excluded_entities`
- Change `CollisionLayers::groups_bits()` and `CollisionLayers::masks_bits()` to `layers.memberships.0` and `layers.filters.0`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant