Replace entity spawning abstraction #231

benfrankel · 2024-08-06T00:09:20Z

Fixes #223.
Part of #203.

Will need to update the design doc as well.

MiniaczQ

pattern looks good, i can look for nits tomorrow if this is still up

janhohenheim

I'm unhappy about the definition site of the construction functions, as I think this kind of function will be very surprising to a newcomer. The solution probably lies in finding a good naming convention, but I can't think of one.

src/screens/playing.rs

src/ui/widget.rs

janhohenheim · 2024-08-06T00:52:43Z

src/spawn.rs

+    fn spawn_with<M: 'static>(&mut self, command: impl EntityCommand<M>) -> EntityCommands;
+
+    /// Spawn an entity with a [`System`] that receives the new entity ID via [`In<Entity>`].
+    fn spawn_fn<M>(


The distinction between spawn_with and spawn_fn is currently probably impossible to grasp from the naming alone. I suggest renaming this to insert_with.

These both spawn a new entity, though. The difference is whether we're passing in data.

Oh wait, you're right. Can a system implement EntityCommand so that we can remove this function?

Due to the orphan rule, that would have to be done upstream. But it would be nice if so.

Fair enough. We still need a better name though.

Actually, in this case you can bypass the orphan rule by wrapping the system in a struct and then implementing traits on that struct. Not an option for the template because that is severe type magic, but just wanted to let you know.

janhohenheim · 2024-08-06T00:57:46Z

src/screens/playing.rs

+fn playing_screen(In(id): In<Entity>, mut commands: Commands) {
+    commands
+        .entity(id)
+        .add_fn(widget::ui_root)


It seems very confusing from a first glance when one should use add_fn and when spawn_fn. If this is confusing to me as a reviewer, I imagine it would be really really confusing to a newcomer.

An explanation in the design patterns doc should mitigate that a bit, but I'm open to bikeshedding.

src/screens/credits.rs

janhohenheim · 2024-08-06T02:01:38Z

@alice-i-cecile could you also take a look at this?

alice-i-cecile

I really don't like this, especially in learning material. There's a ton of complexity and type magic here, and it makes simple tasks appear esoteric and difficult. Boilerplate is better than complexity for this.

I understand the desire to improve things here, but I would be strongly opposed to endorsing code with these patterns as official learning materials.

janhohenheim · 2024-08-06T13:44:53Z

@alice-i-cecile I've though about this for half a day now and I'm tending the same way. I feel a bit bad since we both gave a thumbs-up for it in #223, but seeing it in practice is just too "scary". I've been thinking about ways to improve the design and make it more approachable, but I'm really struggling with that.

My feelings are a less negative than yours since I think the actual user-land code here is quite elegant while still staying flexible, but it is formulated in a foreign design language. As such, I also cannot recommend it for the template.

Things would be way easier to accept if

this was an upstreamed API, and so was able to be simplified due to no orphan rule, or
this was a library, and so a user is opting into it and ready to deal with the new concepts.

In either case, the implementation would be hidden, which would be a big plus.

For the moment, I'd instead go for the simpler route of replacing observers with custom commands.

benfrankel · 2024-08-06T16:32:08Z

Custom commands means having to extract system parameters from World, which is even worse in that regard, no? The only scary thing here is spawn.rs afaict. Observers use the same pattern of "first argument contains the Entity and extra data (the event), and then the rest are system parameters". The ergonomics of World are so bad that it's probably better to stick to observers.

janhohenheim · 2024-08-06T16:54:41Z

I don't like extracting stuff manually from World, but at least that is "standard Bevy", for better or worse.
Using an Entity as the first param is a good analogy to observers, agreed. I personally don't mind it, but I think the Bevy maintainers will disagree with introducing this as an abstraction.

My main issues are more:

Using the provided abstraction requires active learning, which I don't think is as bad for the other abstractions we use. You can take a single look at fn plugin or at commands.button and you know how to use it. The same cannot be said for things like the interplay / differences between spawn_with, spawn_fn, add_fn.
spawn.rs feels scary enough that for newcomers it might as well be a black box imo.

As for Commands vs Observers: I only recommend using commands because that was @alice-i-cecile's feedback. I personally would prefer an observer because of the ergonomics, even when that means a layer of indirection. My opinion on this is not strong however and I will accept either implementation.

benfrankel · 2024-08-06T16:57:33Z

For the record, custom commands still requires spawn_with and extension traits. It would only mean removing spawn_fn and add_fn and extracting everything from World instead of using system parameters.

janhohenheim · 2024-08-06T17:08:22Z

@benfrankel I think that could work, in terms of not introducing too many new abstractions / concepts.

In the worst case we can still use the crappy variant that does not return an Entity to the caller. It's suboptimal, but it's not our fault that this is fundamentally missing in Bevy and it might be fixed by BSN.

benfrankel · 2024-08-06T21:29:11Z

Done. The tradeoff is not worth losing usability in a real game IMO but it's w/e.

janhohenheim

I think this version is way clearer for an outsider. Other than some minor issues I noted, I'd merge this.
But I also want to hear @alice-i-cecile's opinion again, as this is controversial enough imo.

I want to stress that because the underlying spawning API is missing from Bevy, we will either have a somewhat complex, but decently usable pattern, or a very simple pattern that will break down when the user experiments with it.

I think this PR strikes a good balance now. I'd prefer to ship some way of spawning stuff while returning Entity or EntityCommands or similar as, in contrast to many other tradeoffs of the template, this is not something where a user can just switch to a popular community plugin when they are running into trouble.

src/spawn.rs

janhohenheim · 2024-08-06T21:37:15Z

src/demo/player.rs

-    // to animate our player character. You can learn more about texture atlases in
-    // this example: https://github.com/bevyengine/bevy/blob/latest/examples/2d/texture_atlas.rs
+/// Construct a player entity.
+pub fn player(id: Entity, world: &mut World) {


Could we rename these to construct_player and construct_level now? Seeing as the underlying pattern has shifted.
children.spawn_with(construct_player); sounds completely reasonable to be.

I still think that's more confusing (verbosity aside). The underlying pattern hasn't shifted at all, I just removed support for using a regular system instead of an exclusive system.

I think we are at an impasse with the naming here. Requesting a third opinion from anyone, I'll gladly accept the majority vote.

janhohenheim · 2024-08-06T21:37:25Z

src/demo/player.rs

-    // can specify which section of the image we want to see. We will use this
-    // to animate our player character. You can learn more about texture atlases in
-    // this example: https://github.com/bevyengine/bevy/blob/latest/examples/2d/texture_atlas.rs
+/// Construct a player entity.


Suggested change

/// Construct a player entity.

/// Construct a player entity by inserting the relevant components.

This could spawn children / do other things as well. Think health bar, nametag, multiple sprite children, colliders, etc.

I would like to guide users more on what "construct" means in this context and why there is a Entity as a parameter.

janhohenheim · 2024-08-06T21:42:41Z

src/demo/level.rs

+/// Construct a level entity.
+pub fn level(id: Entity, world: &mut World) {


Same as for fn player

janhohenheim · 2024-08-06T21:43:27Z

src/ui/widget.rs

+}
+
+/// Construct a UI node that fills the window and centers its children.
+pub fn ui_root(id: Entity, world: &mut World) {


same as fn player

Co-authored-by: Jan Hohenheim <[email protected]>

alice-i-cecile · 2024-08-07T20:31:01Z

src/spawn.rs

+
+/// An extension trait to support spawning an entity from an [`EntityCommand`].
+pub trait SpawnExt {
+    // Workaround for <https://github.com/bevyengine/bevy/issues/14231#issuecomment-2216321086>.


IMO this should just be fixed upstream.

alice-i-cecile · 2024-08-07T20:34:04Z

I think this is an improvement, but I'm still not happy with the level of abstraction / ad-hoc tools here. I think this should be merged, but the linked bug / limitation should be fixed upstream (ping me for reviews!), and I'm still reluctant to officially recommend this template until this is substantially simpler.

If you feel that the limitations of just doing things the dumb way are unacceptable, I'm fine to hold off on endorsing this until the BSN work is shipped.

janhohenheim · 2024-08-08T00:27:31Z

Closing this for now. Sorry again for the whole work that is not merged. Let's revisit this after some improvements have been implemented upstream.

benfrankel · 2024-08-08T00:38:36Z

All good, it was mostly copy/paste and some mechanical refactoring.

janhohenheim requested review from MiniaczQ and janhohenheim August 6, 2024 00:21

MiniaczQ approved these changes Aug 6, 2024

View reviewed changes

janhohenheim requested changes Aug 6, 2024

View reviewed changes

src/screens/credits.rs Outdated Show resolved Hide resolved

alice-i-cecile requested changes Aug 6, 2024

View reviewed changes

benfrankel force-pushed the spawn branch 2 times, most recently from 169aad0 to 7714a81 Compare August 6, 2024 21:11

benfrankel requested review from janhohenheim and alice-i-cecile August 6, 2024 21:29

janhohenheim requested changes Aug 6, 2024

View reviewed changes

benfrankel added 10 commits August 7, 2024 09:58

Replace entity spawning abstraction

ed0a7e5

Use children.label

cb3182d

Make text fields pub

cfd8fcf

Reword a few doc comments slightly

5cfe64e

Rename internal Self::spawn -> construct for consistency

ff8d8ed

Remove TODO from comment

9a833a3

Remove SpawnSystemExt

5eba238

Change some comments

9ee02cb

Yeet spawn_fn and add_fn I guess

bd6c8b0

Also yeet In<(Entity, Self)>

b744177

Update comments

5a1a210

benfrankel force-pushed the spawn branch from 57f951d to 5a1a210 Compare August 7, 2024 16:58

benfrankel and others added 3 commits August 7, 2024 12:52

Update src/spawn.rs

4123323

Co-authored-by: Jan Hohenheim <[email protected]>

Update src/spawn.rs

58d59e3

Co-authored-by: Jan Hohenheim <[email protected]>

Update src/spawn.rs

75ed50d

Co-authored-by: Jan Hohenheim <[email protected]>

alice-i-cecile reviewed Aug 7, 2024

View reviewed changes

janhohenheim mentioned this pull request Aug 7, 2024

Use custom commands instead of observers for spawning #242

Merged

janhohenheim closed this Aug 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace entity spawning abstraction #231

Replace entity spawning abstraction #231

benfrankel commented Aug 6, 2024 •

edited

Loading

MiniaczQ left a comment

janhohenheim left a comment •

edited

Loading

janhohenheim Aug 6, 2024

benfrankel Aug 6, 2024

janhohenheim Aug 6, 2024

benfrankel Aug 6, 2024

janhohenheim Aug 6, 2024

janhohenheim Aug 6, 2024

janhohenheim Aug 6, 2024

benfrankel Aug 6, 2024

janhohenheim commented Aug 6, 2024

alice-i-cecile left a comment

janhohenheim commented Aug 6, 2024 •

edited

Loading

benfrankel commented Aug 6, 2024 •

edited

Loading

janhohenheim commented Aug 6, 2024 •

edited

Loading

benfrankel commented Aug 6, 2024

janhohenheim commented Aug 6, 2024 •

edited

Loading

benfrankel commented Aug 6, 2024 •

edited

Loading

janhohenheim left a comment •

edited

Loading

janhohenheim Aug 6, 2024

benfrankel Aug 7, 2024 •

edited

Loading

janhohenheim Aug 7, 2024

janhohenheim Aug 6, 2024

benfrankel Aug 7, 2024 •

edited

Loading

janhohenheim Aug 7, 2024

janhohenheim Aug 6, 2024

janhohenheim Aug 6, 2024

alice-i-cecile Aug 7, 2024

alice-i-cecile commented Aug 7, 2024

janhohenheim commented Aug 8, 2024

benfrankel commented Aug 8, 2024

	/// Construct a player entity.
	/// Construct a player entity by inserting the relevant components.

		/// Construct a level entity.
		pub fn level(id: Entity, world: &mut World) {

Replace entity spawning abstraction #231

Replace entity spawning abstraction #231

Conversation

benfrankel commented Aug 6, 2024 • edited Loading

MiniaczQ left a comment

Choose a reason for hiding this comment

janhohenheim left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janhohenheim commented Aug 6, 2024

alice-i-cecile left a comment

Choose a reason for hiding this comment

janhohenheim commented Aug 6, 2024 • edited Loading

benfrankel commented Aug 6, 2024 • edited Loading

janhohenheim commented Aug 6, 2024 • edited Loading

benfrankel commented Aug 6, 2024

janhohenheim commented Aug 6, 2024 • edited Loading

benfrankel commented Aug 6, 2024 • edited Loading

janhohenheim left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benfrankel Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benfrankel Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alice-i-cecile commented Aug 7, 2024

janhohenheim commented Aug 8, 2024

benfrankel commented Aug 8, 2024

benfrankel commented Aug 6, 2024 •

edited

Loading

janhohenheim left a comment •

edited

Loading

janhohenheim commented Aug 6, 2024 •

edited

Loading

benfrankel commented Aug 6, 2024 •

edited

Loading

janhohenheim commented Aug 6, 2024 •

edited

Loading

janhohenheim commented Aug 6, 2024 •

edited

Loading

benfrankel commented Aug 6, 2024 •

edited

Loading

janhohenheim left a comment •

edited

Loading

benfrankel Aug 7, 2024 •

edited

Loading

benfrankel Aug 7, 2024 •

edited

Loading