Sounds

I tried to implement a few sound effect in the code, but I hit a roadblock.

Instead of creating the SDL chunk everytime the sound is needed, I do it once and for all. However this breaks the layering in the code, so may be a new object should be created.

Also, I’m not sure how to properly propagate the sound handler (“mush”) to the objects that need it. I did it for CClient but I feel it’s not right, and couldn’t do it for CGPickable::onHeroVisit. At least the new day now has a sound effect :sunglasses:

If anyone has an idea, I’ll take it.

Patch attached. It’s more a proof of concept than anything else.
vcmi_snd1.diff.zip (3.36 KB)

New patch with more sounds.
vcmi_snd2.diff.zip (5 KB)

New patches:

  • added support for music (now plays the intro)
  • changed the way the sounds are loaded (more efficient and fixes a few bugs)

Not perfect but it’s getting better.
vcmi_snd3.zip (14.2 KB)

I’m very happy that you started working on sounds. I probably wouldn’t touch this feature for another year because it’s irrelevant for game mechanics. On the other hand sound and music are essential element of H3 climate and it will be great to finally hear VCMI :slight_smile:

I’ve taken a look at your patches and I have several remarks / thoughts:

  1. Generally avoid adding includes to the includes (*.h) files if possible. If you use there only a pointer or reference to some undefined previously type (eg. CMusicHandler *) it would be enough to declare only that type (eg. class CMusicHandler;) at the beginning of file.

  2. Playing sounds directly in onHeroVisit methods for particular objects (CObjectHandler.cpp) won’t work. These methods are invoked by server and its results are later sent to client. Server is always silent (and generally invisible for end user). When multiplayer mode is working, server can be on different machine than client. Therefore, sounds can be played only in client.

  3. CGameInterface shouldn’t have pointer for music handler since it’s abstract class to be derived by GUI and AIs. AIs don’t need music, so it would be enough to add this pointer to CPlayerInterface (it’s main class responslible for player GUI). But…

  4. Neither CClient nor CPlayerInterface need its own pointer to music handler because there is already one globally available: in any part of client it should be accessible via CGI->mush. I’d rather use it than add its copies to various parts of client.

  5. I’m not convinced that sending from server to client a soundname with every dialog is a good solution. I think that sounds should be handled solely by the client, server should not be bothered.
    I would create in client (it could be a member of CMusicHandler) a map<int,std::string> with object_ID => sound_played_on_visit. Then I would simply check in CPlayerInterface::heroMoved on every successful move what visitable object is on the destination tile ( cb->getVisitableObjs( details.dst - int3(1,0,0) ) ) and get the soundname - if present - from map. *

Could you try to solve these issues? I can do it on my own but not within several upcoming days (lack of time). I will not commit anything for now, though if you want I can commit soon obviously good part of patches but I don’t know if it’s good to mess with such half-commits now.

Thanks for the contribution!*

Thanks for the review.

Will fix.

That’s good news. I’ll see if I can use that.

[quote=“Tow”]
2. Playing sounds directly in onHeroVisit methods for particular objects (CObjectHandler.cpp) won’t work. These methods are invoked by server and its results are later sent to client. Server is always silent (and generally invisible for end user). When multiplayer mode is working, server can be on different machine than client. Therefore, sounds can be played only in client.

  1. I’m not convinced that sending from server to client a soundname with every dialog is a good solution. I think that sounds should be handled solely by the client, server should not be bothered.
    I would create in client (it could be a member of CMusicHandler) a map<int,std::string> with object_ID => sound_played_on_visit. Then I would simply check in CPlayerInterface::heroMoved on every successful move what visitable object is on the destination tile ( cb->getVisitableObjs( details.dst - int3(1,0,0) ) ) and get the soundname - if present - from map. *

Doing that in the client would require the client to recreate a lot of code that exist on the server. For instance a lot of events on the server side involve only the creation of a dialog. The base information is lost, and thus the client would have to do some legwork to recreate it (and could get it wrong). The server already knows what to do, so why not use it. AIs will just ignore that parameter.

The changes I think will be better are:

  • use a sound id instead of a filename (same I did for the musics with CMusicHandler::musicNames). I could create a simple object with only these defs and it would be inherited by CGPickable (server) and CMusicHandler (client).
  • then move the sdl sound chunk from CSndHandler into CMusicHandler where they really belong.
  • for events where the client cannot guess what sound to play (basically it should be all events including a dialog), the server would send the sound code along.

Don’t commit anything. It’s only half baked yet.[/quote]

New patch. I’m now happy with the infrastructure. Added a few more sounds.
There’s some cleanup left to do, as well as a lot of grunt work to add all the sounds.
vcmi_snd4.zip (20.7 KB)

Macrodefinition counting over 1000 lines… it’s definitely one of the things I’ll never forget in my life :stuck_out_tongue:

AFAIK the only situation, when dialog comes with sound and requires client to guess, is visiting object. Then guessing is trivial, since we have guarantee that at the tile we have at most one visitable object (except of hero itself) and we receive call when hero moves on that object.

