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

Add argument parameter for AESND callbacks #129

Merged
merged 1 commit into from
Jul 13, 2022

Conversation

HTV04
Copy link
Contributor

@HTV04 HTV04 commented May 29, 2022

Adds an argument parameter for AESND's audio and voice callbacks. This allows these static functions to have a pointer to data passed over when called, for example, a pointer to a C++ class instance.

@eku
Copy link

eku commented May 30, 2022

Did you check if this breaks existing homebrew projects?

@WinterMute
Copy link
Member

It's a hard call tbh. I can see the utility of your change but of course this means that the ABI gets broken & old projects that use this interface need updated accordingly.

What I'd suggest is perhaps updating AESND_RegisterVoiceCallback to set the arg to NULL & adding a new function which takes the argument (i.e. AESND_RegisterVoiceCallbackWithArg) instead.

@DacoTaco
Copy link
Member

What I'd suggest is perhaps updating AESND_RegisterVoiceCallback to set the arg to NULL & adding a new function which takes the argument (i.e. AESND_RegisterVoiceCallbackWithArg) instead.

i was looking at this yesterday before going to bed and i was thinking the same. this would make it not break for existing projects. C equivalent of a function overload (aka a new function with a different name in C haha)

@HTV04
Copy link
Contributor Author

HTV04 commented May 30, 2022

It's a hard call tbh. I can see the utility of your change but of course this means that the ABI gets broken & old projects that use this interface need updated accordingly.

What I'd suggest is perhaps updating AESND_RegisterVoiceCallback to set the arg to NULL & adding a new function which takes the argument (i.e. AESND_RegisterVoiceCallbackWithArg) instead.

Done

gc/aesndlib.h Outdated Show resolved Hide resolved
@HTV04 HTV04 force-pushed the aesnd-callback-arg branch 2 times, most recently from 5a62c09 to 5d5fb4e Compare May 30, 2022 16:52
@HTV04 HTV04 requested a review from DacoTaco May 30, 2022 16:52
Copy link
Member

@WinterMute WinterMute left a comment

Choose a reason for hiding this comment

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

These changes will mean code using the old API will emit a single call to the new API instead of a double call.

libaesnd/aesndlib.c Outdated Show resolved Hide resolved
libaesnd/aesndlib.c Outdated Show resolved Hide resolved
@HTV04
Copy link
Contributor Author

HTV04 commented Jun 3, 2022

I moved the 3 non-WithArg functions to aesndlib.h and made them static inline as requested.

@HTV04
Copy link
Contributor Author

HTV04 commented Jul 13, 2022

Not to sound impatient, but is there anything else I should change for my PR? GitHub still says "changes requested," but I think I have made all of the requested changes.

@DacoTaco
Copy link
Member

Not to sound impatient, but is there anything else I should change for my PR? GitHub still says "changes requested," but I think I have made all of the requested changes.

sorry for the waiting. ive been a bit busy.
the comments that request changes have not been marked as resolved, despite being resolved ( i think ). in a PR somebody (either the author of the PR or the reviewer) should mark comments as resolved when they are resolved, it doesn't happen automatically.

ill leave this page open on my pc and take a look at this tonight :)

@DacoTaco DacoTaco merged commit 3d123eb into devkitPro:master Jul 13, 2022
@DacoTaco
Copy link
Member

there, merged.

thanks for the PR @HTV04
sorry it took so long

@HTV04
Copy link
Contributor Author

HTV04 commented Jul 13, 2022

there, merged.

thanks for the PR @HTV04 sorry it took so long

No problem, thanks for merging it!

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

Successfully merging this pull request may close these issues.

4 participants