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

Fix a lag spike in the audio bus editor #81611

Closed
wants to merge 1 commit into from

Conversation

MewPurPur
Copy link
Contributor

@MewPurPur MewPurPur commented Sep 13, 2023

I disconnected bus_layout_changed from the function that rebuilds audio buses, as renaming audio buses causes the whole audio bus editor to rebuild which is undesired. This required a hefty workaround to avoid a lag spike when renaming, and it also caused duplicating to rebuild the audio bus editor twice just to add a new bus, and to then rename it. Now I removed the hefty workaround.

On top of unnecessary rebuilds from renaming, UndoRedo often asked for YET another rebuild. Rebuilding is an expensive operation that can cause a visible lag spike, especially if you have a lot of audio buses, which made this blunder quite noticeable. The rebuilds UndoRedo asks for are now the only ones I've kept (actually I removed one when resetting a bus volume, as you can ask for a single bus to be updated there)

  • Duplicating buses: 3 rebuilds -> 1 rebuild
  • Adding, moving, and deleting buses: 2 rebuilds -> 1 rebuild
  • Resetting bus volume: 1 rebuild -> no rebuild

Regarding the comment I wiped - I'm positive that renaming audio buses should emit bus_layout_changed - as it changes how you interact with the audio bus system, not just how it affects audio. For example, AudioStreamPlayer relies on this signal to have an up-to-date list of buses you can pick in the inspector.

@MJacred
Copy link
Contributor

MJacred commented Sep 13, 2023

For renaming audio buses, I wish for a new signal: bus_name_changed(bus_index, old_name, new_name)

For now, we could add bus_name_changed(bus_index, old_name, new_name) additionally (yes, 2 signals emitted at the same time)
and make a second PR with compatibility breaking changes, i.e. not emitting bus_layout_changed when changing the name.

My reasoning is: When I developed an audio manager plugin, any bus renamings in the editor ended up with me having to cache all bus names and then on each bus_layout_changed signal I need to re-check if (1) a rename happened and (2) guess/deduce what the new name is and (3) change bus name for all managed audio files.
At least 1 + 2 would become obsolete right away.

@MewPurPur
Copy link
Contributor Author

Hmm okay. I don't know enough about audio to judge if this is a reasonable compat-break?

@MJacred
Copy link
Contributor

MJacred commented Sep 13, 2023

Once bus_layout_changed is not emitted anymore for renames, the only other internal change is that all 3 AudioStreamPlayer* classes need to listen to that signal so that they call notify_property_list_changed();.

And if any project relies on the bus_layout_changed for bus renames, they would need to connect to the new signal. Only a simple fix would be required, but it counts as compat-breaking I reckon?

Hm… I guess I can open an issue+PR and then see what the feedback is

@MewPurPur
Copy link
Contributor Author

MewPurPur commented Sep 13, 2023

Yes, please open a proposal! Would be good to bring it up with audio contribs. These systems were implemented by reduz like 7 years ago and I think I can easily implement this change if we agree on it.

(actually I'm going to make an alternative PR in a bit)

@YuriSizov YuriSizov requested a review from a team September 14, 2023 15:35
@YuriSizov
Copy link
Contributor

YuriSizov commented Sep 15, 2023

Note on merging: This is one of two alternatives to address the problem, #81641 is the other one. We likely need to pick just one, because they don't really make sense together.

Edit: On RC reduz confirmed that #81641 is a better option.

@akien-mga
Copy link
Member

Superseded by #81641.

@akien-mga akien-mga closed this Sep 16, 2023
@MewPurPur MewPurPur deleted the bus-go-brrrr branch September 16, 2023 19:11
@YuriSizov YuriSizov removed this from the 4.x milestone Sep 18, 2023
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.

6 participants