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.

Sign up for GitHub

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

Jump to bottom

Remove add_sub_app api #7286

Closed
hymm opened this issue Jan 20, 2023 · 0 comments
Closed

Remove add_sub_app api #7286

hymm opened this issue Jan 20, 2023 · 0 comments
Labels
A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Trivial Nice and easy! A great choice to get started with Bevy

Comments

@hymm
Copy link
Contributor

hymm commented Jan 20, 2023

Consider removing the add_sub_app api. This method is somewhat redundant with the insert_sub_app api. Consider:

let sub_app = App::empty();
app.add_sub_app(SubAppLabel, sub_app, extract_function);

vs

let sub_app = App::empty();
app.insert_sub_app(
  SubAppLabel,
  SubApp {
    app: sub_app,
    extract: Box::new(extract_function),
  }
);

This is a somewhat niche feature and may not deserve an extra api for slight better ergonomics.

See this comment from cart from the pipelined rendering pr

Maybe we should just remove this in favor of insert_sub_app. The insert semantics are "correct" anyway whereas add is generally used for repeatable / non-overwriting actions. This function is slightly more ergonomic, but SubApps aren't exactly a common pattern anyway.
@hymm hymm added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Jan 20, 2023
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Trivial Nice and easy! A great choice to get started with Bevy and removed C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Jan 20, 2023
@james7132 james7132 mentioned this issue Jan 20, 2023
Closed
@bors bors bot closed this as completed in 958a898 Jan 24, 2023
JMS55 pushed a commit to JMS55/bevy that referenced this issue Jan 25, 2023
@james7132 @JMS55
Remove App::add_sub_app (bevyengine#7290)
36f2e88 
# Objective
Fixes bevyengine#7286. Both `App::add_sub_app` and `App::insert_sub_app` are rather redundant. Before 0.10 is shipped, one of them should be removed.

## Solution
Remove `App::add_sub_app` to prefer `App::insert_sub_app`.

Also hid away `SubApp::extract` since that can be a footgun if someone mutates it for whatever reason. Willing to revert this change if there are objections.

Perhaps we should make `SubApp: Deref<Target=App>`? Might change if we decide to move `!Send` resources into it.

---

## Changelog
Added: `SubApp::new`
Removed: `App::add_sub_app`

## Migration Guide
`App::add_sub_app` has been removed in favor of `App::insert_sub_app`. Use `SubApp::new` and insert it via `App::add_sub_app`

Old:

```rust
let mut sub_app = App::new()
// Build subapp here
app.add_sub_app(MySubAppLabel, sub_app);
```

New:

```rust
let mut sub_app = App::new()
// Build subapp here
app.insert_sub_app(MySubAppLabel, SubApp::new(sub_app, extract_fn));
```
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
@james7132 @ItsDoot
Remove App::add_sub_app (bevyengine#7290)
be3122b 
# Objective
Fixes bevyengine#7286. Both `App::add_sub_app` and `App::insert_sub_app` are rather redundant. Before 0.10 is shipped, one of them should be removed.

## Solution
Remove `App::add_sub_app` to prefer `App::insert_sub_app`.

Also hid away `SubApp::extract` since that can be a footgun if someone mutates it for whatever reason. Willing to revert this change if there are objections.

Perhaps we should make `SubApp: Deref<Target=App>`? Might change if we decide to move `!Send` resources into it.

---

## Changelog
Added: `SubApp::new`
Removed: `App::add_sub_app`

## Migration Guide
`App::add_sub_app` has been removed in favor of `App::insert_sub_app`. Use `SubApp::new` and insert it via `App::add_sub_app`

Old:

```rust
let mut sub_app = App::new()
// Build subapp here
app.add_sub_app(MySubAppLabel, sub_app);
```

New:

```rust
let mut sub_app = App::new()
// Build subapp here
app.insert_sub_app(MySubAppLabel, SubApp::new(sub_app, extract_fn));
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Trivial Nice and easy! A great choice to get started with Bevy
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Merged by Bors] - Remove App::add_sub_app james7132/bevy
2 participants
@hymm @alice-i-cecile