aevyrie / bevy_mod_raycast

A little mesh raycasting plugin for Bevy
https://crates.io/crates/bevy_mod_raycast
MIT License
304 stars 92 forks source link

`Intersection` API updates #69

Closed chrisjuchem closed 1 year ago

chrisjuchem commented 1 year ago

This adds a few features and simplifies some of the mod's API.

encounter commented 1 year ago

I have a fix for the debug cursor here: 5769e48316239156e25b674afb3d1e1a7b067523

It assumes there's only ever one intersection for a given raycast set, not sure if that's a valid assumption.

aevyrie commented 1 year ago

Can you provide a justification for this change? I don't understand what problem this is solving. In addition, all the examples appear to be broken.

I would also need a good reason to justify adding and removing components on the fly. Especially for something like raycasting where it can be happening quite frequently, this is bad for performance (it causes entity archetype changes) and easily leads to frame delays due to command propagation.

encounter commented 1 year ago

The commit I posted should fix the examples.

Can you provide a justification for this change? I don't understand what problem this is solving.

Currently, this documentation is wrong:

https://github.com/aevyrie/bevy_mod_raycast/blob/bd84951952f58a36c8144725f6539e7b3f60ed0f/src/primitives.rs#L71-L73

This PR fixes that behavior. This allows users to perform queries like

Query<Entity, With<Intersection<T>>>

to fetch the entity that the ray intersected with. This behavior makes sense, in my opinion.

Right now, that query will return an empty entity created just for the Intersection component. There's also no reference from IntersectionData to the intersected entity. The intersected entity ID only lives on the RaycastSource, which begs the question of why the Intersection component even exists. Overall, even if the documentation was updated to properly reflect this, I don't find it very intuitive.

I would also need a good reason to justify adding and removing components on the fly. Especially for something like raycasting where it can be happening quite frequently, this is bad for performance (it causes entity archetype changes) and easily leads to frame delays due to command propagation.

This would yield a maximum of 2 component updates per RaycastSource, no? That's assuming that the ray intersects a different entity every frame. Is it expected to be casting more than a couple of rays?

Just looked up entity archetype changes, that seems like an interesting thing to consider. I'm curious how impactful it is here.

encounter commented 1 year ago

Just noticed that #60 is related to this behavior as well.

aevyrie commented 1 year ago

@encounter would you be willing to take over this PR? This would involve incorporating your fix commit, and working with me to get this to a mergeable state.

There is definitely an error in the documentation. The current impl, and the top level doc for Intersection, is:

Holds the topmost intersection for the raycasting set T.

Which implies there should only be one Intersection component per raycast set T. My preference is that we remove the Intersection type entirely, and better document the intended use, which is to use https://docs.rs/bevy_mod_raycast/latest/bevy_mod_raycast/struct.RaycastSource.html#method.get_intersections

The reason I'm not excited about adding an Intersection component to every RaycastMesh is twofold:

  1. From a learning POV, components are supposed to add behavior. A RaycastMesh is already used to make entities raycast receivers. If anything, intersection data should be added as a field/method on RaycastMesh.
  2. We would need to be careful about performance with large numbers of entities. Adding/removing components is usually better to avoid, instead opting to use a bool field on the component that can be toggled. I want to support raycasting without early-exit (all intersections, not just the first one), which could result in a lot of archetype changes, not just 2 per raycasting source.

I definitely see the value in being able to query the ray targets, rather than the sources. To do this while meeting the above two requirements, would result in adding an intersection field to RaycastMesh<T>. Maybe something like:

pub struct RaycastMesh<T: Reflect> {
    intersection: Option<IntersectionData>,
    _marker: PhantomData<T>,
}

In which case, we would be able to do:

query: Query<RaycastMesh<GunRaycasts>, With<Enemy>>
// ...
if let Some(intersection) = enemies.get_intersection() {
    // Do things
}

Thoughts?

jim-ec commented 1 year ago

@aevyrie I tried implementing your approach in #81.