Only get valid component ids#19510
Conversation
ElliottjPierce
added
C-Bug
| /// Note that [`Self::get()`] may still return `Err` if the resource does not exist. | ||
| pub fn has_read<R: Resource>(&self) -> bool { | ||
| let component_id = self.world.components().resource_id::<R>(); | ||
| let component_id = self.world.components().valid_resource_id::<R>(); |
There was a problem hiding this comment.
To make sure I understand the reasoning on most of these: The component must be valid before it can actually be inserted. So if we're trying to access component data, then we can ignore queued component ids, since even with the ID we'd never find any data.
I don't think that argument applies to FilteredResources::has_read, FilteredResourcesMut::has_read, or FilteredResourcesMut::has_write. It's possible to grant access to this by component id, in which case it may have access to a component that's only queued. And I don't think these methods are called much, if at all, so it should be fine if they have slightly worse perf.
But it's not a big deal either way, since a user who wants the other behavior can always just get the ID and check the access themselves.
There was a problem hiding this comment.
That's fair. But IDK why anyone would make a FilteredResource, etc, and then not try to use it to get the resource.
But I'd be happy to revert those; that does seem more technically correct. Anyone have a strong opinion?
There was a problem hiding this comment.
I'd prefer a revision for the filtered cases here. I think it's slightly more correct, and it's rare enough that the perf edge case doesn't bother me.
alice-i-cecile
added
S-Ready-For-Final-Review
# Objective - bevyengine#19504 showed a 11x regression in getting component values for unregistered components. This pr should fix that and improve others a little too. - This is some cleanup work from bevyengine#18173 . ## Solution - Whenever we expect a component value to exist, we only care about fully registered components, not queued to be registered components since, for the value to exist, it must be registered. - So we can use the faster `get_valid_*` instead of `get_*` in a lot of places. - Also found a bug where `valid_*` did not forward to `get_valid_*` properly. That's fixed. ## Testing CI
5 participants
Objective
Solution
get_valid_*instead ofget_*in a lot of places.valid_*did not forward toget_valid_*properly. That's fixed.Testing
CI