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.

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

Improve logic related to editing audio buses (and prevent crashes) #74560

Merged
merged 1 commit into from
Mar 10, 2023

Conversation

YuriSizov
Copy link
Contributor

@YuriSizov YuriSizov commented Mar 7, 2023

Fixes #74374 and a bunch of related errors.

Generally speaking, the EditorAudioBus and EditorAudioBuses code is pretty nasty. I'm not exactly sure why issues are only reported now for 4.0 stable, because most of the relevant code has been like that for some time. Perhaps there are other reports that are relevant here? Do link them if you know them.

Handling of bus renaming is all sorts of incorrect, and the same applies to reordering them as well. Most of the problems are obvious now because AudioServer emits the bus_layout_changed signal when buses are renamed, which triggers a rebuild of the entire layout editor. When this happens old code memdeletes all the existing nodes in place, which is a big no-no as it leads to nasty race conditions and bad memory bugs (and that's the reason for the reported crash, more or less). I've replaced it with a graceful remove_child & queue_free. Same is done to the placeholder node used for drag'n'drop indication.

But the thing is, the part of the editor that handles bus renames is written in a way that doesn't assume that the entire layout is rebuilt. There is a lot of code to gracefully handle that case and update only the relevant parts. Furthermore, I'm not sure if AudioServer firing the bus_layout_changed signal is intended or a bug. It doesn't send one when setting the bus send value, for example. Something is fishy here. But I am not familiar with the audio system, so I can only patch the editor side. And thus I added a flag to prevent full rebuild of the layout panel when we are already handling it gracefully via the renaming handler.

I've tested a bunch of cases of renaming and reordering buses and fixed issues as they appeared, so there are some extra fixes on top of all that. Overall, it should work better and faster now.


Although, drag'n'drop seems to still be slow, and it has issues with detecting which node we're hovering over due to controls eating input. It wasn't critical enough for me to dive into now, but it's something worth fixing in future.

@akien-mga akien-mga merged commit 2dc8ad2 into godotengine:master Mar 10, 2023
@akien-mga
Copy link
Member

Thanks!

@YuriSizov YuriSizov deleted the editor-audio-bus-congestion branch March 10, 2023 21:59
@YuriSizov
Copy link
Contributor Author

Cherry-picked for 4.0.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Renaming an audio bus crashes the editor
2 participants