[PATCH] A cleanup of SoundGroup XML files

[s0600204]

The XML files used to define sound groups could do with a light dust and a vacuum.

Firstly, many of the attribute tags set within the XML files redundantly match the default set for that attribute, as set in the c++ source. These tags may be safely removed under these circumstances. The current defaults can be found at the SoundGroup wiki page, or in source/soundmanager/scripting/SoundGroup.cpp

Secondly, the <replacement> tag is no longer used at all and can safely be removed. The mods/mod/audio/sound_group.rnc grammar definition and source/tools/entity/checkref.pl tool should be updated to follow.

Thirdly, on the c++ side of things, the example given in the block comment in SoundGroup.h should be updated to use the <path> tag.

---

To be considered: The <decay> and <threshold> tags are read into m_Decay and m_IntensityThreshold on the c++ side of things. Although space is reserved, defaults are provided and the values are read from XML, nothing is currently being done with these properties. They could be removed safely, but discussion on that is needed.

[otero]

The patch for this task is long but easy to read, as it's just changes in a few properties inside XML files. But, if you think it's better to divide the patch into multiple patches, please let me know.

[otero]

Patch for audio XML files

[elexis]

Thanks for your patch, looks good to go. Notice it addresses part 1 of the TODOs. The other ones should be easy to implement too though.

Someone with experience in soundgroups should also look if we should rather support those replacement-tags instead of removing them in their unused state.

The other TODOs in this ticket are simple to solve too.

---

Why the patch is correct (for the first TODO):

Those useless lines that overwrite defaults with defaults and the other useless lines that define something that is not being read should should be removed.

Using

cat audio_files.patch  | grep "^-" | grep -v "^---" | sort | uniq

one can see a short list of the changes you performed, removing duplicate lines. It immediately becomes visible that you only the mentioned default settings and the replacement tags:

-	<ConeInner>360</ConeInner>
-	<ConeOuter>360</ConeOuter>
-	<Decay>3</Decay>
-	<GainLower>0.8</GainLower>
-	<GainUpper>1</GainUpper>
-	<Pitch>1</Pitch>
-	<PitchLower>0.9</PitchLower>
-	<PitchUpper>1.1</PitchUpper>
-	<Priority>60</Priority>

XML files are loaded in CSoundGroup::LoadSoundGroup of SoundGroup.xml. The defaults are defined in CSoundGroup::SetDefaultValues:

	// sane defaults; will probably be replaced by the values read during LoadSoundGroup.
	SetGain(0.7f);
	m_Pitch = 1.0f;
	m_Priority = 60;
	m_PitchUpper = 1.1f;
	m_PitchLower = 0.9f;
	m_GainUpper = 1.0f;
	m_GainLower = 0.8f;
	m_ConeOuterGain = 0.0f;
	m_ConeInnerAngle = 360.0f;
	m_ConeOuterAngle = 360.0f;
	m_Decay = 3.0f;
	m_IntensityThreshold = 3;

This means you don't remove non-default values, which is good.

When checking for the other default values, I noticed in lumbeirng.xml there is one file that sets a default (m_ConeOuterGain):

	<ConeGain>0</ConeGain>

But it is not worth the removal since all other files define non-defaults.

Also you don't seem to be forgetting any other defaults, as neither <Gain>0.7 nor "<Threshold>3` appears in the xml files.

The removal of Replacement-tags is correct, since there is no reference to them in CSoundGroup::LoadSoundGroup and as far as i can see, only the SoundManager loads soundgroups.

[otero]

source/soundmanager/scripting/SoundGroup.h patch

[otero]

source/tools/entity/checkrefs.pl patch

[otero]

binaries/data/mods/mod/audio/sound_group.rnc patch

[otero]

Patch for audio XML files v2

[otero]

Fixed all the TODOs in the tickets and eliminated the tag ConeGain. Some patches seem to have more changes that what is necessary, the reason is that my text editor has eliminated a few extra spaces unnecessary in the source code and also I had to change a few tabs into spaces ( 4 spaces for each tab ) as it's a configuration that I have. Hope it doesn't generate any problems.

[elexis]

In 17576:

Remove trailing whitespace. Based on patch by otero, refs #3268.

[s0600204]

Looking through the patches, they look good to me.

However, attempting to apply SoundGroup_h_diff.patch locally fails. Could I request you regenerate it from on top of the current SVN state so as to take into account the changes made in r17576? (Although that commit was based on your patch, elexis included changes that are not in your patch, hence: merge conflicts.)

And I'd say that

• binaries/data/mods/public/audio/voice/latin/civ/civ_female_my_lord.xml
• binaries/data/mods/public/audio/voice/latin/civ/civ_male_my_lord.xml

both need a new line at the end of the files, but that is a small style issue that can be resolved by whoever commits this

[otero]

Fixed the conflict with the latest changes. Is the coding convention to use tabs instead spaces? I asked because I've uploaded another patch where I used the convention of 4 spaces instead of tabs, but I see that in the new changes are tabs what is used. I ask to know on future patches

[otero]

Why was this patch moved to Alpha 21?

[Itms]

Hi! Since we are getting close to another release, we pushed all pending work to the next milestone. That doesn't mean your patch is wrong or anything :)

It is possible that we delay the release, based on recent discussions. If that happens, you patch may be committed after another review.

Thanks for your work!

[otero]

Ah, Ok don't worry. I thought there was something missing in the patch and I was asking to know, so I could fix it.

[Itms]

In 17976:

Cleanup of SoundGroup XML files.

Remove some useless settings using the default values.
Remove the obsolete <Replacement> tag.
Update some documentation in the code, fix whitespace and unify line endings.

Patch by otero, fixes #3268

[Itms]

Thanks for that patch. I had to fix the sound files in the mod mod, where you forgot to do the changes you did elsewhere (especially removing the Replacement tag, which would have broken the XML validation). I also bumped the copyright year in the source file you touched.

Yes, the convention is indeed to use tabs (see wiki:Coding_Conventions). I added some line-ending unification in all the touched files.

Keep it up!