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

Allow users to blend &mut World with other system parameters using a "scoped World" pattern #4158

Closed
7 tasks
alice-i-cecile opened this issue Mar 8, 2022 · 8 comments
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible

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?

The hard line between exclusive and non-exclusive systems results in very frustrating UX: users must learn a whole new API with complex borrow-checking rules in order to access the world.

Many convenient, ergonomic APIs (like EventReader) must be manually reimplemented or worked around, encouraging code duplication both within the engine and in user code.

What solution would you like?

  1. Other system parameters can coexist with &mut World.
  2. Conflicts are checked on the basis of these other system parameters.
  3. The World returned by &mut World is incomplete: any data required by other system parameters is removed from it for the duration of the system.
  4. Under the hood, this uses resource_scope and other parallel APIs.

Status

This is a large task, and should be done in a piecemeal fashion.

As a result, it seems like there's a split between "scopable" and "non-scopable" system parameters, that we should probably try to reflect in the type system somehow.

What alternative(s) have you considered?

WorldCell is an API that exists largely to get around this pain. #3939 proposes removing it completely, due to its limitations and runtime-panicking nature.

#4090 also attempts to relieve this pain, but there are some things that genuinely require &mut World access.

Additional context

Dissolving the boundaries between exclusive and regular (FunctionSystem) systems is one of the incidental goals behind the Stageless RFC. This design would help further that process.

#4157 is a necessary step to allow us to extend this work to queries (and hence entity-component data).

This is likely to be useful when working with e.g. saving the game, working with scenes or networking, as we commonly want to operate on a massive scale across the whole world, except for some data that is used for coordinating the task.

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events labels Mar 8, 2022
@maniwani
Copy link
Contributor

maniwani commented Mar 9, 2022

In the "stageless" test branch I'm working on, I've been able to make &mut World a SystemParam, which accomplishes removing the internal distinction between exclusive and non-exclusive systems. (Everything is a FunctionSystem). That also accomplishes compatibility with the local system params.

I could split that out into a PR. (It's just the MutPtr<'world, World> ptrfication stuff I mentioned on Discord.) Seems strictly better than introducing new scoped APIs.

@alice-i-cecile
Copy link
Member Author

I could split that out into a PR.

Yes please: we should do everything possible to make the stageless work easier to review.
It's also a necessary pre-requisite of this case.

Seems strictly better than introducing new scoped APIs.

I think I may have failed to explain the point properly. We can't have a system with both &mut World and Res<Time> in its parameters currently. However, in 99% of cases, the resource scope behavior will give users the desired, intuitive behavior: access to the resource they requested plus access to the rest of the world.

This becomes particularly gnarly with complex exclusive systems: you end up having to use deeply nested resource_scope closures, swap to WorldCell or deal with SystemState.

IMO we should move as much information about the data that these systems need to the type signature as possible for ergonomics.

Incidentally, this also enables us to run systems with &mut World in parallel with some other systems!

Suppose we have

fn exclusive_events(world: &mut World, events: EventReader<Foo>) {}

fn simple_events(events: EventReader<Foo>) {}

Because we've moved the resource scope to the system parameter level, we can actually run these systems in parallel! exclusive_events has no way* to mutate the Events<Foo> resource, and so we can make stronger inferences about compatibility.

*requires some cleverness about how we re-merge the scoped world back in (or limitations on what the scoped world can do)

@maniwani
Copy link
Contributor

maniwani commented Mar 9, 2022

We can't have a system with both &mut World and Res<Time> in its parameters.

If we just want to retain the ability to coerce any system to be exclusive without &World or &mut World, we can just add an empty token Exclusive system param that produces the same access conflicts with other systems but doesn't conflict with other params in the same system (except &World and &mut World).

Incidentally, this also enables us to run systems with &mut World in parallel with some other systems!

I'd reeeeeeeeeeeeaaaaaaaaaly rather not.

  1. Introducing systems that do resource_scope under the hood (or params that take ownership and return it on completion) needs its own discussion. Personally, I think that creates too much overlap with bevy_tasks, which IMO exists specifically for doing stuff with owned data asynchronously with systems running in the schedule.
  2. Removing one dichotomy just to add another doesn't feel like a win, especially considering you can't use anything but resources and locals in these hypothetical scoped systems.
  3. If these systems were allowed to queue commands, we'd be setting up new and exciting ways to have non-determinism.

