Skip to content

Commit

Permalink
Fix confusing comment in pbr example (bevyengine#16996)
Browse files Browse the repository at this point in the history
# Objective

After a recent fix for a panic in the pbr example (bevyengine#16976), the code
contains the following comment:

```rust
// This system relies on system parameters that are not available at start
// Ignore parameter failures so that it will run when possible
.add_systems(Update, environment_map_load_finish.never_param_warn())
```

However, this explanation is incorrect. `EnvironmentMapLabel` is
available at start. The real issue is that it is no longer available
once it has been removed by `environment_map_load_finish`.

## Solution

- Remove confusing/incorrect comment and `never_param_warn()`.
- Make `Single<Entity, With<EnvironmentMapLabel>>` optional in
`environment_map_load_finish`, and check that the entity has not yet
been despawned.

Since it is expected that an entity is no longer there once it has been
despawned, it seems better to me to handle this case in
`environment_map_load_finish`.

## Testing

Ran `cargo run --example pbr`.
  • Loading branch information
mdickopp authored and mrchantey committed Feb 4, 2025
1 parent 4b7a1fe commit 3b6426e
Showing 1 changed file with 6 additions and 5 deletions.
11 changes: 6 additions & 5 deletions examples/3d/pbr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,7 @@ fn main() {
App::new()
.add_plugins(DefaultPlugins)
.add_systems(Startup, setup)
// This system relies on system parameters that are not available at start
// Ignore parameter failures so that it will run when possible
.add_systems(Update, environment_map_load_finish.never_param_warn())
.add_systems(Update, environment_map_load_finish)
.run();
}

Expand Down Expand Up @@ -130,7 +128,7 @@ fn environment_map_load_finish(
mut commands: Commands,
asset_server: Res<AssetServer>,
environment_map: Single<&EnvironmentMapLight>,
label_entity: Single<Entity, With<EnvironmentMapLabel>>,
label_entity: Option<Single<Entity, With<EnvironmentMapLabel>>>,
) {
if asset_server
.load_state(&environment_map.diffuse_map)
Expand All @@ -139,7 +137,10 @@ fn environment_map_load_finish(
.load_state(&environment_map.specular_map)
.is_loaded()
{
commands.entity(*label_entity).despawn();
// Do not attempt to remove `label_entity` if it has already been removed.
if let Some(label_entity) = label_entity {
commands.entity(*label_entity).despawn();
}
}
}

Expand Down

0 comments on commit 3b6426e

Please sign in to comment.