Improve SpatialQuery
APIs and docs, and add more configuration
#510
+852
−473
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:Without variables for everything, it would be almost entirely unreadable:
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:
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:
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:(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
tomax_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
RayCastConfig
. Most ray casting methods now take the config instead of many separate argumentsShapeCastConfig
. Most shape casting methods now take the config instead of many separate argumentsRayHitData::time_of_impact
,ShapeHitData::time_of_impact
, and many other occurrences of "time of impact" todistance
SpatialQueryPipeline
, but notSpatialQuery
target_distance
andcompute_impact_on_penetration
configuration options for shape casts, to match Parry's capabilitiespredicate
argument docsexcluded_entities
to use anEntityHashSet
Migration Guide
Ray Casting and Shape Casting Configuration
SpatialQuery
methods likecast_ray
andray_hits
now take aRayCastConfig
, which contains a lot of the existing configuration options.Shape casts have received a similar treatment. For example,
shape_hits
: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_impact
→RayCaster::max_distance
RayCaster::with_max_time_of_impact
→RayCaster::with_max_distance
ShapeCaster::max_time_of_impact
→ShapeCaster::max_distance
ShapeCaster::with_max_time_of_impact
→ShapeCaster::with_max_distance
RayHitData::time_of_impact
→RayHitData::distance
ShapeHitData::time_of_impact
→ShapeHitData::distance
max_time_of_impact
onSpatialQuery
methods →RayCastConfig::max_distance
orShapeCastConfig::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.