-
Notifications
You must be signed in to change notification settings - Fork 627
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
OpenAL cleanup #909
OpenAL cleanup #909
Conversation
This is ready for review! |
src/OpenAL/Extensions/OpenTK.OpenAL.Extensions.Creative.EFX/Presets/ReverbPresets.cs
Outdated
Show resolved
Hide resolved
src/OpenAL/Extensions/OpenTK.OpenAL.Extensions.Creative.EFX/Presets/ReverbPresets.cs
Outdated
Show resolved
Hide resolved
src/OpenAL/Extensions/OpenTK.OpenAL.Extensions.Creative.EFX/Presets/ReverbProperties.cs
Outdated
Show resolved
Hide resolved
src/OpenAL/Extensions/OpenTK.OpenAL.Extensions.EXT.Capture/AudioCapture.cs
Outdated
Show resolved
Hide resolved
src/OpenAL/Extensions/OpenTK.OpenAL.Extensions.EXT.Capture/AudioCapture.cs
Outdated
Show resolved
Hide resolved
src/OpenAL/Extensions/OpenTK.OpenAL.Extensions.EXT.Capture/Capture.cs
Outdated
Show resolved
Hide resolved
I do apologize for the seemingly robotic and daunting review. A lot of this PR is predicated on the fact that the format for long method calls is wrong. It is not, ReSharper just says it is because we can't configure the way we do it. I'm pretty confident my review is just asking you to revert your changes with this regard. Speak to @Nihlus for more info regarding this. |
It's fine, should've asked before doing it. Fixed everything you commented on. |
Will run another pass over this PR, review should be in the next 3 days hopefully. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You missed a few method invocations. Try again, we're close!!
src/OpenAL/Extensions/OpenTK.OpenAL.Extensions.Creative.EFX/Presets/ReverbProperties.cs
Outdated
Show resolved
Hide resolved
src/OpenAL/Extensions/OpenTK.OpenAL.Extensions.EXT.Capture/Capture.cs
Outdated
Show resolved
Hide resolved
That should be everything you mentioned |
Needs some conflict resolution on your side, then I'll do a final review so we can merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome stuff :)
While I wait for #908 to be finished so I can put the last touches on #867, I'm going to start on a project I've been meaning to get to for a while.
This is going to be a pretty big cleanup of the OpenAL bindings; as of right now, the compiler generates a lot of warnings, and Rider has a bunch of issues with the code style. Once this is over, it should build without any complaints.