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

[Merged by Bors] - Adding WorldQuery for WithBundle #2024

Closed
wants to merge 15 commits into from

Conversation

CGMossa
Copy link
Contributor

@CGMossa CGMossa commented Apr 27, 2021

In response to #2023, here is a draft for a PR.

Fixes #2023

I've added an example to show how to use WithBundle, and also to test it out.

Right now there is a bug: If a bundle and a query are "the same", then it doesn't filter out
what it needs to filter out.

Example:

Print component initated from bundle.
[examples/ecs/query_bundle.rs:57] x = Dummy( <========= This should not get printed
    111,
)
[examples/ecs/query_bundle.rs:57] x = Dummy(
    222,
)
Show all components
[examples/ecs/query_bundle.rs:50] x = Dummy(
    111,
)
[examples/ecs/query_bundle.rs:50] x = Dummy(
    222,
)

However, it behaves the right way, if I add one more component to the bundle,
so the query and the bundle doesn't look the same:

Print component initated from bundle.
[examples/ecs/query_bundle.rs:57] x = Dummy(
    222,
)
Show all components
[examples/ecs/query_bundle.rs:50] x = Dummy(
    111,
)
[examples/ecs/query_bundle.rs:50] x = Dummy(
    222,
)

I hope this helps. I'm definitely up for tinkering with this, and adding anything that I'm asked to add
or change.

@MinerSebas
Copy link
Contributor

I wouldn't qualify that as a Bug, perhaps only as a surprise for Newbies.
My reason for this is that once an Entity is spawned the Fact that a Bundle was used is discarded.
This means that WithBundle just checks whether an Entity is composed out of Components from a Bundle, and not whether the entity was spawned using the provided Bundle.

@cart
Copy link
Member

cart commented Apr 27, 2021

We've been discussing whether or not we should implicitly add "bundle marker components" to entities that were spawned with a Bundle. Then you could just check if something has With<Bundle<MyBundle>>. There are good reasons to do this (allowing old scenes to adapt to components newly added / removed from Bundles), but also good reasons not to (adds a new concept / makes Bundles "special" / fractures archetypes).

Until then, I think WithBundle is a good middle ground. We could consider being a bit more explicit (ex: call it WithBundleComponents), but that is a pretty nasty name to look at / type.

examples/ecs/query_bundle.rs Outdated Show resolved Hide resolved
examples/ecs/query_bundle.rs Outdated Show resolved Hide resolved
examples/ecs/query_bundle.rs Outdated Show resolved Hide resolved
examples/ecs/query_bundle.rs Outdated Show resolved Hide resolved
Copy link
Member

@NiklasEi NiklasEi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it.
One point though: this negates current inline documentation of Bundle. Could you please update https://github.com/bevyengine/bevy/blob/main/crates/bevy_ecs/src/bundle.rs#L15?

Copy link
Contributor

@MinerSebas MinerSebas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new example also needs to be added to the README.md inside the examples Folder.

examples/ecs/query_bundle.rs Outdated Show resolved Hide resolved
examples/ecs/query_bundle.rs Outdated Show resolved Hide resolved
examples/ecs/query_bundle.rs Outdated Show resolved Hide resolved
@Moxinilian Moxinilian added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible labels Apr 27, 2021
CGMossa and others added 7 commits April 28, 2021 09:52
Co-authored-by: MinerSebas <66798382+MinerSebas@users.noreply.github.com>
Co-authored-by: MinerSebas <66798382+MinerSebas@users.noreply.github.com>
Co-authored-by: MinerSebas <66798382+MinerSebas@users.noreply.github.com>
This change disappeared and now I bring it back again.
@CGMossa
Copy link
Contributor Author

CGMossa commented Apr 28, 2021

If I don't add a single_threaded stage, I get printing (due to parallelism) that looks like this:

Apr 28 10:01:19.628  INFO query_bundle: Show all entites with component `Name`
Apr 28 10:01:19.628  INFO query_bundle: Print `Name` component residing in entitt are added via `PersonBundle`.
Apr 28 10:01:19.630  INFO query_bundle: Name("Bob")
Apr 28 10:01:19.629  INFO query_bundle: Name("Steve")
Apr 28 10:01:19.630  INFO query_bundle: Name("Bob")

which is not great. I still haven't figured out how to make an App single_threaded or a SystemSet to be single threaded.. Or change an already included stage CoreStage::Update to be single threaded.

@cart
Copy link
Member

cart commented Apr 28, 2021

I still haven't figured out how to make an App single_threaded or a SystemSet to be single threaded.. Or change an already included stage CoreStage::Update to be single threaded.

Our default answer to system order requirements is currently System Labels. Its slightly more boilerplate than a single threaded stage, but it has the benefit of:

  1. encoding dependencies between systems instead of them being there implicitly from registration order
  2. maximizing parallelism (systems can all run in the same stage, in parallel by default, but respecting order when its there)

@cart
Copy link
Member

cart commented Apr 28, 2021

Lets use labels with a new type deriving SystemLabel (see the link above for an example).

