Move scene spawner systems to SpawnScene schedule #9260

lewiszlw · 2023-07-24T03:11:03Z

Objective

Fixes Move scene_spawner_system to PostUpdate. #9250

Changelog

Move scene spawner systems to a new SpawnScene schedule which is after Update and before PostUpdate (schedule order: [PreUpdate][Update][SpawnScene][PostUpdate])

Migration Guide

Move scene spawner systems to a new SpawnScene schedule which is after Update and before PostUpdate (schedule order: [PreUpdate][Update][SpawnScene][PostUpdate]), you might remove system ordering code related to scene spawning as the execution order has been guaranteed by bevy engine.

Shatur · 2023-07-24T07:16:59Z

Please, fill the migration guide and changelog properly.

lewiszlw · 2023-07-24T10:54:25Z

Please, fill the migration guide and changelog properly.

Done.

Shatur · 2023-07-24T10:58:06Z

Migration guide describes what users need to do in order to migrate. So you need to ask users to remove .after(scene::scene_spawner_system) if their systems was in Update and add .after(scene::scene_spawner_system) if they have systems that runs on PreUpdate and they want it to run before scene spawn.

crates/bevy_scene/src/lib.rs

Co-authored-by: Hennadii Chernyshchyk <genaloner@gmail.com>

Shatur · 2023-07-30T19:00:39Z

crates/bevy_scene/src/lib.rs

-            .add_systems(Update, scene_spawner_system)
-            // Systems `*_bundle_spawner` must run before `scene_spawner_system`
-            .add_systems(PreUpdate, scene_spawner);
+            .add_systems(PostUpdate, (scene_spawner, scene_spawner_system).chain());


For reviewers: If we run it in PostUpdate, should run it after propagate_transforms?..

IMO it should run before propagate_transforms and require a command application between them as it would otherwise be improperly placed in the following tick.

Oh, yes, I wanted to write "before".
But why do we need a command flush? Scenes spawned in exclusive system with all necessary parenting.

@james7132 alternatively we could move it to PreUpdate as I suggested in the issue first. Currently we have scene_spawner_system in Update and I think it's not a good choice.

Ah sorry, wasn't too familiar with the scene systems. Sounds good to me to have it just run before propagate_transforms in that case.

james7132

Just looking to ensure that the system runs before the propagation systems, otherwise LGTM.

crates/bevy_scene/src/lib.rs

Co-authored-by: Hennadii Chernyshchyk <genaloner@gmail.com>

nicopap · 2023-08-07T08:32:18Z

crates/bevy_scene/src/lib.rs

-            .add_systems(Update, scene_spawner_system)
-            // Systems `*_bundle_spawner` must run before `scene_spawner_system`
-            .add_systems(PreUpdate, scene_spawner);
+            .add_systems(PreUpdate, (scene_spawner, scene_spawner_system).chain());


Suggested change

.add_systems(PreUpdate, (scene_spawner, scene_spawner_system).chain());

.add_systems(PostUpdate, (scene_spawner, scene_spawner_system).chain().before(propagate_transform));

We discussed it in Discord and decided that it's better to put it in PostUpdate to have fully updated transform.
@lewiszlw I really sorry you had to change it so many times 😄
But this one you can't apply from the browser, you need to import propagate_transform.

No worries. Should be .before(TransformSystem::TransformPropagate)?

Yes, looks like this is what mostly used in the codebase.

Before you make any change, take a look at what cart suggesting, I think it makes sense: #9260 (comment)
Because both solution (PreUpdate and PostUpdate) have downsides, this is why we have such hard time deciding it.

Shatur

Judging by the messages in the discord, option 3 from #9260 (comment) is the most reasonable choice. Please add a new schedule and put these systems here.

cart · 2023-08-11T20:59:13Z

Agreed!

Scene spawning will happen after `Update`, so two updates will be needed anyway, see bevyengine/bevy#9260

# Objective - Fixes bevyengine#9250 ## Changelog - Move scene spawner systems to a new SpawnScene schedule which is after Update and before PostUpdate (schedule order: [PreUpdate][Update][SpawnScene][PostUpdate]) ## Migration Guide - Move scene spawner systems to a new SpawnScene schedule which is after Update and before PostUpdate (schedule order: [PreUpdate][Update][SpawnScene][PostUpdate]), you might remove system ordering code related to scene spawning as the execution order has been guaranteed by bevy engine. --------- Co-authored-by: Hennadii Chernyshchyk <genaloner@gmail.com>

# Objective #9260 added this schedule, but it's not in prelude like other schedules. ## Solution Add to prelude.

Pust scene_spawner_system in PreUpdate

be4d3a0

lewiszlw changed the title ~~Pust scene_spawner_system in PreUpdate~~ Move scene_spawner_system to PreUpdate Jul 24, 2023

Shatur approved these changes Jul 26, 2023

View reviewed changes

Shatur reviewed Jul 26, 2023

View reviewed changes

crates/bevy_scene/src/lib.rs Outdated Show resolved Hide resolved

Use .chain()

bf91563

Shatur approved these changes Jul 27, 2023

View reviewed changes

Shatur mentioned this pull request Jul 28, 2023

Introduce BlueprintSet projectharmonia/bevy_replicon#28

Merged

mockersf approved these changes Jul 29, 2023

View reviewed changes

mockersf 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 Jul 29, 2023

Shatur suggested changes Jul 30, 2023

View reviewed changes

crates/bevy_scene/src/lib.rs Outdated Show resolved Hide resolved

