EntityRef/EntityMut deduplication

[ItsDoot]

Objective

We can observe that EntityRef, FilteredEntityRef, and EntityRefExcept provide the same kind of access: reading components from a certain entity. However, they differ in what they're allowed to access. EntityRefs can access all components, while FilteredEntityRef's access is determined at runtime and EntityRefExcept's at compile time.

A problem arises where each new feature for entity reference types must be duplicated up to six(!) times. This is terrible for code review and maintainability! The solution below reduces the necessary duplication down to just two: one variant for immutable entity references and one variant for mutable ones.

Solution

All three immutable entity reference types (and all three mutable types) have the same mode of operation, so they can be collapsed into just EntityRef<A: AsAccess = All> and EntityMut<A: AsAccess = All>. AsAccess is a new trait that checks read and write access for a given component:

pub trait AsAccess: Copy + Deref<Target=Access> {}

This PR provides the following three accesses:

• All: has access to all components. It is the default access, so when you see a plain EntityRef or EntityMut, it will act exactly the same as before.
• Filtered: access is dictated by a held &Access. It acts exactly the same as FilteredEntityRef and FilteredEntityMut, which are now type aliases of EntityRef<Filtered> and EntityMut<Filtered>, respectively.
• Except<B: Bundle>: access is dictated by the B generic parameter. Just like EntityRefExcept and EntityMutExcept, it too holds an &Access. EntityRefExcept<B> and EntityMutExcept<B> are now type aliases of EntityRef<Except<B>> and EntityMut<Except<B>>, respectively.

Notes for reviewers

• Functions that are only safe on EntityRef<All> and EntityMut<All> have been moved to a new all.rs module.
• The new safety comments feel a bit word-salady, but I tried to cover the bases as best I could. Alternative suggestions welcome!
• Could probably do with some additional docs.

Testing

Added tests for the newly introduced AsAccesss. Otherwise reusing current tests.

Looking ahead

Here's what I have plans for after this big collapse:

• De-traitify EntityRef/Mut::get_by_id family of functions, and rename them to get_untyped, get_many_untyped, iter_many_untyped, iter_untyped, etc.
• Add new family of functions that return dyn Reflect instead of Ptr: get_reflect, get_many_reflect, iter_many_reflect, iter_reflect, etc.
• Replace EntityWorldMut with EntityMut<Global>, introducing a new Global access. This means we get EntityRef<Global> AKA EntityWorldRef for free.
• Add EntityRef<Only<B>> and EntityMut<Only<B>>, introducing a new Only access. This would allow us to split EntityMuts into pairs of EntityMutExcept and EntityMutOnly, providing an alternative to EntityMut::get_components_mut.

[github-actions]

It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note.

Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes.

[hymm]

I'm not sure if the changes to UnsafeEntityCell make sense. I don't think we want to add checks there as these are supposed to be low overhead methods where the caller should be verifying the access if they think it is necessary.

[hymm]

This is just a high level review. Generally onboard with these changes. It's nice that the new generic doesn't show up in user facing apis. We should probably run benches for any of the methods that we have them for, and I think we should do some spot checking comparing the before and after for the assembly generated.

[hymm]

Suggested change

	pub fn scope(&self) -> &S {
	pub fn access(&self) -> &S {

I think scope is a bit ambiguous here. Scope by itself is more a reference to lifetime scopes (or block, function, etc.)

[ItsDoot]

I'm down for a rename, but lets also rename AccessScope too; a few suggestions:

• AccessPolicy
• AccessBounds
• ComponentAccess a bit redundant since Access already uses ComponentIds
• AccessFilter

[Diddykonga]

AccessKind
EntityAccess - Less precise if it works with World in the future

[chescock]

I'm down for a rename, but lets also rename AccessScope too

What about something like AsAccess? Another way of thinking about this is that we always have an Access, but for All we compress the &'static Access to a ZST.

Hmm, for that matter, would it work to use something like AsRef<Access> or Deref<Target = Access>? In theory, the compiler should be able to inline the Access::new_mutable call and then see that can_read always passes and still eliminate the whole thing. We'd have to test whether that works in practice, of course.

[ItsDoot]

I've updated it to be trait AsAccess: Copy + Deref<Target = Access>. Thoughts?

[ItsDoot]

I'm not sure if the changes to UnsafeEntityCell make sense. I don't think we want to add checks there as these are supposed to be low overhead methods where the caller should be verifying the access if they think it is necessary.

What would an alternative look like? Since we need access to the fetched ComponentId even for the generic <T> functions, I believe we would either need to perform a double-lookup, or duplicate the entire logic of each UnsafeEntityCell function to avoid the double-lookup.

It should still be low overhead for power users if they provide an All scope (worth comparing the asm output), but maybe we could provide a Manual scope that always returns true (like All does), for documentation purposes?

[chescock]

Oh, yay, glad this is back on track! I left some thoughts, and I agree that we should bikeshed scope a bit, but everything looks reasonable!

I also agree that it's worth spot-checking some of the generated assembly. I've found https://github.com/pacak/cargo-show-asm helpful for that.

[chescock]

I'm down for a rename, but lets also rename AccessScope too

What about something like AsAccess? Another way of thinking about this is that we always have an Access, but for All we compress the &'static Access to a ZST.

Hmm, for that matter, would it work to use something like AsRef<Access> or Deref<Target = Access>? In theory, the compiler should be able to inline the Access::new_mutable call and then see that can_read always passes and still eliminate the whole thing. We'd have to test whether that works in practice, of course.

[chescock]

Should this be a new error variant? It's a little misleading to report that the component isn't found when the issue is really that we couldn't access it. Although it would be unfortunate to make callers that want to be exhaustive with EntityMut::get_mut_by_id have to handle an impossible case.

[ItsDoot]

This latest commit implements @chescock's suggestion for an AsAccess-style solution, and renames things to follow suit. It does greatly simplify things, but I am curious if the All case is properly inlined. Even if it doesn't, I don't think that will be a noticeable problem since EntityRef/EntityMut are not typically used on the hot path.

I only minimally updated the docs for this rename so that might need an additional pass.

cc @hymm

[chescock]

This looks very clean!

I do think we should check the assembly to make sure the checks are removed with All access, but I guess even the unoptimized version is just reading a bool, so it might not matter even if it's left in.

[Diddykonga]

LGTM, and that ratio sure is nice 😃