@CGMossa
Copy link
Contributor Author

CGMossa commented Apr 28, 2021

Thanks for clarifying. I really needed this last two comments :P But here it is now.

@cart
Copy link
Member

cart commented Apr 28, 2021

Made a few small tweaks to make the example even more minimal.

@cart cart marked this pull request as ready for review April 28, 2021 19:34
@cart
Copy link
Member

cart commented Apr 28, 2021

I also adopted the for x in query.iter() approach, as we recommend doing that by default.

@CGMossa
Copy link
Contributor Author

CGMossa commented Apr 28, 2021

Again, thanks so much. I've read all of your changes and will attempt to adhere to these standards next time.

@cart
Copy link
Member

cart commented Apr 28, 2021

bors r+

bors bot pushed a commit that referenced this pull request Apr 28, 2021
In response to #2023, here is a draft for a PR. 

Fixes #2023

I've added an example to show how to use `WithBundle`, and also to test it out. 

Right now there is a bug: If a bundle and a query are "the same", then it doesn't filter out
what it needs to filter out. 

Example: 

```
Print component initated from bundle.
[examples/ecs/query_bundle.rs:57] x = Dummy( <========= This should not get printed
    111,
)
[examples/ecs/query_bundle.rs:57] x = Dummy(
    222,
)
Show all components
[examples/ecs/query_bundle.rs:50] x = Dummy(
    111,
)
[examples/ecs/query_bundle.rs:50] x = Dummy(
    222,
)
```

However, it behaves the right way, if I add one more component to the bundle,
so the query and the bundle doesn't look the same:

```
Print component initated from bundle.
[examples/ecs/query_bundle.rs:57] x = Dummy(
    222,
)
Show all components
[examples/ecs/query_bundle.rs:50] x = Dummy(
    111,
)
[examples/ecs/query_bundle.rs:50] x = Dummy(
    222,
)
```

I hope this helps. I'm definitely up for tinkering with this, and adding anything that I'm asked to add
or change. 





Co-authored-by: Carter Anderson <mcanders1@gmail.com>
@bors bors bot changed the title Adding WorldQuery for WithBundle [Merged by Bors] - Adding WorldQuery for WithBundle Apr 28, 2021
@bors bors bot closed this Apr 28, 2021
NiklasEi pushed a commit to NiklasEi/bevy that referenced this pull request May 1, 2021
In response to bevyengine#2023, here is a draft for a PR. 

Fixes bevyengine#2023

I've added an example to show how to use `WithBundle`, and also to test it out. 

Right now there is a bug: If a bundle and a query are "the same", then it doesn't filter out
what it needs to filter out. 

Example: 

```
Print component initated from bundle.
[examples/ecs/query_bundle.rs:57] x = Dummy( <========= This should not get printed
    111,
)
[examples/ecs/query_bundle.rs:57] x = Dummy(
    222,
)
Show all components
[examples/ecs/query_bundle.rs:50] x = Dummy(
    111,
)
[examples/ecs/query_bundle.rs:50] x = Dummy(
    222,
)
```

However, it behaves the right way, if I add one more component to the bundle,
so the query and the bundle doesn't look the same:

```
Print component initated from bundle.
[examples/ecs/query_bundle.rs:57] x = Dummy(
    222,
)
Show all components
[examples/ecs/query_bundle.rs:50] x = Dummy(
    111,
)
[examples/ecs/query_bundle.rs:50] x = Dummy(
    222,
)
```

I hope this helps. I'm definitely up for tinkering with this, and adding anything that I'm asked to add
or change. 





Co-authored-by: Carter Anderson <mcanders1@gmail.com>
ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
In response to bevyengine#2023, here is a draft for a PR. 

Fixes bevyengine#2023

I've added an example to show how to use `WithBundle`, and also to test it out. 

Right now there is a bug: If a bundle and a query are "the same", then it doesn't filter out
what it needs to filter out. 

Example: 

```
Print component initated from bundle.
[examples/ecs/query_bundle.rs:57] x = Dummy( <========= This should not get printed
    111,
)
[examples/ecs/query_bundle.rs:57] x = Dummy(
    222,
)
Show all components
[examples/ecs/query_bundle.rs:50] x = Dummy(
    111,
)
[examples/ecs/query_bundle.rs:50] x = Dummy(
    222,
)
```

However, it behaves the right way, if I add one more component to the bundle,
so the query and the bundle doesn't look the same:

```
Print component initated from bundle.
[examples/ecs/query_bundle.rs:57] x = Dummy(
    222,
)
Show all components
[examples/ecs/query_bundle.rs:50] x = Dummy(
    111,
)
[examples/ecs/query_bundle.rs:50] x = Dummy(
    222,
)
```

I hope this helps. I'm definitely up for tinkering with this, and adding anything that I'm asked to add
or change. 





Co-authored-by: Carter Anderson <mcanders1@gmail.com>
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

Successfully merging this pull request may close these issues.

WithBundle missing WorldQuery implementation
5 participants