Page MenuHomeWildfire Games

Add support for hotloading of sound-related files
Needs ReviewPublic

Authored by snokis on May 5 2020, 4:37 PM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

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.

Test Plan

I've tested manually be changing parameters in sound groups and replacing
currently playing music and ambient sounds. Not sure how to automate it.

Event Timeline

snokis created this revision.May 5 2020, 4:37 PM
Vulcan added a comment.May 5 2020, 4:39 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/661/display/redirect

Vulcan added a comment.May 5 2020, 4:49 PM

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

Stan added a subscriber: Stan.EditedMay 5 2020, 4:59 PM

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 :)

536

nullptr :)

639

nullptr :)

648

nullptr :)

662

nullptr :)

787

Use nullptr instead.

943

Not sure it's worth the function call, I guess it won't be called often.
item maybe? could be a const method

Angen added a subscriber: Angen.May 5 2020, 5:02 PM
Angen added inline comments.
source/soundmanager/SoundManager.cpp
233

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

Angen added a comment.May 5 2020, 5:34 PM

thank you for working on this,
sorry my battery is low and i do not have cabel

source/soundmanager/SoundManager.cpp
237

You can omit { and } when only one line is part of the branch, for loop, while loop etc

255

remove return

529

nullptr

541

nullptr

637

Second parameter is super redundant, please remove it.
Also deleting not owned pointer is not safe.

639

nuke

641

Locale variable

Stan added a comment.Jul 29 2020, 1:17 AM

Hey, any news?

snokis added a comment.Aug 9 2020, 6:01 PM
In D2723#126380, @Stan wrote:

Hey, any news?

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?

asterix added a subscriber: asterix.Aug 9 2020, 6:08 PM
In D2723#126380, @Stan wrote:

Hey, any news?

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?

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.

Stan added a comment.Aug 9 2020, 6:09 PM

Well it's not a priority but it a nice feature for testing audio :)

snokis updated this revision to Diff 13418.Sep 5 2020, 5:05 PM
  • Fix nullptrs and comment-style
  • Remove redundant parameter in LoadGroup
Vulcan added a comment.Sep 5 2020, 5:12 PM

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

Angen added inline comments.Sep 5 2020, 6:05 PM
source/soundmanager/SoundManager.cpp
264

{ under if

284

please remove this empty line

645

please replace that two lines with SAFE_DELETE(group) https://trac.wildfiregames.com/wiki/Coding_Conventions

656

could you invert if and do early return ?

vladislavbelov added inline comments.
source/soundmanager/ISoundManager.h
53

Hotloader isn't a proper name for the method doing something.

source/soundmanager/SoundManager.cpp
225

The s letter seems useless here.

226

Different braces.

229

Space after //.

230

We don't use space after if (. Also some braces aren't needed.

233

Agree with @Angen. This loop makes one core doing nothing.

523

In that cases braces aren't needed.

643

Wrong indentation.

645

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.

Angen added a reviewer: Restricted Owners Package.Sep 9 2020, 3:57 PM