-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
Make WorldQuery use &World for initialization
#22670
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 8 commits
2469aa5
bbd7033
e4adb46
05ebb69
083e678
3c7ae5d
817dcf5
b71036f
256d34a
211774c
af64884
f701804
0fea178
98596ba
8256518
a2d8cd2
a039b5f
972b8a3
c6cb619
18aadd9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -205,6 +205,7 @@ pub use server::*; | |||
| pub use uuid; | ||||
|
|
||||
| use crate::{ | ||||
| asset_changed::AssetChanges, | ||||
| io::{embedded::EmbeddedAssetRegistry, AssetSourceBuilder, AssetSourceBuilders, AssetSourceId}, | ||||
| processor::{AssetProcessor, Process}, | ||||
| }; | ||||
|
|
@@ -650,6 +651,7 @@ impl AssetApp for App { | |||
| )); | ||||
| } | ||||
| self.insert_resource(assets) | ||||
| .init_resource::<AssetChanges<A>>() | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the reason this used to be initialized only in the bevy/crates/bevy_asset/src/assets.rs Line 594 in 78166fb
I don't know enough about assets to evaluate how important that is, though. If we do need to preserve that behavior, I think there are still ways to do it with only
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it's not ideal. I'd like more information about the perf implications here.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have a strong opinion here. It's quite unfortunate to have this running for every asset type though - some asset types may mutate frequently (e.g., Materials) but those asset types are also most likely to actually take advantage of this feature. I wouldn't block on preserving this unless folks complain (which seems unlikely). With assets-as-entities unblocked, we may also throw out this type entirely, so putting the work in to fix it seems a little early. |
||||
| .allow_ambiguous_resource::<Assets<A>>() | ||||
| .add_message::<AssetEvent<A>>() | ||||
| .add_message::<AssetLoadFailedEvent<A>>() | ||||
|
|
||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -433,11 +433,15 @@ fn hook_on_add<E: Event, B: Bundle, S: ObserverSystem<E, B>>( | |
| ) { | ||
| world.commands().queue(move |world: &mut World| { | ||
| let event_key = world.register_event_key::<E>(); | ||
| let components = B::component_ids(&mut world.components_registrator()); | ||
| let components = B::component_ids(&mut world.components_registrator()) | ||
| .collect::<smallvec::SmallVec<[ComponentId; 16]>>(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understand correctly, this But this only actually allocates when creating an observer with more than 16 components, which should basically never happen and which is already allocating to box the system, so the cost is pretty low.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly. We could monomorphize |
||
|
|
||
| if let Some(mut observer) = world.get_mut::<Observer>(entity) { | ||
| observer.descriptor.event_keys.push(event_key); | ||
| observer.descriptor.components.extend(components); | ||
| observer | ||
| .descriptor | ||
| .components | ||
| .extend_from_slice(&components); | ||
|
|
||
| let system: &mut dyn Any = observer.system.as_mut(); | ||
| let system: *mut dyn ObserverSystem<E, B> = system.downcast_mut::<S>().unwrap(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a totally fine limitation: this filter is useless without initialized assets.