Remove WorldCell #3939

alice-i-cecile · 2022-02-13T23:08:27Z

Objective

WorldCell is not fully featured: see Expand WorldCell to support queries #1555.
As a result, it does not fully behave like a Cell (a runtime borrow-checked API to split borrows arbitrarily).
Making it fully-featured is a questionable idea: see discussion in World cell queries rfcs#42 on how this promotes very panic-prone, fragile and hard-to-reason about code that is easy to write.

Solution

Remove WorldCell completely, rather than keeping a half-working implementation around.
Users should use SystemState or resource_scope as needed.
As other clear use cases arise that cannot be comfortably tackled with the existing API, add additional, less error-prone methods on World.

Notes

In the places where this was used:

several places were exclusive systems that did not need to be exclusive
a couple usages were trivially replaced with a resource_scope
other usages were trivially replaced with a SystemState
some were apparently completely unnecessary (assorted pipeline FromWorld impls)
one (winit_runner_with) was straightforward but rather verbose, and reflective of generally not-great code quality (it should be reusable but isn't, and is rather unstructured)

alice-i-cecile · 2022-02-13T23:11:22Z

@bevyengine/ecs-team; this is fundamentally a design decision. RFC #42 seems fairly doomed (a decision I support), but if it's doomed we should close it and remove the half-working WorldCell API.

~~As a point in favor of this removal, this API was never used internally.~~

cart · 2022-02-13T23:15:37Z

As a point in favor of this removal, this API was never used internally.

This isn't true :)

alice-i-cecile · 2022-02-13T23:19:02Z

Ah, my search for WorldCell missed it. Let me try fixing those...

cart · 2022-02-13T23:22:59Z

Pretty sure they can all be replaced with SystemState or resource_scope.
What WorldCell really enables is "arbitrary nesting of non-conflicting contexts with arbitrary ad-hoc borrows". Bevy doesn't use that pattern internally except for the RenderGraph resource_scope, but I can see why some users might want it. We might want it if we want to support Entitas-style event handlers.

alice-i-cecile · 2022-02-13T23:27:19Z

What WorldCell really enables is "arbitrary nesting of non-conflicting contexts with arbitrary ad-hoc borrows"

I agree.

This is a risky pattern, and non-trivial uses of this are likely to lead to complex runtime panics whenever surrounding code is changed. IMO the ideas discussed in #2192 are a much better way to accomplish the same goals.

Remove WorldCell uses

alice-i-cecile · 2022-02-14T00:32:18Z

It's a bit frustrating not to have a better solution for #2192 yet; the extremely complex system state constructed in winit_runner_with would particularly benefit from the caching and complexity management. Looking at the code, I actually think it may be cleanest to just create / store / run a little schedule on the App to push the information from winit into the various event resources. This would also make winit_runner much more reusable / hackable.

Overall, that refactor was quite painless, even for extremely complex uses of WorldCell. And now the system state can be cached and the borrow checker rules are upheld at compile time.

TheRawMeatball · 2022-02-14T15:52:42Z

We might want it if we want to support Entitas-style event handlers.

I personally think requiring &mut World and adding hooks would be a better way to support this.

TheRawMeatball

I agree with the idea, but the winit runner can be done better imo.

crates/bevy_winit/src/lib.rs

# Objective - In the large majority of cases, users were calling `.unwrap()` immediately after `.get_resource`. - Attempting to add more helpful error messages here resulted in endless manual boilerplate (see #3899 and the linked PRs). ## Solution - Add an infallible variant named `.resource` and so on. - Use these infallible variants over `.get_resource().unwrap()` across the code base. ## Notes I did not provide equivalent methods on `WorldCell`, in favor of removing it entirely in #3939. ## Migration Guide Infallible variants of `.get_resource` have been added that implicitly panic, rather than needing to be unwrapped. Replace `world.get_resource::<Foo>().unwrap()` with `world.resource::<Foo>()`. ## Impact - `.unwrap` search results before: 1084 - `.unwrap` search results after: 942 - internal `unwrap_or_else` calls added: 4 - trivial unwrap calls removed from tests and code: 146 - uses of the new `try_get_resource` API: 11 - percentage of the time the unwrapping API was used internally: 93%

aevyrie · 2022-03-04T19:17:26Z

Please ping me when this is ready to merge, assuming it happens after #3974. I want to double check that everything still behaves as expected. 🙂

crates/bevy_pbr/src/render/light.rs

crates/bevy_render/src/lib.rs

Co-authored-by: Aevyrie <[email protected]>

alice-i-cecile · 2022-03-05T13:30:16Z

Please ping me when this is ready to merge

Will do! From my perspective, this is ready-to-merge; it's just waiting on a final decision from Cart.

aevyrie · 2022-03-05T18:58:44Z

Got it! We'll just have to be mindful if/when this and #3974 are merged that we don't accidentally break anything in the merging process. We don't have an effective way to automatically test the window updating just yet.

- In the large majority of cases, users were calling `.unwrap()` immediately after `.get_resource`. - Attempting to add more helpful error messages here resulted in endless manual boilerplate (see bevyengine#3899 and the linked PRs). - Add an infallible variant named `.resource` and so on. - Use these infallible variants over `.get_resource().unwrap()` across the code base. I did not provide equivalent methods on `WorldCell`, in favor of removing it entirely in bevyengine#3939. Infallible variants of `.get_resource` have been added that implicitly panic, rather than needing to be unwrapped. Replace `world.get_resource::<Foo>().unwrap()` with `world.resource::<Foo>()`. - `.unwrap` search results before: 1084 - `.unwrap` search results after: 942 - internal `unwrap_or_else` calls added: 4 - trivial unwrap calls removed from tests and code: 146 - uses of the new `try_get_resource` API: 11 - percentage of the time the unwrapping API was used internally: 93%

alice-i-cecile · 2022-06-26T23:22:17Z

Closing in favor of the dramatically less controversial #5109. I still think this should be done, but the code-quality changes should be ported over ASAP anyways.

james7132 · 2024-03-17T22:56:05Z

Our internal uses of World::cell outside of tests is now at 0. It might be a good time to revisit this now that we have UnsafeWorldCell and DeferredWorld.

Remove WorldCell

68c1b83

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Feb 13, 2022

alice-i-cecile marked this pull request as draft February 13, 2022 23:20

alice-i-cecile added 3 commits February 13, 2022 18:35

Convert pointless exclusive systems

eec5e13

Replace simple WorldCell uses

c3e3d58

Remove WorldCell uses

Remove WorldCell from pipelines

f0432f1

alice-i-cecile marked this pull request as ready for review February 14, 2022 00:35

TheRawMeatball self-requested a review February 14, 2022 15:53

TheRawMeatball reviewed Feb 14, 2022

View reviewed changes

crates/bevy_winit/src/lib.rs Show resolved Hide resolved

alice-i-cecile added 2 commits February 14, 2022 13:40

Move change_window to bottom of file where it belongs

f563959

Simplify massive type using #derive(SystemParam)

7a23b63

TheRawMeatball approved these changes Feb 14, 2022

View reviewed changes

alice-i-cecile mentioned this pull request Feb 14, 2022

Basic WorldCell docs #3389

Closed

alice-i-cecile mentioned this pull request Feb 27, 2022

[Merged by Bors] - Make get_resource (and friends) infallible #4047

Closed

alice-i-cecile mentioned this pull request Mar 4, 2022

[Merged by Bors] - Add infallible resource getters for WorldCell #4104

Closed

aevyrie suggested changes Mar 4, 2022

View reviewed changes

crates/bevy_pbr/src/render/light.rs Outdated Show resolved Hide resolved

crates/bevy_render/src/lib.rs Outdated Show resolved Hide resolved

Use new infallible API

476465b

Co-authored-by: Aevyrie <[email protected]>

This was referenced Mar 8, 2022

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

Closed

Expand WorldCell to support queries #1555

Closed

alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Apr 22, 2022

alice-i-cecile mentioned this pull request Jun 7, 2022

Windows as Entities #4947

Closed

9 tasks

alice-i-cecile mentioned this pull request Jun 26, 2022

Remove unnecessary usages of WorldCell API #5109

Closed

alice-i-cecile closed this Jun 26, 2022

alice-i-cecile deleted the worldcell-yeet branch June 26, 2022 23:26

alice-i-cecile mentioned this pull request Mar 18, 2024

Reconsider removing WorldCell #12549

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove WorldCell #3939

Remove WorldCell #3939

alice-i-cecile commented Feb 13, 2022 •

edited

Loading

alice-i-cecile commented Feb 13, 2022 •

edited

Loading

cart commented Feb 13, 2022

alice-i-cecile commented Feb 13, 2022 •

edited

Loading

cart commented Feb 13, 2022 •

edited

Loading

alice-i-cecile commented Feb 13, 2022 •

edited

Loading

alice-i-cecile commented Feb 14, 2022 •

edited

Loading

TheRawMeatball commented Feb 14, 2022

TheRawMeatball left a comment

aevyrie commented Mar 4, 2022

alice-i-cecile commented Mar 5, 2022

aevyrie commented Mar 5, 2022

alice-i-cecile commented Jun 26, 2022

james7132 commented Mar 17, 2024

Remove WorldCell #3939

Remove WorldCell #3939

Conversation

alice-i-cecile commented Feb 13, 2022 • edited Loading

Objective

Solution

Notes

alice-i-cecile commented Feb 13, 2022 • edited Loading

cart commented Feb 13, 2022

alice-i-cecile commented Feb 13, 2022 • edited Loading

cart commented Feb 13, 2022 • edited Loading

alice-i-cecile commented Feb 13, 2022 • edited Loading

alice-i-cecile commented Feb 14, 2022 • edited Loading

TheRawMeatball commented Feb 14, 2022

TheRawMeatball left a comment

Choose a reason for hiding this comment

aevyrie commented Mar 4, 2022

alice-i-cecile commented Mar 5, 2022

aevyrie commented Mar 5, 2022

alice-i-cecile commented Jun 26, 2022

james7132 commented Mar 17, 2024

alice-i-cecile commented Feb 13, 2022 •

edited

Loading

alice-i-cecile commented Feb 13, 2022 •

edited

Loading

alice-i-cecile commented Feb 13, 2022 •

edited

Loading

cart commented Feb 13, 2022 •

edited

Loading

alice-i-cecile commented Feb 13, 2022 •

edited

Loading

alice-i-cecile commented Feb 14, 2022 •

edited

Loading