The Big CMake + MinGW + Sound PR#127
The Big CMake + MinGW + Sound PR#127TheBrokenRail wants to merge 29 commits intonbcraft-org:masterfrom
Conversation
| }; No newline at end of file | ||
| }; | ||
|
|
||
| #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.
|
|
||
| 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 |
|
When I get more time, I'm going to split this up into multiple PRs. Until then, I'll mark it as WIP. |
* Fixed building on Windows * Added support for mob sounds * Added support for fire sounds
|
We should really be loading all of our SoundDesc structs dynamically based on the file hierarchy inside /assets/sounds. MCPE 0.12.1 has an "example" of this, even though it's getting most of its info from a JSON file. |
* Re-enabled lighting & normals that were disabled due to merge * Re-added smooth outline rendering as fallback * Fixed OpenGL reference in CMake files
|
@TheBrokenRail Could you please fix the CMake files? Not sure why the SDL2 and GLES compatibility layer stuff is throwing back errors. I'm gonna make sure the Xcode project works for macOS and iOS tomorrow. |
|
Additionally, it's worth nothing, that for whatever reason, an Ogg Vorbis decoder is already present in one of our thirdparty libraries in ~/thirdparty/stb_image/include/stb_vorbis.c |
|
|
CMake changes are in #149. |
|
@TheBrokenRail For clarification, are sound changes also now being split out into a new PR as well? If so, is this PR going to be archived/closed? |
|
Yeah, once I make the sound PR, I'm closing this one. |
|
This branch is a disaster. |
Cool. I'm planning on adding OGG support on my flight back to the U.S. |
|
The sound code has been split off into its own PR: #150. |
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.