search

Ralith / hecs

A handy ECS
Apache License 2.0
967 stars 82 forks source link

Refactor World::get to take reference types #274

Closed Ralith closed 2 years ago

Ralith commented 2 years ago

Many users have reported confusion arising from the use of component types in World::get in contrast to the use of reference sin World::query and friends. This replaces get and get_mut on World and EntityRef with a single get that is generic over references to components, so that the intuitive thing Just Works, and passing an explicit component type generates a trait error.

For consistency, EntityBuilder:;get and get_mut are both refactored similarly, although they cannot be combined into a single method as they do not perform dynamic borrow checking.

This also resolves the inconsistency between get_mut referring to the mutability of the component borrow, while other _mut methods on World instead refer to the mutability of the world borrow.

adamreichold commented 2 years ago

While I have not formed an opinion of whether this simplifies explaining things (In my mental model, World::get(_mut) were the primitives out of which the queries are built via their DSL.), I wanted to add that the query combinators potentially have a similar foot gun, e.g. Query<With<(&R, &S), &T>> does not work as one might expect and the correct Query<With<(&R, &S), T>> might be surprising. (I guess it isn't completely the same as &T and &mut T imply no difference for the query combinator.)

Ralith commented 2 years ago

I've received many independent user reports of footgun-related injuries here, so I'm convinced of the benefit.

With/Without are an interesting point. I've also been a bit unhappy with how they have to be nested to consider multiple components. Maybe we should refactor those to operate on two queries, rather than a query and a component type? Logically equivalent to the 2-tuple combinator, except that the results for the second item are not yielded (or borrowed).

adamreichold commented 2 years ago

Maybe we should refactor those to operate on two queries, rather than a query and a component type?

This sounds sensible and would also be nicely consistent with Satisfies.

Ralith commented 2 years ago

Comically, I have just now discovered a bug in my own project that this would have prevented. Will probably publish a release soon.