Fix panics in scene_viewer and audio_control
#16983
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alice-i-cecile
approved these changes
Dec 26, 2024
alice-i-cecile
added
C-Examples
| @@ -34,29 +34,48 @@ fn setup(mut commands: Commands, asset_server: Res<AssetServer>) { | |||
| #[derive(Component)] | |||
| struct MyMusic; | |||
|
|
|||
| fn update_speed(sink: Single<&AudioSink, With<MyMusic>>, time: Res<Time>) { | |||
| fn update_speed(music_controller: Query<&AudioSink, With<MyMusic>>, time: Res<Time>) { | |||
| let Ok(sink) = music_controller.get_single() else { | |||
There was a problem hiding this comment.
Why this? I thought Single here was very intentional and intended to replace this pattern. This seems to just ignore invalid state.
There was a problem hiding this comment.
Single now panics again. Using Option<Single> here would work, but that's kind of a sidegrade.
There was a problem hiding this comment.
Or we can reconfigure the system to ignore failing params
There was a problem hiding this comment.
It's expected that the sink won't be present until PostUpdate on the first frame.
Specifying Single and disabling warnings feels like a dirty way to write this code to me. Option<Single> seems even wackier IMO. But please discuss and feel free to close for an alternative implementation.
Or I can fix it up late tonight or tomorrow if there's consensus the other way.
There was a problem hiding this comment.
This solution is totally fine, just listing alternatives
There was a problem hiding this comment.
Agreed, this solution is fine.
|
Got this while running |
mockersf
added a commit
that referenced
this pull request
Jan 3, 2025
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]>
ecoskey
pushed a commit
to ecoskey/bevy
that referenced
this pull request
Jan 6, 2025
# 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
# 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]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Fixes #16978
While testing, discovered that the morph weight interface in
scene_viewerhas 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. 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
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.