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

Fire events when scenes spawn #2425

Closed
wants to merge 13 commits into from
Closed

Conversation

szunami
Copy link
Contributor

@szunami szunami commented Jul 2, 2021

Objective

Solution

  • Introduce SceneSpawnedEvent as the new event type. Open to a different name / structure; are there related events that should be considered?
  • As part of spawn_queued_scenes, fire events when spawning scenes succeeds

Also, this is my first slightly larger code contribution, apologies if my style / approach is off :)

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jul 2, 2021
Ok(_) => {}
Err(SceneSpawnError::NonExistentScene { .. }) => {
self.dynamic_scenes_to_spawn.push(scene_handle)
world.resource_scope(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually spawning scenes requires a mut World, so I think this is necessary. Open to other approaches.

@szunami szunami marked this pull request as ready for review July 2, 2021 18:14
examples/scene/scene.rs Outdated Show resolved Hide resolved
crates/bevy_scene/src/scene_spawner.rs Show resolved Hide resolved
szunami and others added 2 commits July 2, 2021 16:53
Co-authored-by: Nathan Ward <43621845+NathanSWard@users.noreply.github.com>
@NathanSWard NathanSWard added A-Core Common functionality for all bevy apps C-Feature A new feature, making something new possible and removed S-Needs-Triage This issue needs to be labelled labels Jul 2, 2021
@szunami szunami requested a review from NathanSWard July 3, 2021 17:54
@alice-i-cecile
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Jul 3, 2021
@mockersf
Copy link
Member

mockersf commented Jul 4, 2021

You should also send events when scenes are spawned with the sync methods (spawn_sync and spawn_dynamic_sync).

@szunami
Copy link
Contributor Author

szunami commented Jul 4, 2021

@mockersf so you'd prefer if the event were written inside the sync methods? Is there a case where the sync methods get called directly?

I didn't pursue this initially because we need to get an event writer via resource_scope; we could certainly do this in the sync methods, but I'm not sure what the ramifications of that would be. Is this your preference?

Alternatively, the sync methods could take an EventWriter as input or something like that.

@alice-i-cecile
Copy link
Member

I didn't pursue this initially because we need to get an event writer via resource_scope; we could certainly do this in the sync methods, but I'm not sure what the ramifications of that would be. Is this your preference?

That's correct. There should be effectively no ramifications to that though: we already have &mut World (which contains our events) as a parameter on those methods, so we're not actually blocking any extra data.

In exchange, consistency and reliability are improved significantly. I definitely agree with Francois's proposal.

@mockersf
Copy link
Member

mockersf commented Jul 4, 2021

@mockersf so you'd prefer if the event were written inside the sync methods? Is there a case where the sync methods get called directly?

They are public, so we don't know if anyone is using them directly... They are the only way to spawn a scene without waiting for a frame, so it makes sense to keep them public.

@szunami
Copy link
Contributor Author

szunami commented Jul 4, 2021

@mockersf I had another pass at this.

It is a little awkward that we event one event in spawn_dynamic_sync and the other in spawn_sync_internal; this seemed like the most natural way to do things given the way the code is layed out, but seems a little likely for one to be missed in future updates.

@mockersf
Copy link
Member

mockersf commented Jul 4, 2021

Yup dynamic scenes are missing a few things that added complexity to non dynamic scenes... I changed some of that in #2424. Yours or mine will have to be updated whichever gets merged last (probably mine as yours is much simpler)

@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@mockersf mockersf removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 24, 2021
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 4, 2022
@alice-i-cecile
Copy link
Member

bors try

@bors
Copy link
Contributor

bors bot commented Feb 4, 2022

try

Merge conflict.

crates/bevy_scene/src/scene_spawner.rs Outdated Show resolved Hide resolved
crates/bevy_scene/src/scene_spawner.rs Outdated Show resolved Hide resolved
examples/scene/scene.rs Show resolved Hide resolved
szunami and others added 3 commits February 4, 2022 17:10
Co-authored-by: Charles <IceSentry@users.noreply.github.com>
Co-authored-by: Charles <IceSentry@users.noreply.github.com>
@cart
Copy link
Member

cart commented Feb 8, 2022

Just left some thoughts in #2218. I'd like to discuss UX + api design a bit more before moving forward on this.

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Apr 25, 2022
@cart
Copy link
Member

cart commented Mar 20, 2023

Closing this out for now as I think we need to think about the comments in #2218 before designing this API. We can resurrect if this ends up being the right base for it.

@cart cart closed this Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Core Common functionality for all bevy apps C-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants