-
Notifications
You must be signed in to change notification settings - Fork 0
audio: audio engine rework w/ miniaudio #1
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
base: master
Are you sure you want to change the base?
Conversation
swagtoy
left a comment
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.
Seems good; some requests
src/audio/mixer.cpp
Outdated
| if (result != MA_SUCCESS) | ||
| { | ||
| Logger::error("Mixer", "Oops! ded."); | ||
| std::exit(-1); |
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.
Nein :) for now throw an exception, but try to handle it instead of panicking, we want the game to keep running but display an error
| #define SUPERTUX_SRC_AUDIO_MUSIC_DATA_HPP | ||
|
|
||
| #include <string> | ||
|
|
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.
spacing
src/audio/music_reader.cpp
Outdated
|
|
||
| } | ||
|
|
||
| MusicData MusicReader::open(const std::string& filename) |
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.
Check for errors, compare int-max and negative values. I should probably add better functions to the parser for that..
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.
also put return type on separate line
| std::string join(const std::string &a, const std::string& b) | ||
| { | ||
| return a + "/" + b; | ||
| } |
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.
Sure..... :)
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.
This is what was being done for FS::path, so...
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.
| message(STATUS "NOT using SDL3 Mixer...") | ||
| endif (NEOTUX_USE_MIXER) | ||
| if (PSP) | ||
| message(STATUS "Building PSP version...") |
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.
when merging, try to rebase, I moved this stuff now to psp.cmake
| @@ -1,5 +1,4 @@ | |||
| #cmakedefine NEOTUX_BGFX | |||
| #cmakedefine NEOTUX_USE_MIXER | |||
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.
No go
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.
I made a comment explicity saying not to remove this bit but I mustve forgot to submit it; please add this logic back though.
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.
This logic can be emulated by using the Null miniaudio backend.
|
I think for now you should abstract the audio backend and select it at compile time in case we want to try something else one day. Have generic headers in src/audio and audio.hpp, then have src/audio/miniaudio implement. I dont want to hard depend on miniaudio so we can roll our own audio dummy too (i.e miniaudio has compile issues on some machine, or user is lazy) well have to stop globbing, or we can glob all but the audio folder in cmake |
|
we can also maybe use a namepace and just use a global with static stuff instead. That way no need for pimpl |
| std::runtime_error(std::format("{} (ma error: {})", what, result)) | ||
| {} | ||
|
|
||
| struct Mixer::Impl |
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.
See my other comment. We can abstract it all at compile time...
This is a rewrite of the mixer to a better more versatile audio backend (miniaudio) that also adds new features.