The Big CMake + MinGW + Sound PR #127
Conversation
| }; | ||
|
|
||
| #define SOUND_SYSTEM SoundSystemDS |
There was a problem hiding this comment.
I disagree with renaming all sound systems to CustomSoundSystem. I especially disagree with your choice of using a #define where a typedef would have fit incredibly well.
I would say revert this and then in each AppPlatform, use preprocessor conditions to determine the sound system in that AppPlatform. And use typedef for that.
You can't really use DirectSound on non-Windows; nor can you use OpenSL ES on desktop Linux and Windows.
There was a problem hiding this comment.
You can't really use DirectSound on non-Windows; nor can you use OpenSL ES on desktop Linux and Windows.
The issue is OpenAL. It can be used on both SDL and Win32, so you end up with:
#ifdef USE_OPENAL
#include "SoundSystemAL.h"
#define SOUND_SYSTEM SoundSystemAL
#else
#include "SoundSystemDS.h"
#define SOUND_SYSTEM SoundSystemDS
#endifIn both platforms/sdl/main.cpp and platforms/windows/main.cpp.
I'll switch to typedefs though, that is nicer.
There was a problem hiding this comment.
Yep, that's fine. I don't mind that at all.
| return ""; | ||
| AssetFile file = readAssetFile("patches/patch_data.txt"); | ||
| std::string out = std::string(file.data, file.data + file.size); | ||
| delete file.data; |
There was a problem hiding this comment.
You should delete data within the destructor and add move and copy constructors to AssetFile. Because we're using C++.
There was a problem hiding this comment.
Wouldn't that require SoundDesc maintaining a copy of AssetFile so the sound data isn't deleted?
There was a problem hiding this comment.
Yes. But you return AssetFile from AppPlatform::readAssetFile so you can just keep the AssetFile in memory (ideally you'd also use std::move to avoid copying)
There was a problem hiding this comment.
So, funny story. Turns out SoundRepository copies SoundDesc a lot.
So the options are:
- Modify
SoundRepositoryto store pointers or references (which in turn would require modifying theSoundSystems). - Add a copy constructor to
AssetFileand copy the sound data whenever it is used, which sounds... really inefficient. - Keep the data externally-managed.
| @@ -176,7 +176,7 @@ void MobRenderer::renderNameTag(Mob* mob, const std::string& str, float x, float | |||
|
|
|||
| glPushMatrix(); | |||
| glTranslatef(x + 0.0f, y + 2.3f, z); | |||
| glNormal3f(0.0f, 1.0f, 0.0f); | |||
|
Also worth noting that I don't really like giant PRs. Which is why I ask that you create separate PRs for different changes from now on. |
|
Yeah, sorry about that. I got a bit carried away (clearly). |
|
Also, I added |
This PR:
CMakeLists.txtso you no longer need tocd platforms/sdlbefore building the project.platforms/sdl/CMakeLists.txtlike you would expect instead of insource/CMakeLists.txt.tools/grabsounds.pynow extracts sounds togame/assets/soundusing LIEF (which is downloaded automatically).AppPlatform::readAssetFile, which has been implemented (and tested) for both SDL, Win32, and native Android.AppPlatform::getPatchDatahas been changed to useAppPlatform::readAssetFile.ln -sto the CI, someone should probably fix that.Logger.