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

order of commands.with(...) matters #1120

Closed
fopsdev opened this issue Dec 21, 2020 · 8 comments
Closed

order of commands.with(...) matters #1120

fopsdev opened this issue Dec 21, 2020 · 8 comments
Labels
A-ECS Entities, components, systems, and events C-Docs An addition or correction to our documentation S-User-Error This issue was caused by a mistake in the user's approach

Comments

@fopsdev
Copy link

fopsdev commented Dec 21, 2020

Bevy version
0.4
The release number or commit hash of the version you're using.
master 3b2c6ce
Operating system & version
Win 10

What you did
Adding a Component (Empty Struct) to Example sprite,rs

...
commands
        .with(Sprite)
        .spawn(Camera2dBundle::default())
        ...

What you expected to happen
Sprite Component should be added

What actually happened
Panic

Additional information
It works when adding the

.with(...)

at the end like:

    commands
        .spawn(Camera2dBundle::default())
        .spawn(SpriteBundle {
            material: materials.add(texture_handle.into()),

            ..Default::default()
        })
        .with(Sprite);
@fopsdev fopsdev changed the title order of commands .with(...) matters order of commands.with(...) matters Dec 21, 2020
@cart
Copy link
Member

cart commented Dec 21, 2020

Anything that uses with_x naming is a modifier on the "current" thing (in this case, an entity). This fails because there is no "current" entity. spawn is the command that creates a new entity / entity id, so we can't create the "with" command because the entity id hasn't been created yet. And semantically it has no meaning because the "X" in X.with() is undefined.

We could consider adopting a "typed builder" pattern to only allow the correct call pattern, but this has implications for ergonomics because it requires the typed builder to "terminate" (which would mean adding a .finish() function call).

I'll leave this open for a bit in case anyone wants to respond, but I consider this to be the "expected / desirable" behavior.

@smokku
Copy link
Member

smokku commented Dec 21, 2020

We could add a more meaningful panic message though, suggesting what to do.

@cart
Copy link
Member

cart commented Dec 21, 2020

Good call!

@smokku
Copy link
Member

smokku commented Dec 22, 2020

Current message is "Cannot add component because the 'current entity' is not set. You should spawn an entity first." which is fine I think.

Maybe "Cannot add component because the 'current entity' is not set. You should use .spawn() method to create an entity first."?

@memoryruins memoryruins added C-Docs An addition or correction to our documentation A-ECS Entities, components, systems, and events S-User-Error This issue was caused by a mistake in the user's approach labels Dec 22, 2020
@fopsdev
Copy link
Author

fopsdev commented Dec 22, 2020

Or maybe adding a hint to the "Warning" Section which already exists and is discoverable by rust analyzer:
image

@fopsdev
Copy link
Author

fopsdev commented Dec 22, 2020

though there are some more places where a method expects an existing entity...
not sure how to add this meta information to the method signatures without a lot of duplication

my vote: carts mentioned "typed builder" - pattern. i wouldn't mind adding a .finish() method to those candidates

On the other hand the panic msg is quite ok. together with the stack trace myself as a beginner was able to locate the issue.
so for sure not high prio :)

@smokku
Copy link
Member

smokku commented Dec 22, 2020

This panic is a small price to pay for not having to use .finish() every time you spawn an entity. And since this engine is ECS based, you deal with entity spawning a lot.

Like you mentioned, panic explains what you need to do, and in a moment it will be habit.

@fopsdev
Copy link
Author

fopsdev commented Dec 22, 2020

yes, i think we can close this. current behaviour is ok

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-Docs An addition or correction to our documentation S-User-Error This issue was caused by a mistake in the user's approach
Projects
None yet
Development

No branches or pull requests

4 participants