This becomes particularly gnarly with complex exclusive systems: you end up having to use deeply nested resource_scope closures, swap to WorldCell or deal with SystemState.

I think this is reading too far ahead. The Discord threads on turn-based flows only had scopes two levels deep IIRC. If that makes compiler errors harder to debug, maybe we should just recommend users manually remove and re-insert their resources.

@alice-i-cecile
Copy link
Member Author

alice-i-cecile commented Mar 9, 2022

Introducing systems that do resource_scope under the hood (or params that take ownership and return it on completion) needs its own discussion

That was intended to be the heart of this discussion. I can see your point though, this does need its own RFC. I'm clearly not motivating this well enough :)

Removing one dichotomy just to add another doesn't feel like a win, especially considering you can't use anything but resources and locals in these hypothetical scoped systems.

That's the rest of the issue: you should be able to use queries and event readers / writers too. Being able to use event-readers in particular would be a massive win.

Removing one dichotomy just to add another doesn't feel like a win

I'm not sure I see a second dichotomy?

@maniwani
Copy link
Contributor

maniwani commented Mar 9, 2022

I'm not sure I see a second dichotomy?

I was responding to this:

As a result, it seems like there's a split between "scopable" and "non-scopable" system parameters.

I mostly took issue with systems running in parallel with systems that take &mut World. That's going to be a tough sell. Only a system that take locals and owned data could run in parallel with something that takes &mut World. And if you have to take data out of the World though, parallelism goes down in other places, since other systems wanting those things can't run.

I also don't think we want the headache of systems being able to smuggle commands past a flush (post-stageless).
(edit: As-is, this wouldn't be allowed anyway because Commands has a borrow on a world's Entities metadata.)

That was intended to be the heart of this discussion. I can see your point though, this does need its own RFC. I'm clearly not motivating this well enough :)

There's a few distinct ideas here:

  • &mut World being compatible with non-aliasing params (e.g. Local<T>, possibly Commands if reworked)
  • system params that take ownership from World
  • &World and &mut World being compatible with system params that take ownership
    • within the same system
    • among other systems

Two of these add significant complexity to the system access model while the other does not.

That's the rest of the issue: you should be able to use queries and event readers / writers too. Being able to use event-readers in particular would be a massive win.

To me right now, these seem like minor ergonomic wins compared to the amount of work required to make it error gracefully and not panic when users do something bad.

@maniwani
Copy link
Contributor

maniwani commented Mar 9, 2022

I should clarify that when I said "needs its own discussion" I just meant that I saw two different items being lumped together.

  • removing the type boundaries separating exclusive systems from the rest
  • adding another layer of access, params that take ownership to allow "aliasing" params to coexist in the same system

I didn't mean to suggest that one or the other needs an RFC, just that I think those are separate issues.

If the "run a system in parallel with another system taking &mut World" that appears as a side effect is something you want to highlight though, I do think that should be contrasted with other async strategies (bevy_tasks, "accessors", multiple worlds, component partitions in one world, etc.), since I don't think we should bless multiple APIs that do the same thing.

@alice-i-cecile
Copy link
Member Author

removing the type boundaries separating exclusive systems from the rest

Ah: that was taken as "implicitly and obviously correct" for this discussion: I wrote this issue for a post-unification design, intending to focus solely on the params that take ownership as a way to improve ergonomics of exclusive systems.

I agree about the need to compare-and-contrast. Fundamentally, I want to wait longer on this: I'm not sure I understand the legitimate use cases of exclusive systems well enough to make firm conclusions, particularly in a post-stageless design. The increased parallelism is very interesting, but I'm not sure it's actually effective, either from a complexity budget or performance perspective.

Regardless, it's better to have a place to record these thoughts, in case they become more relevant down the line.

@james7132
Copy link
Member

james7132 commented Mar 4, 2024

Exclusive systems now support using (&mut World, &mut SystemState<...>). It's not an ideal UX, but it does seem to fill the gap here. Anything supported by SystemParam, is supported under that flow. Can we close this issue? I don't think we are going to find a way to safely split the borrow as it currently stands.

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
Projects
None yet
Development

No branches or pull requests

3 participants