Skip to content
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

Add World::query_scope API #4157

Closed
alice-i-cecile opened this issue Mar 8, 2022 · 5 comments
Closed

Add World::query_scope API #4157

alice-i-cecile opened this issue Mar 8, 2022 · 5 comments
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use

Comments

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Mar 8, 2022

What problem does this solve or what need does it fill?

World::resource_scope is very useful for complex access patterns in exclusive systems.
However, it doesn't work with queries.

What solution would you like?

Add `fn query_scope(|q: Query<Q: WorldQuery, F: WorldQuery + FilterFetch>, world: &mut World |{}).

This temporarily removes all matching entities from the World, splitting the borrow.

What alternative(s) have you considered?

Use #4090 or SystemState instead. This won't cover all use cases: there are some things that can only be done with a &mut World.

Expand WorldCell to support queries, per #1555. This is substantially more error prone and slower, as the borrow-checking is done at runtime instead.

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Mar 8, 2022
@maniwani
Copy link
Contributor

maniwani commented Mar 18, 2022

Have we ever considered having archetypes store a RefCell<()> for each component column? (ArchetypeComponentId)

Assuming the <F as Fetch>::Item (and <F as SystemParamFetch>::Item) types could borrow them, we could remove...

  • the ReadOnlyFetch constraint on QueryState::get
  • the ReadOnlySystemParamFetch constraint on SystemState::get
  • etc.

...and just have runtime borrow-checking as the default behavior (complete with a try_get method that won't panic).

If this sounds sane, I like it more than all previous proposals.

(For context, RefCell<()> is the same size as two isize values, so pretty small)

@alice-i-cecile
Copy link
Member Author

I'd strongly prefer if we kept runtime panics of that sort opt-in. One of my favorite things about Rust is "if it compiles it works", and there are likely to be dark corners that are rarely run.

@maniwani
Copy link
Contributor

maniwani commented Mar 18, 2022

Borrowing a RefCell just increments or decrements an internal isize depending on the type of borrow. It should be super efficient, certainly no worse than the runtime borrow-checking systems and the executor already do with Access<ArchetypeComponentId> to carve up World access.

But even before that, using queries and system states on &mut World is already "giving up" performance, so IMO perf falls behind ergonomics here, and taking still prevents shared access to a degree unlike runtime borrow-checking.

I'm sure a query_scope has a niche, but I don't like expanding the number of types and APIs to address something that IMO could be done as the general case. Would also present a nicer symmetry between system states and systems.

(edit: could still keep the get_mut methods, just "safe" get methods would be borrow-checked)

@alice-i-cecile
Copy link
Member Author

I agree, it should be very efficient (and we don't really care about performance checking anyways). I was mostly nervous about the possibility of introducing more code that passes the compiler, but always fails the first time it's run.

On reflection though, query_scope and resource_scope has exactly the same problem. Okay, you've convinced me! My feeling is that this is a much nicer solution to the problems that WorldCell is trying to solve.

@makspll
Copy link
Contributor

makspll commented May 2, 2022

I think something like this is essential for scripting components, I am currently working on a scripting plugin, and a big issue is the fact that script components live in the world.

my scripting system essentially needs to iterate over all the scripts, but at the same time let them modify the world. This causes an issue since accessing the scripts already requires a world borrow, I don't see a work around for this short of using a resource to store all of the script contexts, since then I can use scoped resource access and get rid of the first world borrow.

And I don't think using a non-exclusive system is a possibility, since script API's need flexible instantaneous access to everything.

@alice-i-cecile alice-i-cecile closed this as not planned Won't fix, can't repro, duplicate, stale May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

No branches or pull requests

3 participants