crates/bevy_scene/src/lib.rs Outdated Show resolved Hide resolved

lewiszlw and others added 2 commits July 30, 2023 18:01

Update crates/bevy_scene/src/lib.rs

efb80f6

Co-authored-by: Hennadii Chernyshchyk <genaloner@gmail.com>

Update crates/bevy_scene/src/lib.rs

d6fc397

Co-authored-by: Hennadii Chernyshchyk <genaloner@gmail.com>

lewiszlw changed the title ~~Move scene_spawner_system to PreUpdate~~ Move scene_spawner_system to PostUpdate Jul 30, 2023

Shatur reviewed Jul 30, 2023

View reviewed changes

mockersf removed 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 Jul 30, 2023

mockersf self-requested a review July 30, 2023 21:22

Shatur mentioned this pull request Jul 31, 2023

One-frame lag when spawning scenes #8616

Closed

james7132 requested changes Aug 3, 2023

View reviewed changes

james7132 added C-Bug An unexpected or incorrect behavior A-Scenes Serialized ECS data stored on the disk M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Aug 3, 2023

james7132 added this to the 0.12 milestone Aug 3, 2023

Shatur suggested changes Aug 3, 2023

View reviewed changes

crates/bevy_scene/src/lib.rs Outdated Show resolved Hide resolved

Update crates/bevy_scene/src/lib.rs

303872b

Co-authored-by: Hennadii Chernyshchyk <genaloner@gmail.com>

lewiszlw changed the title ~~Move scene_spawner_system to PostUpdate~~ Move scene_spawner_system to PreUpdate Aug 7, 2023

lewiszlw requested review from james7132 and Shatur August 7, 2023 08:23

nicopap reviewed Aug 7, 2023

View reviewed changes

Shatur suggested changes Aug 10, 2023

View reviewed changes

Add SpawnScene schedule

8ac4e52

lewiszlw changed the title ~~Move scene_spawner_system to PreUpdate~~ Move scene spawner systems to SpawnScene schedule Aug 14, 2023

lewiszlw requested a review from Shatur August 14, 2023 02:37

Shatur approved these changes Aug 14, 2023

View reviewed changes

Shatur requested review from nicopap and cart August 14, 2023 07:16

nicopap approved these changes Aug 14, 2023

View reviewed changes

cart approved these changes Aug 14, 2023

View reviewed changes

cart added this pull request to the merge queue Aug 14, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 14, 2023

Shatur added a commit to projectharmonia/bevy_replicon that referenced this pull request Aug 15, 2023

Update scene_update_sync test

49121b3

Scene spawning will happen after `Update`, so two updates will be needed anyway, see bevyengine/bevy#9260

alice-i-cecile added this pull request to the merge queue Aug 15, 2023

Merged via the queue into bevyengine:main with commit 55a7109 Aug 15, 2023

Shatur mentioned this pull request Aug 15, 2023

Add SpawnScene to prelude #9451

Merged

github-merge-queue bot pushed a commit that referenced this pull request Aug 19, 2023

Add SpawnScene to prelude (#9451)

756b044

# Objective #9260 added this schedule, but it's not in prelude like other schedules. ## Solution Add to prelude.

hymm mentioned this pull request Aug 30, 2023

Stack overflow with ambiguity detection and .run_if(in_state(...)) #9641

Closed

This was referenced Oct 26, 2023

Set the GlobalTransform of the children when spawning a scene #10265

Closed

Hooks are applied one frame late nicopap/bevy-scene-hook#8

Closed

nfagerlund mentioned this pull request Nov 25, 2023

Inspector systems are ambiguous with the entire Update schedule jakobhellermann/bevy-inspector-egui#169

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move scene spawner systems to SpawnScene schedule #9260

Move scene spawner systems to SpawnScene schedule #9260

lewiszlw commented Jul 24, 2023 •

edited

Loading

Shatur commented Jul 24, 2023

lewiszlw commented Jul 24, 2023

Shatur commented Jul 24, 2023 •

edited

Loading

Shatur Jul 30, 2023

james7132 Jul 30, 2023

Shatur Jul 30, 2023 •

edited

Loading

Shatur Jul 31, 2023 •

edited

Loading

james7132 Aug 3, 2023

james7132 left a comment

nicopap Aug 7, 2023

Shatur Aug 7, 2023

lewiszlw Aug 7, 2023

Shatur Aug 7, 2023

Shatur Aug 7, 2023

Shatur left a comment

cart commented Aug 11, 2023

	.add_systems(PreUpdate, (scene_spawner, scene_spawner_system).chain());
	.add_systems(PostUpdate, (scene_spawner, scene_spawner_system).chain().before(propagate_transform));

Move scene spawner systems to SpawnScene schedule #9260

Move scene spawner systems to SpawnScene schedule #9260

Conversation

lewiszlw commented Jul 24, 2023 • edited Loading

Objective

Changelog

Migration Guide

Shatur commented Jul 24, 2023

lewiszlw commented Jul 24, 2023

Shatur commented Jul 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shatur Jul 30, 2023 • edited Loading

Choose a reason for hiding this comment

Shatur Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james7132 left a 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

Shatur left a comment

Choose a reason for hiding this comment

cart commented Aug 11, 2023

lewiszlw commented Jul 24, 2023 •

edited

Loading

Shatur commented Jul 24, 2023 •

edited

Loading

Shatur Jul 30, 2023 •

edited

Loading

Shatur Jul 31, 2023 •

edited

Loading