legacyclonk / content

Contains the default definition packs and scenario folders as well as some needed c4gs.
Other
4 stars 4 forks source link

Remove Forced Music Level in Tutorials #1

Closed exp111 closed 4 years ago

exp111 commented 5 years ago

Rather use the config value (which is used automatically on MusicSystem Init), because it may be too loud for some people (e.g. me).

maxmitti commented 5 years ago

I understand your concern, but I would prefer to just not allow scripts to enable music if it is disabled in the options. What do you think about that?

exp111 commented 5 years ago

Well that's also a good idea but I think it's a different feature. Why not implement both? (I guess your idea needs to be implemented in the main repo)

maxmitti commented 5 years ago

Yes, my idea has to be implemented in the engine. It is a bit a different feature, but solving your problem while also solving more problems. I would prefer to not remove the Music calls from the scripts because for those who want music it makes a difference.

exp111 commented 5 years ago

This pr doesn‘t remove the music but just removes the SetMusicLevel calls. You can still hear the music but at your volume (which you specify in the options).

maxmitti commented 5 years ago

Right, somehow I must have misread the diff the whole time, sorry. In this case I have another concern. I assume the music level is set to lower the music volume to make it easier to listen to the tutorial speaker. Although, the scripter assumed that the music volume is set to a higher value in the config.

So what about either limiting the effect of SetMusicLevel to the configured music volume or making the value relative to the configured volume?

I'd really prefer to fix it in the engine because in the wild we don't have control over what scripters come up with.

exp111 commented 5 years ago

That‘s a good idea. I‘ve looked into the code. https://github.com/legacyclonk/LegacyClonk/blob/9d2aa691869cdcfb41a9acd0935db38757a64bbb/src/C4Script.cpp#L2383 This is the FnMusicLevel, which gets called by the script.

It calls Game.SetMusicLevel (Which takes the Config value in account and sets the volume to that) and then sets the Volume and overrides the Game.SetMusicLevel one.

Game.SetMusicLevel: https://github.com/legacyclonk/LegacyClonk/blob/9d2aa691869cdcfb41a9acd0935db38757a64bbb/src/C4Game.cpp#L4154

Is that supposed to happen? The Game.SetMusicLevel seems reasonable ad you set the Volume to Level% * ConfigValue.

maxmitti commented 5 years ago

That‘s a good idea.

Which idea do you mean? Scaling the music level with the configured volume?

You are right, one of the two calls FnMusicLevel makes doesn't make any sense. Probably, removing the second call should lead to the desired behavior, possibly even matching the originally intended behavior.

I will try it out later, or if you are familiar with compiling the engine you can just try it too.

exp111 commented 5 years ago

I don‘t know which is better (clamping or relative level). Best to just try it out.

maxmitti commented 4 years ago

For now I made it scale according to the configured volume, as this seems to be the intended behavior. I haven't really tested it, though. If you like you can test it by using the engine from the current autobuild (https://github.com/legacyclonk/LegacyClonk/releases/download/continuous-master/LegacyClonk.zip assuming you are using Windows). It is sufficient to only use the executable from the zip. Be sure the backup the release engine in case you want to use the network mode, as the autobuild is not compatible with other versions.

exp111 commented 4 years ago

Works good. You can now understand the tutorial voice and such. Thanks.