[Sushi]

Getting a crash here. Debug yielded some interesting errors. Log attached.

The crash happens when I switch campaigns/mainhalls or go to the ready room and back out. I'm guessing it'll happen with other game state changes as well. Interestingly the first mainhall loads fine, it's when the game is reloading the mainhall that seems to cause the issue.

the **** does this flag do? " -disable_di_mouse" (not related to problem, just curious)

http://www.hard-light.net/forums/index.php?topic=73511.msg1450702#msg1450702

By default, FSO uses DirectInput for handling the mouse movements. At the time I added the flag, the default mouse behavior made the mouse extremely jumpy (especially noticeable when using the freelancer-style scripted mouse). The flag just turns on the "old" code for mouse handling that makes the experience smoother for some users.

[pecenipicek]

Getting a crash here. Debug yielded some interesting errors. Log attached.

The crash happens when I switch campaigns/mainhalls or go to the ready room and back out. I'm guessing it'll happen with other game state changes as well. Interestingly the first mainhall loads fine, it's when the game is reloading the mainhall that seems to cause the issue.

the **** does this flag do? " -disable_di_mouse" (not related to problem, just curious)

http://www.hard-light.net/forums/index.php?topic=73511.msg1450702#msg1450702

By default, FSO uses DirectInput for handling the mouse movements. At the time I added the flag, the default mouse behavior made the mouse extremely jumpy (especially noticeable when using the freelancer-style scripted mouse). The flag just turns on the "old" code for mouse handling that makes the experience smoother for some users.

topic or board inaccessible. i am asking because i didnt find any notice of that on the wiki.

[Iss Mneur]

The warning %s in '%s' is not an empty list! Its values will be overwritten!, should be an Error so that we trap even in release.

The warning %s in '%s' has %i entries. This does not match entered size of %i. may make more sense as just an mprintf/nprintf.

Code: [Select]

