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

Improve SpatialQuery APIs and docs, and add more configuration #510

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Jondolf
Copy link
Owner

@Jondolf Jondolf commented Sep 7, 2024

Objective

Fixes #458, among many issues that have come up in discussions.

The API for ray casts and shape casts through SpatialQuery is quite hard to read. If we supported all of Parry's configuration options for shape casts, shape_hits would end up looking like this:

// Note: target_distance and compute_impact_on_penetration don't exist prior to this PR
let hits = spatial.shape_hits(
    &shape,
    origin,
    shape_rotation,
    direction,
    max_time_of_impact,
    max_hits,
    target_distance,
    compute_impact_on_penetration,
    ignore_origin_penetration,
    &query_filter,
);

Without variables for everything, it would be almost entirely unreadable:

let hits = spatial.shape_hits(
    &Collider::sphere(0.5),
    Vec3::ZERO,
    Quat::default(),
    Dir3::ZERO,
    100.0,
    10,
    0.0,
    true,
    false,
    &default(),
);

Aside from bad ergonomics and poor readability, this is also bad for documentation, as the docs for each configuration option must be duplicated for each method that uses them. There are also no sane defaults, as every setting must be configured explicitly, even if the user is unlikely to need to care about some of them. This is especially bad for new users for whom the long list of arguments may be confusing and daunting.

Another slightly unrelated issue is that many properties and docs use the term "time of impact", which has reportedly been a bit confusing for many users. In Avian's case, for ray casts and shape casts, it is actually just the distance travelled along the ray, because the cast direction is normalized. Most occurrences of "time of impact" should likely be renamed to just "distance", except for time-of-impact queries where both shapes are moving or there is angular velocity involved.

Solution

This PR fixes several API and documentation issues, along with doing general cleanup. It's a bit large with some slightly unrelated changes, so apologies in advance for potential reviewers!

Ray casting and shape casting methods now take a RayCastConfig/ShapeCastConfig. It holds general configuration options for the cast, and could be extended in the future to even have things like debug rendering options.

The earlier example would now look like this:

let hits = spatial.shape_hits(
    &shape,
    origin,
    shape_rotation,
    direction,
    max_hits,
    &ShapeCastConfig {
        max_distance,
        target_distance,
        compute_impact_on_penetration,
        ignore_origin_penetration,
        filter,
    },
);

In practice, you can often use default values for most properties, use constructor methods, and might move the configuration outside the method call (and even reuse it across shape casts), so it could actually look like this:

let config = ShapeCastConfig::from_max_distance(100.0);
let hits = spatial.shape_hits(
    &Collider::sphere(0.5),
    Vec3::ZERO,
    Quat::default(),
    Dir3::ZERO,
    10,
    &config,
);

Ray casts have received a similar treatment. If no extra configuration is needed, a simple cast_ray call can often collapse into a very nice form:

// Before
let hit = spatial.cast_ray(Vec2::ZERO, Dir2::X, f32::MAX, true, &SpatialQueryFilter::default());

// After
let hit = spatial.cast_ray(Vec2::ZERO, Dir2::X, &RayCastConfig::default());

// Or even just this:
let hit = spatial.cast_ray(Vec2::ZERO, Dir2::X, &default());

(this could potentially be even nicer if it accepted a Ray2d/Ray3d as an argument, but that's another issue)

As you may have noticed, I also changed max_time_of_impact to max_distance. Most occurrences of "time of impact" have indeed been renamed to "distance" as per the reasons mentioned earlier.

Additionally, I fixed several documentation issues and added missing methods and configuration options. See the changelog below for a slightly more thorough overview.


Changelog

  • Added RayCastConfig. Most ray casting methods now take the config instead of many separate arguments
  • Added ShapeCastConfig. Most shape casting methods now take the config instead of many separate arguments
  • Renamed RayHitData::time_of_impact, ShapeHitData::time_of_impact, and many other occurrences of "time of impact" to distance
  • Added missing predicate versions of spatial query methods which existed on SpatialQueryPipeline, but not SpatialQuery
  • Added target_distance and compute_impact_on_penetration configuration options for shape casts, to match Parry's capabilities
  • Fixed several documentation issues, such as:
    • Shape hit data is in world space, not local space
    • Many intra-doc for "See also" sections link to self, not other methods
    • Mentions of "ray" in shape casting docs
    • Confusing wording for predicate argument docs
  • Improved documentation examples
  • Improved clarity and readability in a lot of docs
  • Changed excluded_entities to use an EntityHashSet

Migration Guide

Ray Casting and Shape Casting Configuration

SpatialQuery methods like cast_ray and ray_hits now take a RayCastConfig, which contains a lot of the existing configuration options.

let origin = Vec2::ZERO;
let direction = Dir2::X;

// Before
let hit = spatial.cast_ray(origin, direction, 100.0, true, &SpatialQueryFilter::default());

// After
let config = RayCastConfig::from_max_distance(100.0);
let hit = spatial.cast_ray(origin, direction, &config);

Shape casts have received a similar treatment. For example, shape_hits:

// Before
let hits = spatial.shape_hits(
    &Collider::sphere(0.5),
    Vec3::ZERO,
    Quat::default(),
    Dir3::ZERO,
    100.0,
    10,
    false,
    &SpatialQueryFilter::from_mask(LayerMask::ALL),
);

// After
let config = ShapeCastConfig {
    max_distance: 100.0,
    filter: SpatialQueryFilter::from_mask(LayerMask::ALL),
    ..default()
};
let hits = spatial.shape_hits(
    &Collider::sphere(0.5),
    Vec3::ZERO,
    Quat::default(),
    Dir3::ZERO,
    10,
    &config,
);

Why was the API changed? Separating the configuration options improves readability and documentation, allows the config to be reused, provides sane default values for many options, and makes it possible for Avian to add more configuration options in the future without bloating the method calls further. In many cases, it can also just be more ergonomic and shorter, as several options can typically use default values.

Time of Impact → Distance

Spatial query APIs that mention the "time of impact" have been changed to refer to "distance". This affects names of properties and methods, such as:

  • RayCaster::max_time_of_impactRayCaster::max_distance
  • RayCaster::with_max_time_of_impactRayCaster::with_max_distance
  • ShapeCaster::max_time_of_impactShapeCaster::max_distance
  • ShapeCaster::with_max_time_of_impactShapeCaster::with_max_distance
  • RayHitData::time_of_impactRayHitData::distance
  • ShapeHitData::time_of_impactShapeHitData::distance
  • max_time_of_impact on SpatialQuery methods → RayCastConfig::max_distance or ShapeCastConfig::max_distance

This was changed because "distance" is clearer than "time of impact" for many users, and it is still an accurate term, as the cast directions in Avian are always normalized, so the "velocity" is of unit length.

@Jondolf Jondolf added C-Enhancement New feature or request A-Spatial-Query Relates to spatial queries, such as ray casting, shape casting, and intersection tests X-Contentious There are nontrivial implications that should be thought through C-Usability A quality-of-life improvement that makes Avian easier to use labels Sep 7, 2024
@Jondolf Jondolf added this to the 0.2 milestone Sep 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Spatial-Query Relates to spatial queries, such as ray casting, shape casting, and intersection tests C-Enhancement New feature or request C-Usability A quality-of-life improvement that makes Avian easier to use X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The documentation of ShapeHitData says the points are in local space, but they're in world space
1 participant