-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Store Resources as components on singleton entities #20934
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?
Store Resources as components on singleton entities #20934
Conversation
… into resource_entity_lookup
The access control is currently not enough since #[test]
fn resource_conflict() {
use crate::prelude::*;
#[derive(Resource)]
struct Foo;
let mut world = World::default();
world.insert_resource(Foo);
fn system(mut q: Query<&mut Foo, With<Internal>>, r: ResMut<Foo>) {
let _foo1 = q.single_mut().unwrap();
let _foo2 = r;
unreachable!("This should not be possible")
}
world.run_system_once(system);
} |
IsResource > IsSingleton |
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.
I might add something more later, as right now I'm on a train with a very bad internet connection...
// conflicts with any prior access, a panic will occur. | ||
// SAFETY: ResMut component access is tied to the singleton components under | ||
// QueryState<Ref<T>, With<Internal>>, which guarantees safety. | ||
unsafe impl<'a, T: Resource> SystemParam for Res<'a, T> { |
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.
Can we add a test that this works "correctly" when a resource requires another resource? E.g. ResA
requires ResB
, then you insert ResA
and try to access Res<ResB>
. This should return an error because ResB
should not exist in the world as a resource.
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.
For that I need to check whether or not ResB
s component id is in resource_entities
or not, which I don't know how to do without using unsound code (previous atttempt you commented on).
Unless you know a relatively easy way to do it, I think it's probably best if we forbid using require(Resource)
on a resource untill we figure out all of the semantics.
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.
Unless you know a relatively easy way to do it, I think it's probably best if we forbid using require(Resource) on a resource untill we figure out all of the semantics.
That doesn't seem easy to do. You would have to introduce even more hacks in the derive macro (already the fact it calls the derive for Component
is very hacky!). You would probably have to forbid require(Component)
in general, not just require(Resource)
. Moreover users can register required components at runtime, and that seems even harder to prevent.
The way I see it this is a consequence of this PR trying to do two things at the same time:
- store resources as components on entities
- make resources actual components
IMO we should avoid doing the second point until we figure out all its implications. As long as it's not required for the first part it seems just an added complication.
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.
I agree, but supporting #[require(Foo)]
for resources is specifically necessary for this PR to merge.
This is due to the fact AmbientLight
in bevy_light
is used as both a Component
and a Resource
, requiring Camera
in the case it is a component.
This should both be a short-lived problem, since the rendering folks have ensured me that AmbientLight
will be deprecated in 0.18, but for now Resource
s need to support require
.
The larger problem is structs that both derive Component
and Resource
. For the sake of compatibillity, we have to allow all of the Component attributes to work with Resource.
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 under the assumption that deriving Resource
also derives Component
, no? Delaying that until a solution is found should pose no problem to bevy_light
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.
Yes, but in order to store resources on the world, they have to be components, so I don't understand how you could get around that.
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.
Btw, is this the last problem you see with this PR? Assuming I can fix CI, where do you want this PR to be before you approve it?
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.
Yes, but in order to store resources on the world, they have to be components
You can just wrap them in a newtype that implemente Component
and store that in the world.
Btw, is this the last problem you see with this PR?
There were a bunch of comments that you marked as solved without any discussion or evident fix. Maybe I missed the change that fixes them, but it's not simple and quite time-expensive to look for them so it would be helpful if you commented on them with why/how they were resolved.
|
||
move_as_ptr!(value); | ||
|
||
entity_mut.insert_with_caller( |
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.
since this calls flush, the ticks aren't "correct" here yet, so, as in the new test, an observer that's interested in the tick would read a different value when called from the closure than from outside.
I also think you could use the BundleSpawner here to spawn the bundle without flushing, then flush at the end of the function once the state is fully restored again.
// update them to the entities in the world. | ||
SceneEntityMapper::world_scope(entity_map, world, |world, mapper| { | ||
reflect_component.apply_or_insert_mapped( | ||
&mut world.entity_mut(*entity), |
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 the wrong entity, the world in the closure is the main world.
I believe the block spawning all entities in the main world should be above this resource block
Couldn't these types implement a Edit: seems I am a bit behind on how immutable components work, but I think the implementation might still be interesting for uniqueness. |
This is part of #19731.
Resources as Components
Motivation
More things should be entities. This simplifies the API, the lower-level implementation and the tools we have for entities and components can be used for other things in the engine. In particular, for resources, it is really handy to have observers, which we currently don't have. See #20821 under 1A, for a more specific use.
Current Work
This removes the
resources
field from the world storage and instead store the resources on singleton entities. For easy lookup, we add aHashMap<ComponentId, Entity>
toWorld
, in order to quickly find the singleton entity where the resource is stored.Because we store resources on entities, we derive
Component
alongsideResource
, this means thatturns into
This was also done for reflections, meaning that
becomes
In order to distinguish resource entities, they are tagged with the
IsResource
component. Additionally, to ensure that they aren't queried by accident, they are also tagged as being internal entities, which means that they don't show up in queries by default.Drawbacks
Resource
and aComponent
, becauseResource
expands to also implementComponent
, this means that this throws a compiler error as it's implemented twice.ReflectComponent
you need to importbevy_ecs::reflect::ReflectComponent
every time you use#[reflect(Resource)]
. This is kind of unintuitive.Future Work
Access
in the ECS, to only deal with components (and not components and resources).Res<Resource>
toSingle<Ref<Resource>>
(or something similair).ReflectResource
.