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.

Sign up for GitHub

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

Jump to bottom

Fixed panic in pbr example #16976

Merged
merged 2 commits into from
Dec 27, 2024
Merged

Fixed panic in pbr example #16976

merged 2 commits into from
Dec 27, 2024

Conversation

pin3-free
Copy link
Contributor

Objective

Solution

  • For now it's sufficient to introduce a never_param_warn call when adding the fallible system into the app

Testing

  • Tested on my machine via cargo r --example pbr, it built and ran successfully

@mdickopp
Copy link
Contributor

Thanks, this fixes the panic for me.

@mockersf mockersf added this to the 0.15.1 milestone Dec 26, 2024
@pin3-free @mockersf
Update examples/3d/pbr.rs
7a7220f 
Co-authored-by: François Mockers <[email protected]>
@rparrett rparrett mentioned this pull request Dec 26, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Dec 26, 2024
@rparrett @mockersf
Fix panics in scene_viewer and audio_control (#16983)
78d2149 
# Objective

Fixes #16978

While testing, discovered that the morph weight interface in
`scene_viewer` has been broken for a while (panics when loaded model has
morph weights), probably since #15591. Fixed that too.

While testing, saw example text in morph interface with [wrong
padding](https://bevyengine.org/learn/contribute/helping-out/creating-examples/#visual-guidelines).
Fixed that too. Left the small font size because there may be a lot of
morphs to display, so that seems intentional.

## Solution

Use normal queries and bail early

## Testing

Morph interface can be tested with
```
cargo run --example scene_viewer assets/models/animated/MorphStressTest.gltf
```

## Discussion

I noticed that this fix is different than what is happening in #16976.
Feel free to discard this for an alternative fix. I opened this anyway
to document the issue with morph weight display.

This is on top of #16966 which is required to test.

---------

Co-authored-by: François Mockers <[email protected]>
Co-authored-by: François Mockers <[email protected]>
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 26, 2024
@alice-i-cecile alice-i-cecile added C-Examples An addition or correction to our examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Dec 26, 2024
Merged via the queue into bevyengine:main with commit 80094c6 Dec 27, 2024
30 checks passed
@mdickopp mdickopp mentioned this pull request Dec 27, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Dec 29, 2024
@mdickopp
Fix confusing comment in pbr example (#16996)
6114347 
# Objective

After a recent fix for a panic in the pbr example (#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`.
mockersf added a commit that referenced this pull request Jan 3, 2025
@rparrett @mockersf
Fix panics in scene_viewer and audio_control (#16983)
6138575 
Fixes #16978

While testing, discovered that the morph weight interface in
`scene_viewer` has been broken for a while (panics when loaded model has
morph weights), probably since #15591. Fixed that too.

While testing, saw example text in morph interface with [wrong
padding](https://bevyengine.org/learn/contribute/helping-out/creating-examples/#visual-guidelines).
Fixed that too. Left the small font size because there may be a lot of
morphs to display, so that seems intentional.

Use normal queries and bail early

Morph interface can be tested with
```
cargo run --example scene_viewer assets/models/animated/MorphStressTest.gltf
```

I noticed that this fix is different than what is happening in #16976.
Feel free to discard this for an alternative fix. I opened this anyway
to document the issue with morph weight display.

This is on top of #16966 which is required to test.

---------

Co-authored-by: François Mockers <[email protected]>
Co-authored-by: François Mockers <[email protected]>
mockersf added a commit that referenced this pull request Jan 3, 2025
@pin3-free @mockersf
Fixed panic in pbr example (#16976)
e6256e7 
# Objective

- Fixes #16959
- The `pbr.rs` example in the 3d section panicked because of the changes
in #16638, that was not supposed to happen

## Solution

- For now it's sufficient to introduce a `never_param_warn` call when
adding the fallible system into the app

## Testing

- Tested on my machine via `cargo r --example pbr`, it built and ran
successfully

---------

Co-authored-by: Freya Pines <[email protected]>
Co-authored-by: François Mockers <[email protected]>
mockersf pushed a commit that referenced this pull request Jan 3, 2025
@mdickopp @mockersf
Fix confusing comment in pbr example (#16996)
a590adc 
After a recent fix for a panic in the pbr example (#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`.

- 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`.

Ran `cargo run --example pbr`.
ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Jan 6, 2025
@rparrett @mockersf @ecoskey
Fix panics in scene_viewer and audio_control (bevyengine#16983)
e2925eb 
# Objective

Fixes bevyengine#16978

While testing, discovered that the morph weight interface in
`scene_viewer` has been broken for a while (panics when loaded model has
morph weights), probably since bevyengine#15591. Fixed that too.

While testing, saw example text in morph interface with [wrong
padding](https://bevyengine.org/learn/contribute/helping-out/creating-examples/#visual-guidelines).
Fixed that too. Left the small font size because there may be a lot of
morphs to display, so that seems intentional.

## Solution

Use normal queries and bail early

## Testing

Morph interface can be tested with
```
cargo run --example scene_viewer assets/models/animated/MorphStressTest.gltf
```

## Discussion

I noticed that this fix is different than what is happening in bevyengine#16976.
Feel free to discard this for an alternative fix. I opened this anyway
to document the issue with morph weight display.

This is on top of bevyengine#16966 which is required to test.

---------

Co-authored-by: François Mockers <[email protected]>
Co-authored-by: François Mockers <[email protected]>
ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Jan 6, 2025
@pin3-free @mockersf @ecoskey
Fixed panic in pbr example (bevyengine#16976)
187032a 
# Objective

- Fixes bevyengine#16959
- The `pbr.rs` example in the 3d section panicked because of the changes
in bevyengine#16638, that was not supposed to happen

## Solution

- For now it's sufficient to introduce a `never_param_warn` call when
adding the fallible system into the app

## Testing

- Tested on my machine via `cargo r --example pbr`, it built and ran
successfully

---------

Co-authored-by: Freya Pines <[email protected]>
Co-authored-by: François Mockers <[email protected]>
ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Jan 6, 2025
@mdickopp @ecoskey
Fix confusing comment in pbr example (bevyengine#16996)
af7b55f 
# 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`.
mrchantey pushed a commit to mrchantey/bevy that referenced this pull request Feb 4, 2025
@rparrett @mockersf @mrchantey
Fix panics in scene_viewer and audio_control (bevyengine#16983)
066aa55 
# Objective

Fixes bevyengine#16978

While testing, discovered that the morph weight interface in
`scene_viewer` has been broken for a while (panics when loaded model has
morph weights), probably since bevyengine#15591. Fixed that too.

While testing, saw example text in morph interface with [wrong
padding](https://bevyengine.org/learn/contribute/helping-out/creating-examples/#visual-guidelines).
Fixed that too. Left the small font size because there may be a lot of
morphs to display, so that seems intentional.

## Solution

Use normal queries and bail early

## Testing

Morph interface can be tested with
```
cargo run --example scene_viewer assets/models/animated/MorphStressTest.gltf
```

## Discussion

I noticed that this fix is different than what is happening in bevyengine#16976.
Feel free to discard this for an alternative fix. I opened this anyway
to document the issue with morph weight display.

This is on top of bevyengine#16966 which is required to test.

---------

Co-authored-by: François Mockers <[email protected]>
Co-authored-by: François Mockers <[email protected]>
mrchantey pushed a commit to mrchantey/bevy that referenced this pull request Feb 4, 2025
@pin3-free @mockersf @mrchantey
Fixed panic in pbr example (bevyengine#16976)
0345044 
# Objective

- Fixes bevyengine#16959
- The `pbr.rs` example in the 3d section panicked because of the changes
in bevyengine#16638, that was not supposed to happen

## Solution

- For now it's sufficient to introduce a `never_param_warn` call when
adding the fallible system into the app

## Testing

- Tested on my machine via `cargo r --example pbr`, it built and ran
successfully

---------

Co-authored-by: Freya Pines <[email protected]>
Co-authored-by: François Mockers <[email protected]>
mrchantey pushed a commit to mrchantey/bevy that referenced this pull request Feb 4, 2025
@mdickopp @mrchantey
Fix confusing comment in pbr example (bevyengine#16996)
3b6426e 
# 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`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mockersf mockersf mockersf left review comments

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@mdickopp mdickopp mdickopp approved these changes

Assignees
No one assigned
Labels
C-Examples An addition or correction to our examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
0.15.1
Development

Successfully merging this pull request may close these issues.

Example 3d/pbr.rs panics
4 participants
@pin3-free @mdickopp @alice-i-cecile @mockersf