2020-07-15 00:11 EDT


View Issue Details Jump to Notes ]
IDProjectCategoryView StatusLast Update
0002240FSSCPsoundpublic2010-07-22 00:07
ReporterThe_E 
Assigned Toiss_mneur 
PrioritynormalSeveritymajorReproducibilityalways
StatusresolvedResolutionfixed 
Product Version3.6.12 RC3 
Target VersionFixed in Version 
Summary0002240: sounds.tbl indices must be sequential
DescriptionIn order to work around an issue with a hardcoded sound index, Fury assigned indices starting with 300 for our custom sounds. This leads to a parsing error at gamesnd.cpp, line 81, as the game uses num_game_sounds to check for an entries' validity. This works, as long as the entries are in sequential order, but breaks down once a gap is introduced. This seems pretty bad to me, as it introduces yet another instance of undocumented magic-number type behaviour.
TagsNo tags attached.
Attached Files
  • patch file icon mantis_2240_v2.patch (12,673 bytes) 2010-07-19 21:57 -
    Index: code/gamesnd/gamesnd.cpp
    ===================================================================
    --- code/gamesnd/gamesnd.cpp	(revision 6294)
    +++ code/gamesnd/gamesnd.cpp	(working copy)
    @@ -16,12 +16,10 @@
     #include "sound/ds.h"
     
     
    -int Num_game_sounds = 0;
    -game_snd *Snds = NULL;
    +SCP_vector<game_snd> Snds;
     
    -int Num_iface_sounds = 0;
    -game_snd *Snds_iface = NULL;
    -int *Snds_iface_handle = NULL;
    +SCP_vector<game_snd> Snds_iface;
    +SCP_vector<int> Snds_iface_handle;
     
     #define GAME_SND	0
     #define IFACE_SND	1
    @@ -39,7 +37,8 @@
     //WMC - now ignores file extension.
     int gamesnd_get_by_name(char* name)
     {
    -	for(int i = 0; i < Num_game_sounds; i++)
    +	Assert( Snds.size() <= INT_MAX );
    +	for(int i = 0; i < (int)Snds.size(); i++)
     	{
     		char *p = strrchr( Snds[i].filename, '.' );
     		if(p == NULL)
    @@ -57,6 +56,31 @@
     	return -1;
     }
     
    +int gamesnd_get_by_tbl_index(int index)
    +{
    +	Assert( Snds.size() <= INT_MAX );
    +	for(int i = 0; i < (int)Snds.size(); i++) {
    +		if ( Snds[i].sig == index )
    +		{
    +			return i;
    +		}
    +	}
    +	return -1;
    +}
    +
    +int gamesnd_get_by_iface_tbl_index(int index)
    +{
    +	Assert( Snds_iface.size() <= INT_MAX );
    +	Assert( Snds_iface.size() == Snds_iface_handle.size() );
    +	for(int i = 0; i < (int)Snds_iface.size(); i++) {
    +		if ( Snds_iface[i].sig == index )
    +		{
    +			return i;
    +		}
    +	}
    +	return -1;
    +}
    +
     //Takes a tag, a sound index destination, and the object name being parsed
     //tag and object_name are mostly so that we can debug stuff
     //This also means you shouldn't use optional_string or required_string,
    @@ -74,12 +98,18 @@
     		if(idx != -1)
     			(*idx_dest) = idx;
     		else
    -			(*idx_dest) = atoi(buf);
    +		{
    +			idx = gamesnd_get_by_tbl_index(atoi(buf));
    +			if (idx != -1)
    +				(*idx_dest) = idx;
    +		}
     
    +		Assert( Snds.size() <= INT_MAX );
     		//Ensure sound is in range
    -		if((*idx_dest) < -1 || (*idx_dest) >= Num_game_sounds)
    +		if((*idx_dest) < -1 || (*idx_dest) >= (int)Snds.size())
     		{
    -			Warning(LOCATION, "%s sound index out of range on '%s'. Must be between 0 and %d. Forcing to -1 (Nonexistant sound).\n", tag, object_name, Num_game_sounds);
    +			(*idx_dest) = -1;
    +			Warning(LOCATION, "%s sound index out of range on '%s'. Must be between 0 and %d. Forcing to -1 (Nonexistant sound).\n", tag, object_name, Snds.size());
     		}
     	}
     }
    @@ -97,7 +127,8 @@
     	if ( !Sound_enabled )
     		return;
     
    -	for ( i = 0; i < Num_game_sounds; i++ ) {
    +	Assert( Snds.size() <= INT_MAX );
    +	for ( i = 0; i < (int)Snds.size(); i++ ) {
     		gs = &Snds[i];
     		if ( gs->filename[0] != 0 && strnicmp(gs->filename, NOX("none.wav"), 4) ) {
     			if ( gs->preload ) {
    @@ -121,7 +152,8 @@
     	if ( !Sound_enabled )
     		return;
     
    -	for ( i = 0; i < Num_game_sounds; i++ ) {
    +	Assert( Snds.size() <= INT_MAX );
    +	for ( i = 0; i < (int)Snds.size(); i++ ) {
     		gs = &Snds[i];
     		if ( gs->filename[0] != 0 && strnicmp(gs->filename, NOX("none.wav"), 4) ) {
     			if ( !gs->preload ) { // don't try to load anything that's already preloaded
    @@ -142,7 +174,8 @@
     	int		i;
     	game_snd	*gs;
     
    -	for ( i = 0; i < Num_game_sounds; i++ ) {
    +	Assert( Snds.size() <= INT_MAX );
    +	for ( i = 0; i < (int)Snds.size(); i++ ) {
     		gs = &Snds[i];
     		if ( gs->id != -1 ) {
     			snd_unload( gs->id );
    @@ -164,7 +197,8 @@
     	if ( !Sound_enabled )
     		return;
     
    -	for ( i = 0; i < Num_iface_sounds; i++ ) {
    +	Assert( Snds_iface.size() < INT_MAX );
    +	for ( i = 0; i < (int)Snds_iface.size(); i++ ) {
     		gs = &Snds_iface[i];
     		if ( gs->filename[0] != 0 && strnicmp(gs->filename, NOX("none.wav"), 4) ) {
     			gs->id = snd_load(gs);
    @@ -182,7 +216,8 @@
     	int		i;
     	game_snd	*gs;
     
    -	for ( i = 0; i < Num_iface_sounds; i++ ) {
    +	Assert( Snds_iface.size() < INT_MAX );
    +	for ( i = 0; i < (int)Snds_iface.size(); i++ ) {
     		gs = &Snds_iface[i];
     		if ( gs->id != -1 ) {
     			snd_unload( gs->id );
    @@ -256,7 +291,8 @@
     	// Parse the gameplay sounds section
     	required_string("#Game Sounds Start");
     	while (required_string_either("#Game Sounds End","$Name:")) {
    -		Assert( num_game_sounds < Num_game_sounds);
    +		Assert( Snds.size() < INT_MAX );
    +		Assert( num_game_sounds < (int)Snds.size() );
     		gamesnd_parse_line( &Snds[num_game_sounds], "$Name:" );
     		num_game_sounds++;
     		gamesnd_add_sound_slot( GAME_SND, num_game_sounds );
    @@ -266,7 +302,8 @@
     	// Parse the interface sounds section
     	required_string("#Interface Sounds Start");
     	while (required_string_either("#Interface Sounds End","$Name:")) {
    -		Assert( num_iface_sounds < Num_iface_sounds);
    +		Assert( Snds_iface_handle.size() < INT_MAX );
    +		Assert( num_iface_sounds < (int)Snds_iface.size() );
     		gamesnd_parse_line(&Snds_iface[num_iface_sounds], "$Name:");
     		num_iface_sounds++;
     		gamesnd_add_sound_slot( IFACE_SND, num_iface_sounds );
    @@ -467,6 +504,7 @@
     //
     void gamesnd_init_struct(game_snd *gs)
     {
    +	gs->sig = -1;
     	gs->filename[0] = 0;
     	gs->id = -1;
     	gs->id_sig = -1;
    @@ -482,33 +520,26 @@
     {
     	int		i;
     
    -	if (Snds == NULL) {
    -		Snds = (game_snd *) vm_malloc (sizeof(game_snd) * MIN_GAME_SOUNDS);
    -		Verify( Snds != NULL );
    -		Num_game_sounds = MIN_GAME_SOUNDS;
    -	}
    +	Snds.clear();
    +	Snds.resize(MIN_GAME_SOUNDS);
     
    -	Assert( Num_game_sounds > 0 );
    +	Assert( Snds.size() > 0 );
     
    +	Assert( Snds.size() <= INT_MAX );
     	// init the gameplay sounds
    -	for ( i = 0; i < Num_game_sounds; i++ ) {
    +	for ( i = 0; i < (int)Snds.size(); i++ ) {
     		gamesnd_init_struct(&Snds[i]);
     	}
     
    -	if (Snds_iface == NULL) {
    -		Snds_iface = (game_snd *) vm_malloc (sizeof(game_snd) * MIN_INTERFACE_SOUNDS);
    -		Verify( Snds_iface != NULL );
    -		Num_iface_sounds = MIN_INTERFACE_SOUNDS;
    +	Snds_iface.clear();
    +	Snds_iface.resize(MIN_INTERFACE_SOUNDS);
    +	Snds_iface_handle.resize(MIN_INTERFACE_SOUNDS);
    +	
    +	Assert( Snds_iface.size() > 0 );
     
    -		Assert( Snds_iface_handle == NULL );
    -		Snds_iface_handle = (int *) vm_malloc (sizeof(int) * Num_iface_sounds);
    -		Verify( Snds_iface_handle != NULL );
    -	}
    -
    -	Assert( Num_iface_sounds > 0 );
    -
    +	Assert( Snds_iface.size() < INT_MAX );
     	// init the interface sounds
    -	for ( i = 0; i < Num_iface_sounds; i++ ) {
    +	for ( i = 0; i < (int)Snds_iface.size(); i++ ) {
     		gamesnd_init_struct(&Snds_iface[i]);
     		Snds_iface_handle[i] = -1;
     	}
    @@ -517,20 +548,9 @@
     // close out gamesnd,  ONLY CALL FROM game_shutdown()!!!!
     void gamesnd_close()
     {
    -	if (Snds != NULL) {
    -		vm_free(Snds);
    -		Snds = NULL;
    -	}
    -
    -	if (Snds_iface != NULL) {
    -		vm_free(Snds_iface);
    -		Snds_iface = NULL;
    -	}
    -
    -	if (Snds_iface_handle != NULL) {
    -		vm_free(Snds_iface_handle);
    -		Snds_iface_handle = NULL;
    -	}
    +	Snds.clear();
    +	Snds_iface.clear();
    +	Snds_iface_handle.clear();
     }
     
     // callback function for the UI code to call when the mouse first goes over a button.
    @@ -552,16 +572,14 @@
     	switch (type) {
     		case GAME_SND:
     		{
    -			Assert( Snds != NULL );
    -			Assert( num < (Num_game_sounds + increase_by) );
    +			Assert( Snds.size() <= INT_MAX );
    +			Assert( num < ((int)Snds.size() + increase_by) );
     
    -			if (num >= Num_game_sounds) {
    -				Snds = (game_snd *) vm_realloc (Snds, sizeof(game_snd) * (Num_game_sounds + increase_by));
    -				Verify( Snds != NULL );
    -				Num_game_sounds += increase_by;
    +			if (num >= (int)Snds.size()) {
    +				Snds.resize(Snds.size() + increase_by);
     
     				// default all new entries
    -				for (i = (Num_game_sounds - increase_by); i < Num_game_sounds; i++) {
    +				for (i = ((int)Snds.size() - increase_by); i < (int)Snds.size(); i++) {
     					gamesnd_init_struct(&Snds[i]);
     				}
     			}
    @@ -570,20 +588,17 @@
     
     		case IFACE_SND:
     		{
    -			Assert( Snds_iface != NULL );
    -			Assert( num < (Num_game_sounds + increase_by) );
    +			Assert( Snds_iface.size() < INT_MAX );
    +			Assert( num < ((int)Snds_iface.size() + increase_by) );
     
    -			if (num >= Num_iface_sounds) {
    -				Snds_iface = (game_snd *) vm_realloc (Snds_iface, sizeof(game_snd) * (Num_iface_sounds + increase_by));
    -				Verify( Snds_iface != NULL );
    -				Num_iface_sounds += increase_by;
    +			if (num >= (int)Snds_iface.size()) {
    +				Snds_iface.resize(Snds_iface.size() + increase_by);
     
    -				Assert( Snds_iface_handle != NULL );
    -				Snds_iface_handle = (int *) vm_realloc (Snds_iface_handle, sizeof(int) * Num_iface_sounds);
    -				Verify( Snds_iface_handle != NULL );
    +				Snds_iface_handle.resize(Snds_iface.size());
    +				Assert( Snds_iface.size() < INT_MAX );
     
     				// default all new entries
    -				for (i = (Num_iface_sounds - increase_by); i < Num_iface_sounds; i++) {
    +				for (i = ((int)Snds_iface_handle.size() - increase_by); i < (int)Snds_iface_handle.size(); i++) {
     					gamesnd_init_struct(&Snds_iface[i]);
     					Snds_iface_handle[i] = -1;
     				}
    Index: code/gamesnd/gamesnd.h
    ===================================================================
    --- code/gamesnd/gamesnd.h	(revision 6294)
    +++ code/gamesnd/gamesnd.h	(working copy)
    @@ -27,6 +27,8 @@
     void gamesnd_play_iface(int n);
     void gamesnd_play_error_beep();
     int gamesnd_get_by_name(char* name);
    +int gamesnd_get_by_tbl_index(int index);
    +int gamesnd_get_by_iface_tbl_index(int index);
     
     //This should handle NO_SOUND just fine since it doesn't directly access lowlevel code
     //Does all parsing for a sound
    @@ -40,13 +42,10 @@
     #define MIN_GAME_SOUNDS					202
     #define MIN_INTERFACE_SOUNDS			70
     
    -extern int Num_game_sounds;
    -extern int Num_iface_sounds;
    +extern SCP_vector<game_snd> Snds;
    +extern SCP_vector<game_snd> Snds_iface;
     
    -extern game_snd *Snds;
    -extern game_snd *Snds_iface;
     
    -
     // symbolic names for misc. game sounds.  The order here must match the order in
     // sounds.tbl
     //
    Index: code/parse/lua.cpp
    ===================================================================
    --- code/parse/lua.cpp	(revision 6294)
    +++ code/parse/lua.cpp	(working copy)
    @@ -8149,7 +8149,7 @@
     	if(vol > 100.0f)
     		vol = 100.0f;
     
    -	idx = snd_play(&Snds[idx], pan, vol*0.01f, pri, voice_msg);
    +	idx = snd_play(&Snds[gamesnd_get_by_tbl_index(idx)], pan, vol*0.01f, pri, voice_msg);
     
     	return ade_set_args(L, "b", idx > -1);
     }
    @@ -8160,7 +8160,7 @@
     	if(!ade_get_args(L, "i", &idx))
     		return ade_set_error(L, "b", false);
     
    -	gamesnd_play_iface(idx);
    +	gamesnd_play_iface(gamesnd_get_by_iface_tbl_index(idx));
     
     	return ade_set_args(L, "b", idx > -1);
     }
    Index: code/parse/sexp.cpp
    ===================================================================
    --- code/parse/sexp.cpp	(revision 6294)
    +++ code/parse/sexp.cpp	(working copy)
    @@ -8615,7 +8615,7 @@
     
     	// play sound effect ---------------------------
     	if (sound_index >= 0) {
    -		snd_play_3d( &Snds[sound_index], &origin, &View_position, 0.0f, NULL, 0, 1.0f, SND_PRIORITY_MUST_PLAY  );
    +		snd_play_3d( &Snds[gamesnd_get_by_tbl_index(sound_index)], &origin, &View_position, 0.0f, NULL, 0, 1.0f, SND_PRIORITY_MUST_PLAY  );
     	}
     
     	if (MULTIPLAYER_MASTER) {
    @@ -8638,9 +8638,10 @@
     	multi_get_float(origin.xyz.z);
     	multi_get_int(sound_index);
     
    +
     	if (sound_index != -1) {
     		// play sound effect ---------------------------
    -		snd_play_3d( &Snds[sound_index], &origin, &View_position, 0.0f, NULL, 0, 1.0f, SND_PRIORITY_MUST_PLAY  );
    +		snd_play_3d( &Snds[gamesnd_get_by_tbl_index(sound_index)], &origin, &View_position, 0.0f, NULL, 0, 1.0f, SND_PRIORITY_MUST_PLAY  );
     	}
     }
     
    Index: code/sound/sound.cpp
    ===================================================================
    --- code/sound/sound.cpp	(revision 6294)
    +++ code/sound/sound.cpp	(working copy)
    @@ -14,6 +14,7 @@
     #include "sound/audiostr.h"
     #include "cmdline/cmdline.h"
     #include "osapi/osapi.h"
    +#include "globalincs/vmallocator.h"
     
     #include "gamesnd/gamesnd.h"
     #include "globalincs/alphacolors.h"
    @@ -180,7 +181,7 @@
     	int message_sounds = 0;
     	int interface_sounds = 0;
     	int done = 0;
    -	int s_idx;
    +	size_t s_idx;
     
     	if(!Sound_spew){
     		return;
    @@ -195,7 +196,7 @@
     		done = 0;
     
     		// what kind of sound is this
    -		for(s_idx=0; s_idx<Num_game_sounds; s_idx++){
    +		for(s_idx=0; s_idx < Snds.size(); s_idx++){
     			if(!stricmp(Snds[s_idx].filename, Sounds[idx].filename)){
     				game_sounds++;
     				done = 1;
    @@ -203,7 +204,7 @@
     		}
     
     		if(!done){
    -			for(s_idx=0; s_idx<Num_game_sounds; s_idx++){
    +			for(s_idx=0; s_idx < Snds.size(); s_idx++){
     				if(!stricmp(Snds_iface[s_idx].filename, Sounds[idx].filename)){
     					interface_sounds++;
     					done = 1;
    Index: code/sound/sound.h
    ===================================================================
    --- code/sound/sound.h	(revision 6294)
    +++ code/sound/sound.h	(working copy)
    @@ -30,7 +30,7 @@
     
     typedef struct game_snd
     {
    -	int	sig;						// number of sound in sounds.tbl (not used)
    +	int	sig;						// index number of sound in as specified in sounds.tbl
     	char	filename[MAX_FILENAME_LEN];
     	float	default_volume;		// range: 0.0 -> 1.0
     	int	min, max;				// min: distance at which sound will stop getting louder  max: distance at which sound is inaudible
    
    patch file icon mantis_2240_v2.patch (12,673 bytes) 2010-07-19 21:57 +

-Relationships
+Relationships

-Notes

~0012146

Goober5000 (administrator)

Why, exactly, is this a problem? This isn't magic number behavior, it's violating the game's assumption that the sounds listing is continuous.

And what was the issue that caused the sounds to begin at 300?

~0012148

The_E (administrator)

Well, Fury put the sounds there to avoid any collisions between BP's sounds and FSO's fixed indices. The problem was that this behaviour wasn't documented anywhere (which I have fixed).
Still, if at some point someone wants to tbm-ify this tbl, provisions have to be made for non-continuous indices, IMHO.

~0012215

Echelon9 (developer)

This really should become a STL type to remove the need to separately track a num_game_sounds variable.

~0012239

iss_mneur (developer)

I have written a patch that changes the Snds, Snds_iface, and Snds_iface_handle to be SCP_vectors. It turns the sounds number into just a magic number that refers to the sound that is needed, which can be used anywhere.

The attached patch does not deal with changes to the set of hardcoded sound indexes that the engine uses. That is, sounds in sounds.tbl must be in the same contiguous order that they are now up to the last hardcoded index (thats 201 for general sounds, and 61 for interface sounds). After that the numbers do not have to be contiguous.

I realize that is this not the most complete solution to the problem, but it does remove the necessity of the entries after the hardcoded entries being in sequential order.

~0012240

chief1983 (administrator)

The general solution to this was just to fill in the gaps with empty entries that can be filled in later, that's what FotG was doing. It's working for the short term at least.

~0012242

iss_mneur (developer)

Yes, I realize that. This patch removes the need to do that for any sound.tbl entry after the Hardcoded entries (201 for general sounds, 61 for interface sounds).

~0012243

chief1983 (administrator)

Awesome. That would make FotG's sound guys very happy I think.

~0012249

iss_mneur (developer)

I have attached another version of the patch that deals with some issues in lua.cpp and sexp.cpp to deal with any cases where a tbler can use an index that are not part of the contiguous set from the retail sounds.tbl.

~0012252

iss_mneur (developer)

Setting ticket to resolved, as primary issue that prompted the bug has been fixed with the second patch provided. As the second patch has not caused further issues it looks to be a satisfactory resolution.

~0012253

chief1983 (administrator)

I would normally have recommended waiting to resolve until the RC gets thorough testing, but this is recent enough I'm not likely to forget about it :P
+Notes

-Issue History
Date Modified Username Field Change
2010-06-26 18:37 The_E New Issue
2010-06-27 03:08 Goober5000 Note Added: 0012146
2010-06-27 10:51 The_E Note Added: 0012148
2010-07-10 08:08 Echelon9 Note Added: 0012215
2010-07-19 01:14 iss_mneur Note Added: 0012239
2010-07-19 01:14 iss_mneur Assigned To => iss_mneur
2010-07-19 01:14 iss_mneur Status new => feedback
2010-07-19 01:16 iss_mneur File Added: mantis_2240.patch
2010-07-19 12:28 chief1983 Note Added: 0012240
2010-07-19 12:41 iss_mneur Note Added: 0012242
2010-07-19 12:50 chief1983 Note Added: 0012243
2010-07-19 21:56 iss_mneur Note Added: 0012249
2010-07-19 21:57 iss_mneur File Added: mantis_2240_v2.patch
2010-07-19 21:57 iss_mneur File Deleted: mantis_2240.patch
2010-07-21 22:28 iss_mneur Note Added: 0012252
2010-07-21 22:28 iss_mneur Status feedback => resolved
2010-07-21 22:28 iss_mneur Resolution open => fixed
2010-07-22 00:07 chief1983 Note Added: 0012253
+Issue History