@@ -1182,7 +1182,7 @@
  }
  // animation is not paused
  else {
- for (s_idx = Main_hall->misc_anim_special_sounds[idx][0]; s_idx > 0; s_idx--) {
+ for (s_idx = Main_hall->misc_anim_special_sounds.at(idx).size(); s_idx > 0; s_idx--) {
  // if we've passed the trigger point, then play the sound and break out of the loop
  if ((Main_hall_misc_anim[idx].current_frame >= Main_hall->misc_anim_special_trigger[idx][s_idx]) && !Main_hall->misc_anim_sound_flag[idx][s_idx]) {
  Main_hall->misc_anim_sound_flag[idx][s_idx] = 1;
I haven't check the type of s_idx, but I think that will create a warning because you are losing precession (size_t to int).

Code: [Select]

+ SCP_vector<int> temp; //put *something* in the vector so we don't cause an exception when calling parse_sound_list()
+ m->misc_anim_special_sounds.push_back(temp);
This concerns me because it should not be necessary.  What throws the exception?


May I suggest using flags rather than a bunch of bools to set the options.

Code: [Select]

enum parse_sound_flags {
PARSE_SOUND_GENERAL_SOUND = 0, //!< search for sound in the general table in sound.tbl
PARSE_SOUND_INTERFACE_SOUND = 1 << 0, //!< Search for sound in the interface sound table
PARSE_SOUND_SCP_SOUND_LIST = 1 << 1, //!< Parse the list of sounds SCP style (just indexes and/or files names, no count first)
PARSE_SOUND_MAX
};


Code: [Select]

void parse_sound_core(char* tag, int *idx_dest, char* object_name, char* buf, parse_sound_flags flags)
{
        Assertion(flags < PARSE_SOUND_MAX, "Unknown flag set. (%x)", flags);
if(flags & PARSE_SOUND_INTERFACE_SOUND)
idx = gamesnd_get_by_iface_name(buf);
else
idx = gamesnd_get_by_name(buf);
.....
}


Code: [Select]

void parse_sound_list(char* tag, SCP_vector<int>& destination, char* object_name, parse_sound_flags flags)
{
....
}


Code: [Select]

parse_sound_list("+Misc anim sounds:",
        m->misc_anim_special_sounds.at(idx),
        "+Misc anim sounds:",
        PARSE_SOUND_INTERFACE_SOUND);

parse_sound_list("+Misc anim sounds new:",
        m->misc_anim_special_sounds.at(idx),
        "+Misc anim sounds:",
        PARSE_SOUND_INTERFACE_SOUND | PARSE_SOUND_SCP_SOUND_LIST);

That way when you read the code you are not looking at a string of meaningless words, and then have to go and look up what each parameter is to know what it is supposedly doing.

Otherwise it is looking good.

[Sushi]

Getting a crash here. Debug yielded some interesting errors. Log attached.

The crash happens when I switch campaigns/mainhalls or go to the ready room and back out. I'm guessing it'll happen with other game state changes as well. Interestingly the first mainhall loads fine, it's when the game is reloading the mainhall that seems to cause the issue.

the **** does this flag do? " -disable_di_mouse" (not related to problem, just curious)

http://www.hard-light.net/forums/index.php?topic=73511.msg1450702#msg1450702

By default, FSO uses DirectInput for handling the mouse movements. At the time I added the flag, the default mouse behavior made the mouse extremely jumpy (especially noticeable when using the freelancer-style scripted mouse). The flag just turns on the "old" code for mouse handling that makes the experience smoother for some users.

topic or board inaccessible. i am asking because i didnt find any notice of that on the wiki.

Oops, forgot that was an SCP internal thread. Not really any more information there than I just posted, though.

It probably should be on the wiki, I've just been fail at documenting stuff lately. 

[CommanderDJ]

The warning %s in '%s' is not an empty list! Its values will be overwritten!, should be an Error so that we trap even in release.

After more testing I'm not even sure we should check for this at all. When re-entering the same mainhall, if it is reparsed again (default behaviour for now), the same lists are filled and so they won't be empty in a completely legitimate case.

EDIT: This turned out to be the warning that MjnMixael was getting when mainhalls were being reparsed completely legitimately, so the next patch will have this removed.

The warning %s in '%s' has %i entries. This does not match entered size of %i. may make more sense as just an mprintf/nprintf.

This I have changed.

Code: [Select]

@@ -1182,7 +1182,7 @@
  }
  // animation is not paused
  else {
- for (s_idx = Main_hall->misc_anim_special_sounds[idx][0]; s_idx > 0; s_idx--) {
+ for (s_idx = Main_hall->misc_anim_special_sounds.at(idx).size(); s_idx > 0; s_idx--) {
  // if we've passed the trigger point, then play the sound and break out of the loop
  if ((Main_hall_misc_anim[idx].current_frame >= Main_hall->misc_anim_special_trigger[idx][s_idx]) && !Main_hall->misc_anim_sound_flag[idx][s_idx]) {
  Main_hall->misc_anim_sound_flag[idx][s_idx] = 1;
I haven't check the type of s_idx, but I think that will create a warning because you are losing precession (size_t to int).

I've put in a cast to prevent this.

May I suggest using flags rather than a bunch of bools to set the options.

Also rectified.

Code: [Select]

+ SCP_vector<int> temp; //put *something* in the vector so we don't cause an exception when calling parse_sound_list()
+ m->misc_anim_special_sounds.push_back(temp);
This concerns me because it should not be necessary.  What throws the exception?

It concerns me too. In the following line (note the syntax is slightly different due to the addition of flags),

Code: [Select]

parse_sound_list("+Misc anim sounds:", m->misc_anim_special_sounds.at(idx), "+Misc anim sounds:", PARSE_SOUND_INTERFACE_SOUND);
an std::out_of_range exception is thrown when calling parse_sound_list if misc_anim_special_sounds is empty. It kind of makes sense since we're passing a reference to a vector element that doesn't exist, but I too thought it wouldn't be a problem.

Getting a crash here. Debug yielded some interesting errors. Log attached.

The crash happens when I switch campaigns/mainhalls or go to the ready room and back out. I'm guessing it'll happen with other game state changes as well. Interestingly the first mainhall loads fine, it's when the game is reloading the mainhall that seems to cause the issue.

I have been unable to reproduce it so far, but I will keep investigating as I'm also getting crashes, just not in the same circumstances.

[CommanderDJ]

Another update. This addresses some misplaced warnings, fixes a bunch of crashes, and adds flags instead of boolean parameters for the sound functions. Testing is required!

Code: [Select]

Index: code/gamesnd/gamesnd.cpp
===================================================================
--- code/gamesnd/gamesnd.cpp (revision 8068)
+++ code/gamesnd/gamesnd.cpp (working copy)
@@ -52,6 +52,30 @@
  return -1;
 }
 
+int gamesnd_get_by_iface_name(char* name)
+{
+ Assert( Snds_iface.size() <= INT_MAX );
+ Assert( Snds_iface.size() == Snds_iface_handle.size() );
+ int i = 0;
+ for(SCP_vector<game_snd>::iterator snd = Snds_iface.begin(); snd != Snds_iface.end(); ++snd)
+ {
+ char *p = strrchr( snd->filename, '.' );
+ if(p == NULL)
+ {
+ if(!stricmp(snd->filename, name))
+ {
+ return i;
+ }
+ }
+ else if(!strnicmp(snd->filename, name, p-snd->filename))
+ {
+ return i;
+ }
+ i++;
+ }
+ return -1;
+}
+
 int gamesnd_get_by_tbl_index(int index)
 {
  //if we get passed -1, don't bother trying to look it up.
@@ -71,6 +95,9 @@
 
 int gamesnd_get_by_iface_tbl_index(int index)
 {
+ //if we get passed -1, don't bother trying to look it up.
+ if (index == -1)
+ return -1;
  Assert( Snds_iface.size() <= INT_MAX );
  Assert( Snds_iface.size() == Snds_iface_handle.size() );
  int i = 0;
@@ -85,41 +112,128 @@
 }
 
 /**
- * Parse a sound
- *
+ * Helper function for parse_sound and parse_sound_list. Do not use directly.
+ *
  * @param tag Tag
  * @param idx_dest Sound index destination
  * @param object_name Object name being parsed
+ * @param buf Buffer holding string to be parsed
+ * @param flags See the parse_sound_flags enum
  *
- * This also means you shouldn't use optional_string or required_string,
- * just make sure the destination sound index can handle -1 if things
- * don't work out.
  */
-void parse_sound(char* tag, int *idx_dest, char* object_name)
+void parse_sound_core(char* tag, int *idx_dest, char* object_name, char* buf, parse_sound_flags flags)
 {
- char buf[MAX_FILENAME_LEN];
  int idx;
 
+ if(flags & PARSE_SOUND_INTERFACE_SOUND)
+ idx = gamesnd_get_by_iface_name(buf);
+ else
+ idx = gamesnd_get_by_name(buf);
+
+ if(idx != -1)
+ {
+ (*idx_dest) = idx;
+ }
+ else
+ {
+ if(flags & PARSE_SOUND_INTERFACE_SOUND)
+ idx = gamesnd_get_by_iface_tbl_index(atoi(buf));
+ else
+ idx = gamesnd_get_by_tbl_index(atoi(buf));
+
+ if (idx != -1)
+ (*idx_dest) = idx;
+ }
+
+ int size_to_check = 0;
+
+ if(flags & PARSE_SOUND_INTERFACE_SOUND)
+ {
+ size_to_check = Snds_iface.size();
+ Assert( Snds_iface.size() == Snds_iface_handle.size() );
+ }
+ else
+ {
+ size_to_check = Snds.size();
+ }
+
+ Assert( size_to_check <= INT_MAX );
+
+ //Ensure sound is in range
+ if((*idx_dest) < -1 || (*idx_dest) >= (int)size_to_check)
+ {
+ (*idx_dest) = -1;
+ Warning(LOCATION, "%s sound index out of range on '%s'. Must be between 0 and %d. Forcing to -1 (Nonexistent sound).\n", tag, object_name, size_to_check);
+ }
+}
+
+/**
+ * Parse a sound. When using this function for a table entry,
+ * required_string and optional_string aren't needed, as this function deals with
+ * that as its tag parameter, just make sure that the destination sound index can
+ * handle -1 if things don't work out.
+ *
+ * @param tag Tag
+ * @param idx_dest Sound index destination
+ * @param object_name Object name being parsed
+ * @param flags See the parse_sound_flags enum
+ *
+ */
+void parse_sound(char* tag, int *idx_dest, char* object_name, parse_sound_flags flags)
+{
  if(optional_string(tag))
  {
+ char buf[MAX_FILENAME_LEN];
  stuff_string(buf, F_NAME, MAX_FILENAME_LEN);
- idx = gamesnd_get_by_name(buf);
- if(idx != -1)
- (*idx_dest) = idx;
- else
+
+ parse_sound_core(tag, idx_dest, object_name, buf, flags);
+ }
+}
+
+/**
+ * CommanderDJ - Parse a list of sounds. When using this function for a table entry,
+ * required_string and optional_string aren't needed, as this function deals with
+ * that as its tag parameter, just make sure that the destination sound index(es) can
+ * handle -1 if things don't work out.
+ *
+ * @param destination Vector where sound indexes are to be stored
+ * @param tag Tag
+ * @param object_name Name of object being parsed
+ * @param flags See the parse_sound_flags enum
+ *
+ */
+void parse_sound_list(char* tag, SCP_vector<int>& destination, char* object_name, parse_sound_flags flags)
+{
+ if(optional_string(tag))
+ {
+ int check=0;
+
+ //if we're using the old format, parse the first entry separately
+ if(!(flags & PARSE_SOUND_SCP_SOUND_LIST))
  {
- idx = gamesnd_get_by_tbl_index(atoi(buf));
- if (idx != -1)
- (*idx_dest) = idx;
+ stuff_int(&check);
  }
 
- Assert( Snds.size() <= INT_MAX );
- //Ensure sound is in range
- if((*idx_dest) < -1 || (*idx_dest) >= (int)Snds.size())
+ //now read the rest of the entries on the line
+ for(int i=0;!check_for_eoln();i++)
  {
- (*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());
+ char buf[MAX_FILENAME_LEN];
+ stuff_string_white(buf, MAX_FILENAME_LEN);
+
+ //we do this conditionally to avoid adding needless entries when reparsing
+ if(destination.size() <= (unsigned)i)
+ {
+ destination.push_back(-1);
+ }
+
+ parse_sound_core(tag, &destination.at(i), object_name, buf, flags);
  }
+
+ //if we're using the old format, double check the size)
+ if(!(flags & PARSE_SOUND_SCP_SOUND_LIST) && (destination.size() != (unsigned)check))
+ {
+ mprintf(("%s in '%s' has %i entries. This does not match entered size of %i.", tag, object_name, destination.size(), check));
+ }
  }
 }
 
Index: code/gamesnd/gamesnd.h
===================================================================
--- code/gamesnd/gamesnd.h (revision 8068)
+++ code/gamesnd/gamesnd.h (working copy)
@@ -28,13 +28,25 @@
 void gamesnd_play_iface(int n);
 void gamesnd_play_error_beep();
 int gamesnd_get_by_name(char* name);
+int gamesnd_get_by_iface_name(char* name);
 int gamesnd_get_by_tbl_index(int index);
 int gamesnd_get_by_iface_tbl_index(int index);
 
+//flags for parse_sound and parse_sound_list
+enum parse_sound_flags
+{
+ PARSE_SOUND_GENERAL_SOUND = 0, //!< search for sound in the general table in sound.tbl
+ PARSE_SOUND_INTERFACE_SOUND = (1 << 0), //!< Search for sound in the interface part of sounds.tbl
+ PARSE_SOUND_SCP_SOUND_LIST = (1 << 1), //!< Parse the list of sounds SCP style (just indexes and/or files names, no count first)
+ PARSE_SOUND_MAX
+};
+
 //This should handle NO_SOUND just fine since it doesn't directly access lowlevel code
 //Does all parsing for a sound
-void parse_sound(char* tag, int *idx_dest, char* object_name);
+void parse_sound(char* tag, int* idx_dest, char* object_name, parse_sound_flags = PARSE_SOUND_GENERAL_SOUND);
 
+void parse_sound_list(char* tag, SCP_vector<int>& destination, char* object_name, parse_sound_flags = PARSE_SOUND_GENERAL_SOUND);
+
 // this is a callback, so it needs to be a real function
 void common_play_highlight_sound();
 
Index: code/menuui/mainhallmenu.cpp
===================================================================
--- code/menuui/mainhallmenu.cpp (revision 8068)
+++ code/menuui/mainhallmenu.cpp (working copy)
@@ -107,8 +107,8 @@
  // panning values for each of the misc anims
  float misc_anim_sound_pan[MAX_MISC_ANIMATIONS];
 
- // [N][0] == # of sounds, [N][1-9] sound index
- int misc_anim_special_sounds[MAX_MISC_ANIMATIONS][10];
+ //sounds for each of the misc anims
+ SCP_vector<SCP_vector<int>> misc_anim_special_sounds;
 
  // [N][0] == # of triggers, [N][1-9] >= frame num
  int misc_anim_special_trigger[MAX_MISC_ANIMATIONS][10];
@@ -133,7 +133,7 @@
  int door_anim_coords[MAX_DOOR_ANIMATIONS][4];
 
  // sounds for each region (open/close)
- int door_sounds[MAX_DOOR_ANIMATIONS][2];
+ SCP_vector<SCP_vector<int>> door_sounds;
 
  // pan values for the door sounds
  float door_sound_pan[MAX_DOOR_ANIMATIONS];
@@ -1182,7 +1182,7 @@
  }
  // animation is not paused
  else {
- for (s_idx = Main_hall->misc_anim_special_sounds[idx][0]; s_idx > 0; s_idx--) {
+ for (s_idx = 0; (unsigned)s_idx < Main_hall->misc_anim_special_sounds.at(idx).size(); s_idx++) {
  // if we've passed the trigger point, then play the sound and break out of the loop
  if ((Main_hall_misc_anim[idx].current_frame >= Main_hall->misc_anim_special_trigger[idx][s_idx]) && !Main_hall->misc_anim_sound_flag[idx][s_idx]) {
  Main_hall->misc_anim_sound_flag[idx][s_idx] = 1;
@@ -1195,7 +1195,7 @@
  }
 
  // play the sound
- Main_hall->misc_anim_sound_handles[idx][s_idx] = snd_play(&Snds_iface[Main_hall->misc_anim_special_sounds[idx][s_idx]],Main_hall->misc_anim_sound_pan[idx]);
+ Main_hall->misc_anim_sound_handles[idx][s_idx] = snd_play(&Snds_iface[Main_hall->misc_anim_special_sounds.at(idx).at(s_idx)],Main_hall->misc_anim_sound_pan[idx]);
  break;
  }
  }
@@ -1308,7 +1308,7 @@
  if (Main_hall_door_sound_handles[region] != -1) {
  snd_stop(Main_hall_door_sound_handles[region]);
  }
- Main_hall_door_sound_handles[region] = snd_play(&Snds_iface[Main_hall->door_sounds[region][1]], Main_hall->door_sound_pan[region]);
+ Main_hall_door_sound_handles[region] = snd_play(&Snds_iface[Main_hall->door_sounds.at(region).at(1)], Main_hall->door_sound_pan[region]);
 
  //TODO: track current frame
  snd_set_pos(Main_hall_door_sound_handles[region], &Snds_iface[SND_MAIN_HALL_DOOR_CLOSE],
@@ -1334,7 +1334,7 @@
  if(Main_hall_door_sound_handles[region] != -1){
  snd_stop(Main_hall_door_sound_handles[region]);
  }
- Main_hall_door_sound_handles[region] = snd_play(&Snds_iface[Main_hall->door_sounds[region][0]],Main_hall->door_sound_pan[region]);
+ Main_hall_door_sound_handles[region] = snd_play(&Snds_iface[Main_hall->door_sounds.at(region).at(0)],Main_hall->door_sound_pan[region]);
 
  // start the sound playing at the right spot relative to the completion of the animation
  if( (Main_hall_door_anim[region].num_frames > 0) && (Main_hall_door_anim[region].current_frame != -1) ) {
@@ -1660,8 +1660,7 @@
  }
  for(idx=0; idx<m->num_random_intercom_sounds; idx++){
  // intercom sound id
- required_string("+Intercom sound:");
- stuff_int(&m->intercom_sounds[idx]);
+ parse_sound("+Intercom sound:", &m->intercom_sounds[idx], "+Intercom sound:", PARSE_SOUND_INTERFACE_SOUND);
  }
  for(idx=0; idx<m->num_random_intercom_sounds; idx++){
  // intercom pan
@@ -1714,11 +1713,9 @@
  }
  for(idx=0; idx<m->num_misc_animations; idx++){
  // anim sound id
- required_string("+Misc anim sounds:");
- stuff_int(&m->misc_anim_special_sounds[idx][0]);
- for(s_idx=0; s_idx<m->misc_anim_special_sounds[idx][0]; s_idx++){
- stuff_int(&m->misc_anim_special_sounds[idx][s_idx + 1]);
- }
+ SCP_vector<int> temp; //this is a short term hack and will be properly fixed when main_hall_defines is vectorised
+ m->misc_anim_special_sounds.push_back(temp);
+ parse_sound_list("+Misc anim sounds:", m->misc_anim_special_sounds.at(idx), "+Misc anim sounds:", PARSE_SOUND_INTERFACE_SOUND);
  }
  for(idx=0; idx<m->num_misc_animations; idx++){
  // anim sound triggers
@@ -1761,9 +1758,9 @@
  }
  for(idx=0; idx<m->num_door_animations; idx++){
  // door open and close sounds
- required_string("+Door sounds:");
- stuff_int(&m->door_sounds[idx][0]);
- stuff_int(&m->door_sounds[idx][1]);
+ SCP_vector<int> temp; //this is a short term hack and will be properly fixed when main_hall_defines is vectorised
+ m->door_sounds.push_back(temp);
+ parse_sound_list("+Door sounds:", m->door_sounds.at(idx), "+Door sounds:", (parse_sound_flags)(PARSE_SOUND_INTERFACE_SOUND | PARSE_SOUND_SCP_SOUND_LIST));
  }
  for(idx=0; idx<m->num_door_animations; idx++){
  // door pan value
@@ -1790,10 +1787,10 @@
  if(Vasudan_funny){
  int hall = main_hall_id();
 
- Main_hall_defines[GR_640][hall].door_sounds[OPTIONS_REGION][0] = SND_VASUDAN_BUP;
- Main_hall_defines[GR_640][hall].door_sounds[OPTIONS_REGION][1] = SND_VASUDAN_BUP;
- Main_hall_defines[GR_1024][hall].door_sounds[OPTIONS_REGION][0] = SND_VASUDAN_BUP;
- Main_hall_defines[GR_1024][hall].door_sounds[OPTIONS_REGION][1] = SND_VASUDAN_BUP;
+ Main_hall_defines[GR_640][hall].door_sounds.at(OPTIONS_REGION).at(0) = SND_VASUDAN_BUP;
+ Main_hall_defines[GR_640][hall].door_sounds.at(OPTIONS_REGION).at(1) = SND_VASUDAN_BUP;
+ Main_hall_defines[GR_1024][hall].door_sounds.at(OPTIONS_REGION).at(0) = SND_VASUDAN_BUP;
+ Main_hall_defines[GR_1024][hall].door_sounds.at(OPTIONS_REGION).at(1) = SND_VASUDAN_BUP;
 
  // set head anim. hehe
  strcpy_s(Main_hall_defines[GR_640][hall].door_anim_name[OPTIONS_REGION], "vhallheads");
Index: code/parse/parselo.cpp
===================================================================
--- code/parse/parselo.cpp (revision 8068)
+++ code/parse/parselo.cpp (working copy)
@@ -430,6 +430,15 @@
  return 0;
 }
 
+int check_for_eoln()
+{
+ ignore_gray_space();
+
+ if(*Mp == EOLN)
+ return 1;
+ else
+ return 0;
+}
 // similar to optional_string, but just checks if next token is a match.
 // It doesn't advance Mp except to skip past white space.
 //
Index: code/parse/parselo.h
===================================================================
--- code/parse/parselo.h (revision 8068)
+++ code/parse/parselo.h (working copy)
@@ -155,6 +155,7 @@
 extern int check_for_string(char *pstr);
 extern int check_for_string_raw(char *pstr);
 extern int check_for_eof();
+extern int check_for_eoln();
 
 // from aicode.cpp
 extern void parse_float_list(float *plist, int size);

Apply this patch straight to trunk.

EDIT: This patch has been tested by both mjn.mixael and myself with different mainhall.tbls and everything now works correctly.

[Iss Mneur]

Parselo.ccp and Parselo.h changes committed to trunk as 8077.
The rest was committed to trunk as 8079, sorry, I forgot credit you in the this commit.

[CommanderDJ]

Hey guys,
Another patch ready for testing. This one adds a -reparse_mainhall flag to the commandline that will result in mainhall.tbl being parsed every time a mainhall is loaded (which is currently default behaviour). Default behaviour has been changed to parsing mainhall.tbl just once at game startup.

Code: [Select]

Index: code/cmdline/cmdline.cpp
===================================================================
--- code/cmdline/cmdline.cpp (revision 8079)
+++ code/cmdline/cmdline.cpp (working copy)
@@ -199,6 +199,7 @@
  #ifdef SCP_UNIX
  { "-nograb", "Don't grab mouse/keyboard in a window", true, 0, EASY_DEFAULT, "Dev Tool", "http://www.hard-light.net/wiki/index.php/Command-Line_Reference#-nograb", },
  #endif
+ { "-reparse_mainhall", "Reparse mainhall.tbl when loading halls", false, 0, EASY_DEFAULT, "Dev Tool", "http://www.hard-light.net/wiki/index.php/Command-Line_Reference#-reparse_mainhall", },
 };
 
 // here are the command line parameters that we will be using for FreeSpace
@@ -417,6 +418,7 @@
 #ifdef SCP_UNIX
 cmdline_parm no_grab("-nograb", NULL); // Cmdline_no_grab
 #endif
+cmdline_parm reparse_mainhall_arg("-reparse_mainhall", NULL); //Cmdline_reparse_mainhall
 
 char *Cmdline_start_mission = NULL;
 int Cmdline_dis_collisions = 0;
@@ -436,6 +438,7 @@
 #ifdef SCP_UNIX
 int Cmdline_no_grab = 0;
 #endif
+int Cmdline_reparse_mainhall = 0;
 
 // Other
 cmdline_parm get_flags_arg("-get_flags", NULL);
@@ -1467,6 +1470,11 @@
  ls_force_off = true;
  }
 
+ if( reparse_mainhall_arg.found() )
+ {
+ Cmdline_reparse_mainhall = 1;
+ }
+
  return true;
 }
 
Index: code/cmdline/cmdline.h
===================================================================
--- code/cmdline/cmdline.h (revision 8079)
+++ code/cmdline/cmdline.h (working copy)
@@ -145,5 +145,6 @@
 #ifdef SCP_UNIX
 extern int Cmdline_no_grab;
 #endif
+extern int Cmdline_reparse_mainhall;
 
 #endif
Index: code/freespace2/freespace.cpp
===================================================================
--- code/freespace2/freespace.cpp (revision 8079)
+++ code/freespace2/freespace.cpp (working copy)
@@ -2025,6 +2025,11 @@
  old_alpha_colors_init();
  }
 
+ if(!Cmdline_reparse_mainhall)
+ {
+ main_hall_read_table();
+ }
+
  if (Cmdline_env) {
  ENVMAP = Default_env_map = bm_load("cubemap");
  }
Index: code/menuui/mainhallmenu.cpp
===================================================================
--- code/menuui/mainhallmenu.cpp (revision 8079)
+++ code/menuui/mainhallmenu.cpp (working copy)
@@ -369,9 +369,6 @@
 static int Main_hall_f1_text_frame = 0;
 static int F1_text_done = 0;
 
-// read in main hall table
-void main_hall_read_table();
-
 // "press f1" for help stuff
 #define MAIN_HALL_HELP_TIME 5000
 int Main_hall_help_stamp = -1;
@@ -509,8 +506,11 @@
  int idx,s_idx;
  char temp[100], whee[100];
 
- // read in the main hall table
- main_hall_read_table();
+ //reparse the table here if the relevant cmdline flag is set
+ if(Cmdline_reparse_mainhall)
+ {
+ main_hall_read_table();
+ }
 
  if(!Num_main_halls) {
  Error(LOCATION, "No main halls were loaded to initialize.");
Index: code/menuui/mainhallmenu.h
===================================================================
--- code/menuui/mainhallmenu.h (revision 8079)
+++ code/menuui/mainhallmenu.h (working copy)
@@ -18,6 +18,9 @@
 // initialize the main hall proper
 void main_hall_init(int main_hall_num);
 
+// read in mainhall.tbl
+void main_hall_read_table();
+
 // do a frame for the main hall
 void main_hall_do(float frametime);

Apply this patch straight to trunk as always (and make sure to update your checkouts to get the newly-committed flag expansion!). To test, just add -reparse_mainhall to the custom flags field of your launcher, apply the patch and build as normal. I'd ask that you also test without the flag since that's the actual change in behaviour. Post any bugs or weird behaviour here or catch me on IRC.

[The E]

Sorry for being late to the party, but

an std::out_of_range exception is thrown when calling parse_sound_list if misc_anim_special_sounds is empty. It kind of makes sense since we're passing a reference to a vector element that doesn't exist, but I too thought it wouldn't be a problem.

Then check if the vector is empty before trying to access members of it? I mean, there's a function for that... (vector.is_empty())

[Iss Mneur]

Sorry for being late to the party, but

an std::out_of_range exception is thrown when calling parse_sound_list if misc_anim_special_sounds is empty. It kind of makes sense since we're passing a reference to a vector element that doesn't exist, but I too thought it wouldn't be a problem.

Then check if the vector is empty before trying to access members of it? I mean, there's a function for that... (vector.is_empty())

Yep. We figured out what the problem is and changed the comment on the line appropriately on the commit.  Basically it comes down the the code around the area assuming that we are using static arrays and so it doesn't fill the vector with empty vectors like it should when the table is parsed.  CommanderDJ is planning on fixing that deficiency when he vectorizes the entire mainhall data structure, in the meantime we have that hack.

EDIT: -reparse_mainhall added in revision 8082

[Goober5000]

Looks like one of the patches has a bug in it:

Code: [Select]

+ else if(!strnicmp(snd->filename, name, p-snd->filename))
+ {
+ return i;
+ }

That uses a character array where strnicmp expects a size_t.  I'm fairly certain that will cause problems.

[CommanderDJ]

I did not write that code specifically, that was copied and pasted from gamesnd_get_by_name, which wasn't written by me. Your point is still valid regardless, a char array is passed where a size_t is expected. This might be a n00b question, but why doesn't that cause a compiler error? Is it casted to a size_t somehow? The code itself works fine (or seems to in all cases it's been used), but now that you've pointed it out it doesn't make much sense. What is strnicmp actually meant to do anyway?

[Goober5000]

It's likely that the code works for "well-behaved" tables -- i.e. tables that don't contain any filenames with extensions.  In such cases, the strnicmp statement will never execute.

No compiler error occurs because a pointer is, at its basic level, an unsigned integer; and unsigned integer is the same type as size_t.

Strnicmp is very similar to stricmp, except that it compares only a certain number of characters instead of the entire string's worth of them.

[CommanderDJ]

I see. I just did a quick test with a name with a file extension and it worked as expected, but I do still think we should change that last parameter. But what to? I just tried MAX_FILENAME_LEN and the results weren't pretty .

[Goober5000]

Argh... never mind; I misread the parameter as p_snd->filename, when it's actually p-snd->filename.  The - is a minus sign; it subtracts two char pointers, which is the correct behavior.

So no change is actually required.  However, to ensure nobody else makes this mistake in the future, it would be a good idea to put spaces around the minus sign.

[CommanderDJ]

Hello guys,
I have a new patch for people to test. This one brings you the addition of the +Misc anim sounds new: mainhall.tbl flag. It functions exactly the same as +Misc anim sounds: except that all entries are sound entries, no more need to put the number of sounds following into the entry. So where previously you would have (for example)

Code: [Select]

+Misc anim sounds: 1 Lift_Mix
+Misc anim sounds: 0
+Misc anim sounds: 2 Crane_1 Crane_2
you can now put

Code: [Select]

+Misc anim sounds new: Lift_Mix
+Misc anim sounds new:
+Misc anim sounds new: Crane_1 Crane_2

Note, for zero sounds, you cannot put the number zero anymore as it will be interpreted as a sound entry! From what I've tested leaving a blank line works fine, but I'd like people to test this nonetheless. Also note that the old and new tags can both be mixed and matched; as long as there's the correct number of them, it doesn't matter which tag you use to parse your misc anim sound entries.

For the SCP: I had to change parse_sound and parse_sound_list to int functions instead of void. They simply return 1 if the passed tag is found and 0 if not. Strictly speaking parse_sound didn't need to be changed but I did it for consistency (plus it might come in useful to someone in the future). If this was a bad idea for any reason please let me know and we'll work out an alternative solution.

Here's the patch. Apply straight to trunk. (Oh and this patch also addresses the issue Goober mentioned.)

Code: [Select]

Index: code/gamesnd/gamesnd.cpp
===================================================================
--- code/gamesnd/gamesnd.cpp (revision 8082)
+++ code/gamesnd/gamesnd.cpp (working copy)
@@ -43,7 +43,7 @@
  return i;
  }
  }
- else if(!strnicmp(snd->filename, name, p-snd->filename))
+ else if(!strnicmp(snd->filename, name, p - snd->filename))
  {
  return i;
  }
@@ -67,7 +67,7 @@
  return i;
  }
  }
- else if(!strnicmp(snd->filename, name, p-snd->filename))
+ else if(!strnicmp(snd->filename, name, p - snd->filename))
  {
  return i;
  }
@@ -178,8 +178,11 @@
  * @param object_name Object name being parsed
  * @param flags See the parse_sound_flags enum
  *
+ * \return 1 if tag is found
+ * \return 0 if tag is not found
+ *
  */
-void parse_sound(char* tag, int *idx_dest, char* object_name, parse_sound_flags flags)
+int parse_sound(char* tag, int *idx_dest, char* object_name, parse_sound_flags flags)
 {
  if(optional_string(tag))
  {
@@ -187,7 +190,11 @@
  stuff_string(buf, F_NAME, MAX_FILENAME_LEN);
 
  parse_sound_core(tag, idx_dest, object_name, buf, flags);
+
+ return 1;
  }
+ else
+ return 0;
 }
 
 /**
@@ -201,8 +208,11 @@
  * @param object_name Name of object being parsed
  * @param flags See the parse_sound_flags enum
  *
+ * \return 1 if tag is found
+ * \return 0 if tag is not found
+ *
  */
-void parse_sound_list(char* tag, SCP_vector<int>& destination, char* object_name, parse_sound_flags flags)
+int parse_sound_list(char* tag, SCP_vector<int>& destination, char* object_name, parse_sound_flags flags)
 {
  if(optional_string(tag))
  {
@@ -234,7 +244,11 @@
  {
  mprintf(("%s in '%s' has %i entries. This does not match entered size of %i.", tag, object_name, destination.size(), check));
  }
+
+ return 1;
  }
+ else
+ return 0;
 }
 
 /**
Index: code/gamesnd/gamesnd.h
===================================================================
--- code/gamesnd/gamesnd.h (revision 8082)
+++ code/gamesnd/gamesnd.h (working copy)
@@ -43,8 +43,8 @@
 
 //This should handle NO_SOUND just fine since it doesn't directly access lowlevel code
 //Does all parsing for a sound
-void parse_sound(char* tag, int* idx_dest, char* object_name, parse_sound_flags = PARSE_SOUND_GENERAL_SOUND);
-void parse_sound_list(char* tag, SCP_vector<int>& destination, char* object_name, parse_sound_flags = PARSE_SOUND_GENERAL_SOUND);
+int parse_sound(char* tag, int* idx_dest, char* object_name, parse_sound_flags = PARSE_SOUND_GENERAL_SOUND);
+int parse_sound_list(char* tag, SCP_vector<int>& destination, char* object_name, parse_sound_flags = PARSE_SOUND_GENERAL_SOUND);
 
 // this is a callback, so it needs to be a real function
 void common_play_highlight_sound();
Index: code/menuui/mainhallmenu.cpp
===================================================================
--- code/menuui/mainhallmenu.cpp (revision 8082)
+++ code/menuui/mainhallmenu.cpp (working copy)
@@ -1715,7 +1715,12 @@
  // anim sound id
  SCP_vector<int> temp; //this is a short term hack and will be properly fixed when main_hall_defines is vectorised
  m->misc_anim_special_sounds.push_back(temp);
- parse_sound_list("+Misc anim sounds:", m->misc_anim_special_sounds.at(idx), "+Misc anim sounds:", PARSE_SOUND_INTERFACE_SOUND);
+
+ //if we do not find the tag, maybe they're using the new one
+ if(!parse_sound_list("+Misc anim sounds:", m->misc_anim_special_sounds.at(idx), "+Misc anim sounds:", PARSE_SOUND_INTERFACE_SOUND))
+ {
+ parse_sound_list("+Misc anim sounds new:", m->misc_anim_special_sounds.at(idx), "+Misc anim sounds new:", (parse_sound_flags) (PARSE_SOUND_INTERFACE_SOUND | PARSE_SOUND_SCP_SOUND_LIST));
+ }
  }
  for(idx=0; idx<m->num_misc_animations; idx++){
  // anim sound triggers




[attachment deleted by a basterd]

[mjn.mixael]

Tested. It plays the sounds, but something isn't right. I can't exactly place my finger on it though.

It seems like it's playing all the sounds at the same time (or close to it) at the frame of the first sound trigger. (Found in +Misc Anim Trigger) My guess is the new flag isn't lining up with +Misc Anim Trigger somehow.

+Misc Anim Trigger (Just in case)
Syntax: # of triggers, with the rest being the frame for the trigger.
Misc anim trigger tells the mainhall on what frame of the misc anim to play each sound.
It'll loop around for the sounds, ALA: If there's 4 triggers but 3 sounds, the 4th trigger will play the 1st sound and then on from there. Really, there should never be a different number of sounds and triggers because things can get screwy design wise, not code wise. (As far as I can tell or understand.)

[CommanderDJ]

Okay, so with mjn's help I identified the cause of this. Interestingly enough it should have been picked up before this as the addition of the new flag wasn't the cause, but that's neither here nor there. Turns out it was just a bunch of off-by-one errors caused by the misc_anim_special_sounds vector no longer storing the number of sounds in it (this behaviour was brought in before the new flag). The thing is that there are two possible solutions: the first is a quick and dirty one which has already been confirmed to work (just incrementing a bunch of indexes). The second will take me longer (still not too long) and will involve vectorising several more of the arrays, but since it ultimately results in cleaner code and I was already going to vectorise everything in the mainhall struct at some point anyway, I figure I might as well do it right and take the second option. Stay tuned.

In the meantime, mjn.mixael, is there any chance you could provide a brief explanation of the +Misc anim handles: and +Misc anim flags: flags? I've become much more familiar with the mainhall code recently, but I haven't looked at them thoroughly, and I'd like to know what they're for.

[mjn.mixael]

Those are a bit more of a mystery.

'+Misc anim handles' seems to define the number of sounds that can be playing concurrently for the anim. (Which makes sense to have considering the retail sound code limitations.) This one has a few retail values set to 0, so that may not be entirely accurate.

'+Misc anim flags' is even wierder. It doesn't have any possible flags that do anything, but instead seems to define which sound listed in '+Misc anim sounds' will be played first. But this may or may not be accurate. I leave it as 1, but the retail table has a few set to 2 for some reason and even 0... Actually.. that makes no sense. The retail table for Aquitaine low res has 3 misc anims with the flags being 1, 2, & 2... but the hi res version those anims are set to 2, 2, & 0... so yeah. I've no idea. It may not do anything at all, but it'd be nice to figure it out for sure.

[Goober5000]

CommanderDJ, you ought to be able to easily find the answers to those in the codebase.  Search for the string in question and read the comments.  The comments can be cryptic, but you can ask us to translate any you don't understand.

The +Misc anim flags, I believe, tells how the animation is supposed to play or repeat.  One number means to play once and stop, one number means to play and repeat, and one number means to play at a triggered event, or at a random time, or something.  This is explained somewhere in the code.  (And Mjn.Mixael, I thought I sent you a description of how that worked.)