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 audio mixers potentially not showing in AudioMixer visualiser #4875

Merged
merged 5 commits into from
Nov 9, 2021

Conversation

peppy
Copy link
Sponsor Member

@peppy peppy commented Nov 9, 2021

The underlying events would potentially be fired too early and never reach the hooking methods due to the fact the invocation call site is run on the audio thread, before the binding may have completed.

Also done some restructuring for a future PR which allows showing an identifying string on the visualiser.

Can be tested via the existing TestSceneAudioMixerVisualiser while running in multi-thread mode (previously the new mixers would usually not display).

These would potentially never be fired due to the fact the work that is
going to fire them is run on the audio thread, before the binding may
have completed.
@peppy
Copy link
Sponsor Member Author

peppy commented Nov 9, 2021

Regression tests could be added on the aforementioned test scene if required for this one.

Comment on lines 17 to 20
/// <summary>
/// The handle for this mixer.
/// </summary>
public int Handle { get; protected set; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is a little unfortunate because I think the audio subsystem was designed to look mostly agnostic from BASS but this handle is an implementation detail of BASS. On the other side it's kinda neither here nor there as MixerDisplay unceremoniously calls BassMix.MixerGetChannels() anyway...

I dunno, could be wrapped up a little nicer (eg. by exposing an abstract method returning channel levels here) but I'm not going to go and argue about it or anything.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah considering how dependent on bass everything around this is I didn't feel it was worth the time abstracting out.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class is supposed to be bass-agnostic. Is there a reason you couldn't (mixer as BassAudioMixer) instead?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can but felt pretty pointless (the whole visualiser depends on this).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The visualiser isn't the only class using AudioMixer.

@bdach bdach requested a review from smoogipoo November 9, 2021 08:19
Copy link
Contributor

@smoogipoo smoogipoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest looks fine.

Comment on lines 17 to 20
/// <summary>
/// The handle for this mixer.
/// </summary>
public int Handle { get; protected set; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class is supposed to be bass-agnostic. Is there a reason you couldn't (mixer as BassAudioMixer) instead?

@smoogipoo smoogipoo merged commit b35ccf5 into ppy:master Nov 9, 2021
@peppy peppy deleted the fix-mixer-event-firing branch November 18, 2021 04:24
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.

None yet

3 participants