View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0002240 | FSSCP | sound | public | 2010-06-26 22:37 | 2010-07-22 04:07 |
| Reporter | The_E | Assigned To | iss_mneur | ||
| Priority | normal | Severity | major | Reproducibility | always |
| Status | resolved | Resolution | fixed | ||
| Product Version | 3.6.12 RC3 | ||||
| Summary | 0002240: sounds.tbl indices must be sequential | ||||
| Description | In 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. | ||||
| Tags | No tags attached. | ||||
|
|
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? |
|
|
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. |
|
|
This really should become a STL type to remove the need to separately track a num_game_sounds variable. |
|
|
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. |
|
|
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. |
|
|
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). |
|
|
Awesome. That would make FotG's sound guys very happy I think. |
|
|
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. |
|
2010-07-20 01:57
|
mantis_2240_v2.patch (12,673 bytes)
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
|
|
|
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. |
|
|
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 |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2010-06-26 22:37 | The_E | New Issue | |
| 2010-06-27 07:08 | Goober5000 | Note Added: 0012146 | |
| 2010-06-27 14:51 | The_E | Note Added: 0012148 | |
| 2010-07-10 12:08 | Echelon9 | Note Added: 0012215 | |
| 2010-07-19 05:14 | iss_mneur | Note Added: 0012239 | |
| 2010-07-19 05:14 | iss_mneur | Assigned To | => iss_mneur |
| 2010-07-19 05:14 | iss_mneur | Status | new => feedback |
| 2010-07-19 05:16 | iss_mneur | File Added: mantis_2240.patch | |
| 2010-07-19 16:28 | chief1983 | Note Added: 0012240 | |
| 2010-07-19 16:41 | iss_mneur | Note Added: 0012242 | |
| 2010-07-19 16:50 | chief1983 | Note Added: 0012243 | |
| 2010-07-20 01:56 | iss_mneur | Note Added: 0012249 | |
| 2010-07-20 01:57 | iss_mneur | File Added: mantis_2240_v2.patch | |
| 2010-07-20 01:57 | iss_mneur | File Deleted: mantis_2240.patch | |
| 2010-07-22 02:28 | iss_mneur | Note Added: 0012252 | |
| 2010-07-22 02:28 | iss_mneur | Status | feedback => resolved |
| 2010-07-22 02:28 | iss_mneur | Resolution | open => fixed |
| 2010-07-22 04:07 | chief1983 | Note Added: 0012253 |
