Handle changes of .ogg and .xml files in the audio/ directory. Files
will be reloaded from the SoundManagerWorker thread and not at any random time.
This will deal with changes to sound groups, standalone oggs and oggs used
by sound groups. Tell me if I've missed anything.
Details
Details
- Reviewers
- None
- Group Reviewers
Restricted Owners Package (Owns No Changed Paths)
I've tested manually be changing parameters in sound groups and replacing
currently playing music and ambient sounds. Not sure how to automate it.
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- sound_file_hotloading
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 11579 Build 21264: Vulcan Build Jenkins Build 21263: Vulcan Build (macOS) Jenkins Build 21262: Vulcan Build (Windows) Jenkins Build 21261: arc lint + arc unit
Event Timeline
Comment Actions
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/macos-differential/661/display/redirect
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues:
Executing section Source...
source/soundmanager/ISoundManager.h
| 27| class·ISoundManager
| | [MAJOR] CPPCheckBear (syntaxError):
| | Code 'classISoundManager{' is invalid C code. Use --std or --language to configure the language.
source/soundmanager/SoundManager.h
| 27| #include·"items/ISoundItem.h"
| | [MAJOR] CPPCheckBear (syntaxError):
| | Code 'classISoundManager{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2076/display/redirect
Comment Actions
Hey, thanks for the patch! Here are some style comments.
You can try to add test_ files, changing them and making sure they are hotloaded see how it's done in others areas :)
You should also try with mods. In both the source (binaries/data/mods/) and ~/.local/share/0ad/ https://trac.wildfiregames.com/wiki/GameDataPaths It should work everywhere, no matter what is loaded, as long as the file is not overriden by another mod :)
You might want to pass const refs instead of pointers where possible too.
Btw, if feel free to make other patches, simple or not :) You can make as many as you want
| source/soundmanager/ISoundManager.h | ||
|---|---|---|
| 53 | Is that name used for other classes doing hotloading? | |
| source/soundmanager/SoundManager.cpp | ||
| 124 | Comments on top. Ends with periods :) | |
| 246 | Not sure if we use the XML counterpart XMB (Binary xml files) | |
| 260 | Is .ogg defined somewhere? | |
| 268 | I guss you can use a range based loop here :) | |
| 540 | nullptr :) | |
| 647 | nullptr :) | |
| 656 | nullptr :) | |
| 670 | nullptr :) | |
| 798 | Use nullptr instead. | |
| 954 | Not sure it's worth the function call, I guess it won't be called often. | |
| source/soundmanager/SoundManager.cpp | ||
|---|---|---|
| 232 | could you please take a look into wait notify for example? http://www.cplusplus.com/reference/condition_variable/condition_variable/ active waiting is not good idea | |
Comment Actions
thank you for working on this,
sorry my battery is low and i do not have cabel
| source/soundmanager/SoundManager.cpp | ||
|---|---|---|
| 236 | You can omit { and } when only one line is part of the branch, for loop, while loop etc | |
| 255 | remove return | |
| 533 | nullptr | |
| 545 | nullptr | |
| 645 | Second parameter is super redundant, please remove it. | |
| 647 | nuke | |
| 649 | Locale variable | |
Comment Actions
Yep, soon.. Sorry, started working again, lost interest and then went off-grid for a few weeks :) Is this something you want for the next release?
Comment Actions
Ideally yes, @Stan also made some patches related to audio but unfortunately not everyone is expert in this area, that is why we do not have mixing and other supported features yet and it takes a lot of time in some cases even years.
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
builderr-release-macos.txt /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libsimulation2.a(precompiled.o) has no symbols /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libengine.a(precompiled.o) has no symbols /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libatlas.a(precompiled.o) has no symbols /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libgui.a(precompiled.o) has no symbols
Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1494/display/redirect
| source/soundmanager/SoundManager.cpp | ||
|---|---|---|
| 264 | { under if | |
| 284 | please remove this empty line | |
| 653 | please replace that two lines with SAFE_DELETE(group) https://trac.wildfiregames.com/wiki/Coding_Conventions | |
| 664 | could you invert if and do early return ? | |
| source/soundmanager/ISoundManager.h | ||
|---|---|---|
| 53 | Hotloader isn't a proper name for the method doing something. | |
| source/soundmanager/SoundManager.cpp | ||
| 224–240 | The s letter seems useless here. | |
| 225 | Different braces. | |
| 228 | Space after //. | |
| 229 | We don't use space after if (. Also some braces aren't needed. | |
| 232 | Agree with @Angen. This loop makes one core doing nothing. | |
| 527 | In that cases braces aren't needed. | |
| 651 | Wrong indentation. | |
| 653 | I'd suggest to use std::unique_ptr. | |
| source/soundmanager/SoundManager.h | ||
| 52 | using instead of typedef. ReloadList is a too common name for the global namespace, should be inside the class or should be in a named namespace. What's the most frequent operation with the list: iteration or search? | |
| 113 | What's the difference between HotloadSmth, LoadSmth and ReloadSmth? | |
| 175 | Should be const I suppose. | |