Re-instance audio previews during resource re-import #51377
Conversation
ellenhp
force-pushed
the
fix_crash_on_rename
branch
from
e6a107a
to
387cb74
Compare
| @@ -234,6 +246,12 @@ void AudioStreamPreviewGenerator::_notification(int p_what) { | |||
| to_erase.pop_front(); | |||
| } | |||
| } | |||
| if (p_what == NOTIFICATION_ENTER_TREE) { | |||
There was a problem hiding this comment.
These should probably be else if, or even better a switch() { case ... } statement to be more consistent with the rest of the codebase.
There was a problem hiding this comment.
hmmm, from what I've seen which is admittedly not much the godot codebase overwhelmingly uses individual if statements. I can find a few else if usages for checking p_what but not many. I've certainly never seen one without explicitly searching for it.
There was a problem hiding this comment.
From a quick search in vs code it looks like about half of them use switch. I thought I read somewhere recently that one of the contributors said switch was preferred, I'll see if I can find it :). With compiler optimizations I'd be surprised if there was a difference in performance though.
There was a problem hiding this comment.
Ok. I'm personally not a fan of C-style switch statements because they just feel syntactic sugar on top of goto's. I know all control structures are syntactic sugar on top of goto's but with switch statements it's especially bad :) I'll definitely just use whatever's convention here though.
There was a problem hiding this comment.
Both styles are fine. We tend to move towards switches for _notification specifically, but here existing code was using if and refactoring is not necessarily something that should be done in this PR.
It's fine to do it if you want though.
else if on the other hand would be weird, that's not a style we use for _notification.
|
This doesn't seem to work if the audio is being played back in the editor's audio preview window, but it does work otherwise. I have some improvements that should deal with the case where it's playing. I'll probably push those later tonight pacific time and make a PR shortly after. |
|
I think there's some behavior here I don't understand. This crash may actually be preferable to what I'm observing, which is that saving a project after having renamed an ogg file actually causes it to fail to open. I'm switching this to draft to make sure it doesn't get merged, because it's true that it fixes a crash but I'd rather crash Godot than corrupt a project. |
Calinou
added
bug
topic:editor
crash
cherrypick:3.3
cherrypick:3.x
|
This is fixed in 4.0 as far as I can tell, so no need for this to be around anymore. |
akien-mga
added
archived
and removed
cherrypick:3.x
|
I guess this could still be worth merging in the |
I think this fixes #27800 in most cases. It's not really a complete solution because if the audio stream preview is playing audio, it's still possible to get a use-after-free if the audio thread does a mix after
AudioStreamOGGVorbis::clear_dataand beforeAudioStreamPreviewGenerator::_update_audio_streams. I think I have a fix for #26964 that should at least prevent a crash in that situation, but it's still necessary for the preview window to re-instance all of its preview playbacks, so this PR will be necessary from a usability perspective.