-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Add back hot reloading for Scene
s
#4552
Conversation
* Merge code in `SceneSpawner` for managing `DynamicScene` and `Scene`. * Add back hot reloading for `Scene`s, fixes bevyengine#3759 * Add ability to despawn `Scene`s * Add ability to spawn `DynamicScene` as child of existing entities * Add documentation to `SceneSpawner` methods. * Add `write_to_world` method to `Scene` (does the same as `DynamicScene::write_to_world`) This fixes bevyengine#3759 see this commit's merging PR for further details about this change. Merge PR: bevyengine#4552
937588e
to
a4eea6c
Compare
I'm going to remove the generic parameter. I don't like it and I think it's a major ergonomic loss. I'll not rebase so that it's possible to revert the removal if deemed appropriate. |
pub fn write_to_world( | ||
&self, | ||
world: &mut World, | ||
entity_map: &mut EntityMap, | ||
) -> Result<(), SceneSpawnError> { | ||
let type_registry = world.resource::<TypeRegistryArc>().clone(); | ||
let type_registry = type_registry.read(); | ||
for archetype in self.world.archetypes().iter() { | ||
for scene_entity in archetype.entities() { | ||
let entity = entity_map | ||
.entry(*scene_entity) | ||
.or_insert_with(|| world.spawn().id()); | ||
for component_id in archetype.components() { | ||
let component_info = self | ||
.world | ||
.components() | ||
.get_info(component_id) | ||
.expect("component_ids in archetypes should have ComponentInfo"); | ||
|
||
let reflect_component = type_registry | ||
.get(component_info.type_id().unwrap()) | ||
.ok_or_else(|| SceneSpawnError::UnregisteredType { | ||
type_name: component_info.name().to_string(), | ||
}) | ||
.and_then(|registration| { | ||
registration.data::<ReflectComponent>().ok_or_else(|| { | ||
SceneSpawnError::UnregisteredComponent { | ||
type_name: component_info.name().to_string(), | ||
} | ||
}) | ||
})?; | ||
reflect_component.copy_component(&self.world, world, *scene_entity, *entity); | ||
} | ||
} | ||
} | ||
for registration in type_registry.iter() { | ||
if let Some(map_entities_reflect) = registration.data::<ReflectMapEntities>() { | ||
map_entities_reflect | ||
.map_entities(world, entity_map) | ||
.unwrap(); | ||
} | ||
} | ||
Ok(()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: this was extracted from the SceneSpawner::spawn_sync_internal
method. I think it belongs here, the same way the code for instantiating a DynamicScene
belongs in dynamic_scene.rs
} | ||
|
||
pub fn spawn(&mut self, scene_handle: Handle<Scene>) -> InstanceId { | ||
/// Spawn a dynamic scene. See [`SceneSpawner::spawn`]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The short doc strings with reference to the relevant Scene
implementation is to avoid repeating the same doc string over and over. Since we have here 5 methods that are basically identical with a single DynamicScene
, deduplicating the doc helps tremendously.
/// This will only update the world when [`scene_spawner_system`] runs, see | ||
/// [`SceneSpawner::despawn_sync`] for a method with immediate world | ||
/// update. | ||
pub fn despawn(&mut self, scene_handle: Handle<Scene>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one of the only API breaking change. Before, this method accepted a Handle<DynamicScene>
, which is surprising, given that spawn
, spawn_sync
and spawn_as_child
all accept a Handle<Scene>
I think it's justified to change the API to be more consistent. The new equivalent method is despawn_dynamic
61b1634
to
d1aec75
Compare
* Merge code in `SceneSpawner` for managing `DynamicScene` and `Scene`. * Add back hot reloading for `Scene`s, fixes bevyengine#3759 * Add ability to despawn `Scene`s * Add ability to spawn `DynamicScene` as child of existing entities * Add documentation to `SceneSpawner` methods. * Add `write_to_world` method to `Scene` (does the same as `DynamicScene::write_to_world`) This fixes bevyengine#3759 see this commit's merging PR for further details about this change. Merge PR: bevyengine#4552
* Merge code in `SceneSpawner` for managing `DynamicScene` and `Scene`. * Add back hot reloading for `Scene`s, fixes bevyengine#3759 * Add documentation to `SceneSpawner` methods. * Add `write_to_world` method to `Scene` (does the same as `DynamicScene::write_to_world`) This fixes bevyengine#3759 see this commit's merging PR for further details about this change. Merge PR: bevyengine#4552 A previous version of this change merged `SceneSpawner::*{,_dynamic}`, it made it too difficult to use the `SceneSpawner` so it was reverted.
11fa86c
to
9699f0d
Compare
3ca1d12
to
ca6666f
Compare
} | ||
|
||
#[derive(Debug, Clone)] | ||
struct SpawnCommand { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should avoid "Command" in the name of things that do not impl the Command
trait
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code and doc changes LGTM, and I've verified this works locally.
bors r+
Merge conflict. |
@nicopap merge conflicts, sorry! Should be simple ones. |
Removing the Ready-For-Final-Review label until this is fixed. |
This will require an entire rewrite soon enough. I'll close it |
SceneSpawner
refactorThis PR reworks the
scene_spawner.rs
module inbevy_scene
. Afterhunting for the bug causing #3759, I determined that it was the
culprit. Having difficulties parsing the code myself, I decided
to refactor it.
Relationship with merging of
Scene
andDynamicScene
There has been discussion about getting rid of either
Scene
orDynamicScene
, or merging the two.This PR does not merge
Scene
andDynamicScene
it only limitsitself to refactoring
scene_spawner.rs
. However, the changesintroduced will likely make it easier to transition to an
implementation where only a single
Scene
type exists.Improvements
SceneSpawner
for managingDynamicScene
andScene
.Scene
s, fixesScene
s do not hot reload #3759SceneSpawner
methods.write_to_world
method toScene
(does the same asDynamicScene::write_to_world
)Tests
The tests would require manipulating
ResMut<Assets<T>>
, which I don'tknow how to do. So I left them out.
Further cleanup
Removing duplication in API surface
An earlier version of this PR merged the
SceneSpawner::dynamic_foo
andSceneSpawner::foo
methods by making them generic over the handle they accept.However, it was judged too burdensome for the user. It required adding type specification
on existing code, since it doesn't play nice with
asset_server.load()
which introducesan unbound type variable.
User migration guide
After the second commit, the migration guide consists of:
SceneSpawner::despawn
toSceneSpawner::despawn_dynamic
(note that you should be usingSceneBundle
s andremove
ing theHandle<Scene>
andHandle<DynamicScene>
component instead in any case)Question for reviewers
<T: Into<SceneHandle>>
change? Now that scenes are mostly added throughSceneBundle
, there is not as much use of theSceneSpawner
API with unbound generic types.