However, since your solution is already implemented, let it be. :slight_smile:
Moreover your approach may become even better if we want to add different sounds to various dialogs coming from one object (eg. one sound played at first visit and another on subsequent).

Some remaining issues:
Don’t use type soundBase::soundNames in InfoWindow / BlockingDialog. It’s an enum so it’s not portable (size may vary at different platforms). It can be replaced safely with ui16 (enums are non-negative integers). Then we could even remove CSoundBase.h include from NetPacks.h (less includes in includes = better). For soundType initialization purpose we should fix silence (no sound) id as 0. Then if something doesn’t want to use any sounds, it doesn’t need to include the whole list of them. Same should be done in blockingDialog call-in (use ui16/int for sound id). And I’d make it (when possible) a default parameter set to 0 (most dialogs do not use any sounds).

Also, boost and sndhandler includes should be removed from CMusicHandler.h
Sorry for being so fierce about all these includes but I once (after 0.62 release) spend really much time on fighting with that hell… and my victory was only partial. VCMI is compiling slowly and has terribly messed dependencies, so I really don’t want things to get worse.

I hope it won’t be trouble. :slight_smile:
And, generally - your patch seems to be a great job. Along with artifact support it will be probably main feature of upcoming 0.72 release. Thanks again! :slight_smile:

Next rev. I think I’ve addressed your remarks.
The first 3 patches can go in. If you do, please do one commit per patch.

They other patches are not complete yet.
vcmi_snd5.zip (27.6 KB)

Done, r814, r815, r816. :slight_smile:
Made only minor change to first patch to make it compilable on MSVC (explicit unsigned char* -> char* casting), which was apparently covered by the second one. I’ve also added CSoundBase.h to MSVC project.

There are still some things I’d change but it’s not urgent. Having finally sounds is way more important :slight_smile:
Good job!

This patch add sounds for hero movements. It should follow what H3 does.
vcmi_sound_hero_moving.diff.zip (1.86 KB)

Commited as r818, thanks!
In r819 I made some minor improvements and added several missing sounds to objects.

Thanks for fixing the newTerrain thing.

The first patch fixes a bug in stopSound where all channels would be stopped if -1 is given in parameter.

The second patch add sound to units during combats. There’s a few remaining issues which should take you much less time to fix than me. Search for “todo” in the diff.

And are the duplicates names in crerefnam.txt intentionals, like minautorKing or LightCrossbowman/Archer ?
vcmi_snd6.zip (7.96 KB)

Excellent, I’ve uploaded both patches, (r820, r821). Thanks!
As for todos:

  • defending sounds - I’ll soon add one more if for checking if stack was ordered to Defend
  • entering attackingShowHelper twice with frame==0 - TowDragon should know why it’s so and if it can/should be changed :wink:

Yes, they are. Both names are used in HOTRAITS.TXT entries. Eg. Sir Mullich starts game with “Archer” while other Castle Heroes have “LightCrossbowman”…

Funny. I’ve played that game for years, and I’m still learning some things about it. :unamused:

This patch series contain the remaining changes I have made. Explanantion included in each patch. Basically they add a few more sounds and add music in town and combats.

I had to fix the AI because the bimap header is pulling boost bind, and that creates a conflict. If you can remove that header from cmusichandler.h then this patch shouldn’t be necessary.
vcmi_snd7.zip (11.2 KB)

It was not included in the package…
I removed bimap header from the CMusiChandler.h by moving sounds bimap to the .cpp file (as a global variable). Nasty but is simple, works and doesn’t need changes in the existing code. (Alternatively we can declare bimap template and make sounds member a pointor to bimap, which will be allocated/freed in ctor/dtor).

Music patch (the last one) can’t be applied because the CMusicBase.h header is missing (or did I missed it?). Rest of patches was commited.

You’re right. I forget one file. New patch applied.

As for making “sounds” a global variable, it’s not pretty. Why don’t you use precompiled headers and put all the boost/stl stuff in stdafx.h ?
vcmi_snd8.zip (4.51 KB)

Much prettier then over 1k lines macro…

Firstly, some boost stuff confickts with each other (eg. lambda and bind). Secondly, precompiled headers do not work well for us (I had it turned on some time ago and it caused more trouble than benefit). Another reason is that putting so many headers into every compilation unit makes compilation significantly longer.

Commited, thanks!
Done minor changes to fix compiling on MSVC and remove a few unneeded dependencies (includes, using CGI->mush in CPG). Added missing init of nextMusic (caused crash on showing main menu).

Not pretty… Possible. But try to think about it as of some variation of pimpl idiom :wink:
(hides implementation detail from the rest of world)

Even if we did so, it wouldn’t help to sounds variable - as you saw lambdas and bind don’t like each other. Including everything everywhere seems even less prettier than global variables. Bloated precompiled headers can even slow the compilation and putting all the boost/stl stuff seems… bloating.
I’m considering starting using precompiled headers however but it’ll need some tests which headers can be profitable put there. (Needs tests = time => later)

Thanks for the cleanup. I also had a patch for the missing init for nextMusic, but I posted it under the linux thread.

I understand for the precompiled headers. Damned if you do and damned if you don’t. At least one can drink a beer between compilations :mrgreen: