bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Published 20 hours ago • verified publisher icon bevyengine

Extract Resources into their own dedicated storage

Open james7132 opened this issue 2 years ago • 4 comments

Objective

Resources are currently stored as a dedicated Resource archetype (ID 1). This allows for easy code reusability, but unnecessarily adds 72 bytes (on 64-bit systems) to the struct that is only used for that one archetype. It also requires several fields to be pub(crate) which isn't ideal.

This should also remove one sparse-set lookup from fetching, inserting, and removing resources from a World.

Solution

  • Add Resources parallel to Tables and SparseSets and extract the functionality used by Archetype in it.
  • Remove unique_components from Archetype
  • Remove the pub(crate) on Archetype::components.
  • Remove ArchetypeId::RESOURCE
  • Remove Archetypes::resource and Archetypes::resource_mut

Changelog

Added: Resources type to store resources. Added: Storages::resource Removed: ArchetypeId::RESOURCE Removed: Archetypes::resource and Archetypes::resources Removed: Archetype::unique_components and Archetypes::unique_components_mut

Migration Guide

TODO

james7132 avatar May 20 '22 06:05 james7132

@james7132 can you fix this up when you get the chance? I think the fixes should be straightforward, and I'd like to unblock #4955.

alice-i-cecile avatar Jun 07 '22 04:06 alice-i-cecile

Ping @BoxyUwU for attempt 5 to remember to review this 😂

alice-i-cecile avatar Jun 11 '22 16:06 alice-i-cecile

@alice-i-cecile @BoxyUwU after realizing inserting and removing resources from the storage API was basically impossible to use safely from outside of bevy_ecs, I reworked a lot of this to operate with ResourceInfo instead.

james7132 avatar Jun 15 '22 12:06 james7132

@plof27 I want your review on this too due to the impact on #1481.

alice-i-cecile avatar Jun 24 '22 19:06 alice-i-cecile

Needs a Migration Guide.

alice-i-cecile avatar Oct 16 '22 05:10 alice-i-cecile

[1:32 AM]Alice 🎃: Do you think this deserves the controversial label? I guess I don't really think that this is a particularly challenging PR, or something that anyone opposes [1:34 AM]james7132: Personally, no. Though the unsafe in it was what warranted the label initially I think.

Removing the controversial label; I don't think this is warranted. @JoJoJet @DJMcNab @TheRawMeatball I'd like an approval from one of you + Boxy as well though.

alice-i-cecile avatar Oct 16 '22 05:10 alice-i-cecile

I think all the unsoundness to do with !Send stuff would just disappear if we made World: !Send hold like it ought to. I don't know whether its worth blocking this PR on the !Send unsoundness since you could reasonably just blame the unsound World: Send impl (#3310).

BoxyUwU avatar Oct 17 '22 16:10 BoxyUwU

Unlike !Send though, resources can also be !Sync which makes the resource.rs methods that let you go &self -> Ptr<'_> wrong for the same reason as the !Send unsoundness. You could probably also blame the World: Sync impl here but iunno, the Send/Sync on world are kind of a mess and I don't know what we're gonna do here "long term".

Safest is probably to not merge this PR until all the methods are made unsafe with appropriate safety invariants to do with send/syncness :upside_down_face: I am not sure if we actually have any unsoundness Today to do with World: Sync so we definitely cant merge this if we're adding some, unlike World: Send where there is at least some form of consensus that World: Send is wrong

BoxyUwU avatar Oct 17 '22 16:10 BoxyUwU

Brainfarted here and forgot about the prior effort to make mutating accesses in bevy_ecs non-pub. insert/remove and friends should probably not be pub, and get_mut should be more scoped than giving direct access to ResourceData. I'll add these changes soon.

Unlike !Send though, resources can also be !Sync which makes the resource.rs methods that let you go &self -> Ptr<'_> wrong for the same reason as the !Send unsoundness. You could probably also blame the World: Sync impl here but iunno, the Send/Sync on world are kind of a mess and I don't know what we're gonna do here "long term".

This is interesting since the existing unique_components API also currently allows this IIRC. Not that we shouldn't fix this, but pointing out that this isn't necessarily a regression.

james7132 avatar Oct 17 '22 16:10 james7132

Thinking on this a bit more, Resources within a World are always read-only as there is no public World::storages_mut. Perhaps it might be best to make all data related accesses, mut or not, pub(crate).

james7132 avatar Oct 18 '22 00:10 james7132

I think it would be fine to make it all pub(crate) since we ought to be covering all the require functionality as World methods anyway :thinking: doesnt really change that these methods are all kinda busted with the current send/sync impls though, even if its not publicly a problem its still a huge hazard for maintaining bevy

BoxyUwU avatar Oct 18 '22 01:10 BoxyUwU

There's still (some?) value in being able to get the change ticks and the ArchetypeComponentId. Is that worth exposing at all?

james7132 avatar Oct 18 '22 02:10 james7132

I think we can do those in follow-up PRs. It's useful, but not essential.

alice-i-cecile avatar Oct 18 '22 02:10 alice-i-cecile

This is ready: I agree with Boxy's assessment and think this makes reasoning about the internals of resources much nicer. Small perf improvements are a nice bonus.

bors r+

alice-i-cecile avatar Oct 24 '22 13:10 alice-i-cecile

Pull request successfully merged into main.

Build succeeded:

bors[bot] avatar Oct 24 '22 14:10 bors[bot]