Hard Light Productions Forums

Modding, Mission Design, and Coding => FS2 Open Coding - The Source Code Project (SCP) => Topic started by: mjn.mixael on October 31, 2011, 01:38:13 pm

Title: Modular Mainhall.tbl
Post by: mjn.mixael on October 31, 2011, 01:38:13 pm
From working on my mainhalls, I've obviously been doing a lot with the mainhall.tbl. Currently I have 4 working mainhalls with several more on the way. Ideally, I'd be able to release them as a simple drag and drop VP file.. one for each mainhall. The problem is that the mainhall.tbl isn't modular so that makes it much harder to people to add new mainhalls to any given mod. I'm pretty sure I've had passing discussions about this with various coders, but it's been so long that I can't remember who or what was said. So, I wanted to type out my thoughts on how this might work and see what is or isn't possible from the coders... and hopefully find someone who is willing to try and make it happen (probably after .14 is out).

So, the mainhall table is index based starting with 0 and going up to X (There is a limit to the number of mainhalls that can be loaded and I don't know it off the top of my head). My guess is that is going to be the first hurdle. How do you have FSO organize the indexes without breaking retail while still using modular tables? A solution I thought of was using an optional +Index flag (Required in TBMs but not in TBLs?). Basically, the TBL would parse first and those mainhalls be indexed like normal. Then TBMs are parsed and the mainhalls added to the index number specified. You could use this to override mainhalls in the TBL if necessary. If an index is skipped, the engine should pop up a warning. Then also, an '+Index: -1' (or no +Index?) simply adds the new mainhall from the TBM into the next available Index.

The other hurdle is the mainhall limit. I don't think the limit needs to be removed or extended.. I'm pretty sure that it's high enough that no mod should ever go over it. But the question is, what should FSO do if it encounters enough TBMs that pushes the limit? I'd say throw up a Warning and drop the last mainhall until it's back under the index.

EDIT: Looks like the limit is 10. So perhaps bumping that to something like 20 or 25?

Mainhalls are referenced in campaign files by index number so it seems that the challenge is to get FSO to load mainhalls from a tbm and order them into the same index structure.

The way the mainhall.tbl works.. it seems like the work involved should be similar to ships and weapons tables where it's mostly about adding new entries to the end of the list and the rest of the flags are only important for each individual entry. Except that the mainhalls probably affect a much smaller portion of the engine.

Am I forgetting something? What are some thoughts here? Are there issues I don't know about? Or is there a better way of doing this?

EDIT: Here is an example mainhall.tbl for those not familiar with what it looks like. (http://pastebin.com/DxRP6DYr)
Title: Re: Modular Mainhall.tbl
Post by: Droid803 on November 01, 2011, 12:19:44 am
+Index would work well - you'd be able to control which to overwrite when changing existing entires in the base table as well as append on afterwards with it. :yes:

Like with the sections in $Beaminfo.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on November 01, 2011, 12:55:38 am
IIRC, the function that parses tables and modular tables is the same for the ones currently in the code, so I don't think that you can treat tbls and tbms differently (that said, there's nothing stopping whoever writes the code for this to create a new function just for modular mainhall tables). It should be doable with just one function, and the index thing sounds like a good idea.

If nobody else has looked at this by the time my exams are over, I might have a gander at it.
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on November 01, 2011, 08:33:55 am
Point. There probably doesn't need to be a new function to parse TBLs and TBMs differently if the +Index flag is optional anyway, right?

The TBL would parse and those mainhalls would get dropped in indexes 0-X if they don't have +Index. (Thinking of retail here.) That preserves retail compatibility. Then the TBMs and get dropped in indexes (X+1)-Y.

Then if ANY entry (in a TBM or TBL) has +index, it gets dropped into that specific index replacing whatever is there.

Then just create some warnings to warn the modder when they skipped an index because of +Index (IE: They have 4 mainhalls total, but one of their +Indexes is set to 8) and when they have more mainhalls than the limit will allow.
Title: Re: Modular Mainhall.tbl
Post by: Goober5000 on November 01, 2011, 02:52:07 pm
Suppose you then end up with five TBMs specifying indexes 0, 1, 2, 2, and 7?  And then suppose the campaign file makes reference to index 5?
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on November 01, 2011, 04:08:42 pm
I'd say if an index specified skips over numbers, it should display a warning? Or perhaps autobump it to the highest available index that doesn't skip?

EX: You have five TBMs specifying indexes 0, 1, 2, 3, and 7.. The game gets to 7 and pops up a warning, then switches 7 to 4.

As for the duplicate 2s.. I'd think it'd be as simple as the latest 2 overwrites the previous 2.

As for the mainhall calling for 5 when 5 doesn't exist.. it should do what FSO currently does.. drop a warning and use index 0.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on November 01, 2011, 05:15:50 pm
I think both should happen. Tell the modder they've screwed up but compensate for it so the game doesn't **** bricks. Defensive programming.

With the duplicate 2s, you've got two options (worth considering, that is): either replace the old 2 with the new like you said... or bump the duplicate to index 3 and go on from there (obviously dropping warnings in both cases). Main problem I see with the latter is that the game wouldn't know to compensate for the upped index, right? It would still search for the mainhall with index 2 but would find the old one. At least that's my understanding, I haven't looked at that part of the code in detail.
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on November 01, 2011, 05:24:28 pm
I suggest going with the replace method. That way if we get into a situation where we have a tbm in one mod. Then we have another mod that is using the first as a dependancy that also has a tbm. The latter can replace mainhalls in the former using replacement +index numbers.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on November 01, 2011, 06:10:13 pm
Makes sense to me.  :yes:
Title: Re: Modular Mainhall.tbl
Post by: Goober5000 on November 01, 2011, 09:09:28 pm
I think it would be better if we added the ability to specify main hall names.  That would be clearer and wouldn't depend on the order the tbms were loaded.
Title: Re: Modular Mainhall.tbl
Post by: CaptJosh on November 01, 2011, 10:47:17 pm
Makes sense to me, Goober. The retail behavior remains in place, but an additional feature is there for modders. A clean and simple solution. That just leaves the question of how easy it is to code this simple solution. After all, simple and easy are two different things.
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on November 01, 2011, 11:35:27 pm
See, but wouldn't referencing mainhalls by name would require a change in how campaign files work.. and likely pilot files too?
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on November 02, 2011, 12:09:00 am
And don't forget the campaign editor in FRED. And if you change that, according to some you'd also have to change any documentation such as the FRED walkthrough that reference it.
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on November 02, 2011, 12:12:55 am
Which is why I think going with indexes will be more than enough.
Title: Re: Modular Mainhall.tbl
Post by: CaptJosh on November 02, 2011, 07:28:13 am
And this sort of thing is why I distinguish between simple and easy.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on November 02, 2011, 07:38:49 am
Yeah. I've spoken with The_E, who believes that making the number of mainhalls dynamic is necessary before further work, and I'd agree, as the current implementation wastes a fair bit of memory. However, once this is done it will mean that using index numbers to identify mainhalls will no longer be a reliable method, and that using another identifier such as a name per Goober's suggestion is a much more elegant solution. It will require more work and I likely won't be able to do it on my own (though I'm certainly willing to help where I can), but personally I'm for it as it ultimately leads to better and cleaner code in the long run.
Title: Re: Modular Mainhall.tbl
Post by: CaptJosh on November 02, 2011, 07:43:09 am
The issue with that is the index numbers of 0 through 9 have to remain usable for retail compatibility. So something has to be done to retain that, but perhaps that code can be cleaned up to use less memory.
Title: Re: Modular Mainhall.tbl
Post by: The E on November 02, 2011, 07:46:40 am
Not as large an issue as you may think. As of now, mainhalls are only identified by their index, there's no "Name" field available. As such, if no name is defined, the index can be used as that namehall's "name".
Title: Re: Modular Mainhall.tbl
Post by: CaptJosh on November 02, 2011, 08:21:22 am
True, but the index itself is not defined in the table. This all needs more thinking about, because it all has to be, as you mentioned on IRC, The_E, backward compatible and transparent to all involved.
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on November 02, 2011, 08:38:27 am
But the game could continue to parse the index like it always has and then set that index number as the name.
Title: Re: Modular Mainhall.tbl
Post by: CaptJosh on November 02, 2011, 09:22:14 am
An excellent point. So, basically... (the following is pseudocode)
Code: [Select]
if (isset($name)) {
 $name = $mainhall;
} else { $mainhall == $index; }
Title: Re: Modular Mainhall.tbl
Post by: Goober5000 on November 02, 2011, 11:19:52 pm
See, but wouldn't referencing mainhalls by name would require a change in how campaign files work.. and likely pilot files too?
The newest pilot code (still in antipodes) references ships and weapons by name, rather than index.  So this could be introduced at the same time.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on November 29, 2011, 08:26:52 pm
Bumping for great justice; I'm going to start having a look at this over the next few days and seeing what parts I can implement. Before I do however, I just want to make sure that this is what we want:

1. Dynamic mainhalls: currently the code limits us to 10 mainhalls at maximum, and 2 of those are really ever used, wasting memory. To save memory and to future-proof the code, we want the code to treat mainhalls dynamically.

2. Mainhall names: currently the code refers to mainhalls only by their index into the 2-dimensional Main_hall_defines array (someone correct me on this if I'm wrong). Once mainhalls are made dynamic (using vectors), indexes will no longer be a reliable method of identifying a particular mainhall. Thus mainhalls should have a table-defined Name property that will be used as its identifier. If no name is specified, the code should assign whatever index the mainhalls happens to be loaded at as its name.

3. Modular mainhalls (the reason this thread exists, lol): Enough said, really. After 1 and 2 we want to be able to use modular tables for mainhalls.

Discuss! Once the requirements have been established I'll see what I can do (with the SCP's help) to start implementing them.
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on November 29, 2011, 08:57:39 pm
That all looks pretty good and what I'm hoping for.

Now, this is something else I started thinking about which may be a case of me asking for cake AND cupcakes for desert... but I won't know until I ask.

The table would still be limited in it's modular ability because of the sounds. I know there's a lot of issues to making the sounds.tbl modular... So...

Would it be possible to expand the mainhall.tbl flags "+Intercom sound:" "+Misc anim sounds:" and "+Door sounds:" to also accept a filename string? (Similar to table entries like "$Warpout Start Sound:" in the ships.tbl?
Title: Re: Modular Mainhall.tbl
Post by: Goober5000 on November 30, 2011, 12:15:16 am
Would it be possible to expand the mainhall.tbl flags "+Intercom sound:" "+Misc anim sounds:" and "+Door sounds:" to also accept a filename string? (Similar to table entries like "$Warpout Start Sound:" in the ships.tbl?
This would be the easiest feature in the thread. :p  All you'd have to do is perform a lookup in sounds.tbl if the stuff after the colon is text rather than a number.


One more design change would also be warranted.  Currently, I believe mainhall.tbl is parsed each and every time the mainhall screen is displayed.  It would be better to parse it once at game start.
Title: Re: Modular Mainhall.tbl
Post by: karajorma on November 30, 2011, 04:07:28 am
Parse it every time in debug then! Anybody working on changing the main hall would prefer that. :p
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 02, 2011, 11:08:34 pm
Okay, here's the order in which I'll work on these features:

1. Expansion of "+Intercom sound:" "+Misc anim sounds:" and "+Door sounds:" flags to accept filename strings.
2. Having mainhall.tbl parsed only once at game load in release builds and every time a mainhall is displayed in debug builds (this is current behaviour for all builds).
3. Introducing name identifiers for mainhalls and updating the code to use these to refer to mainhalls rather than indexes, and adding the accompanying +Name flag to mainhall.tbl.
4. Changing the mainhall array to a vector and thus making mainhalls dynamic, and updating the code accordingly.
5. Modularizing mainhall.tbl.

If all goes as planned, expect to see a patch for number 1 later today.
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on December 03, 2011, 08:27:42 pm
OK, this post probably has more info than necessary, but I like to be thorough since few people have actually looked into the mainhall.tbl and it's quirks, but here is a rundown of the different sound flags and how they work/might work with string ability... as requested.

Here's how the sound stuff might work with string ability. The three different sections are all slightly different, so I'll go through each one.

Things to consider for all three: Would the file extension need to be included? (EX: SOUND or SOUND.EXT) Some of the flags list multiple sounds in one line. Should those get quotes or how would spaces be handled in filenames?

Intercom Sounds
Code: [Select]
+Num Intercom Sounds: 1
+Intercom delay: 8000 15000
+Intercom sound: 38
+Intercom pan: 0.0
The entries are fairly self explanatory. The flag in question for giving string ability will be '+Intercom sound:' where it currently gets the index to the sound from the sounds.tbl. This flag should only ever have one sound listed (but the flag itself can be listed the number of times equal to the number of total intercom sounds). An example with string ability.
Code: [Select]
+Num Intercom Sounds: 1
+Intercom delay: 8000 15000
+Intercom sound: 'SOUND.EXT'
+Intercom pan: 0.0

Misc Anim Sounds
Code: [Select]
+Num Misc Animations: 1
+Misc anim: LOWgal-m1
+Misc anim delay: -1 9000 30000
+Misc anim coords: 14 14
+Misc anim mode: 2
+Misc anim pan: -0.5
+Misc anim sounds: 3 67 68 67
+Misc anim trigger: 3 1 20 42
+Misc anim handles: 3
+Misc anim flags: 1
Misc anims are probably the most complicated sound wise. The flag you need to pay attention to is '+Misc anim sounds:'. It's current syntax has the first number being the total number of sounds followed by the index numbers for the sounds listed in the sounds.tbl. Again, tehre can be multiple of this flag matching to each misc anim. An example with string ability.
Code: [Select]
+Num Misc Animations: 1
+Misc anim: LOWgal-m1
+Misc anim delay: -1 9000 30000
+Misc anim coords: 14 14
+Misc anim mode: 2
+Misc anim pan: -0.5
+Misc anim sounds: 3 'SOUND 1.EXT' 'SOUND 2.EXT' 'SOUND 3.EXT'
+Misc anim trigger: 3 1 20 42
+Misc anim handles: 3
+Misc anim flags: 1

Door Anim Sounds
Code: [Select]
+Num Door Animations: 1
+Door anim: LOWgal-d1
+Door coords: 68 260 103 298
+Door sounds: 23 24
+Door pan: -0.7
And again for door animations. However, these will always only have two sounds listed in '+Door sounds:' (Mouse over, mouse off). An example with string ability.
Code: [Select]
+Num Door Animations: 1
+Door anim: LOWgal-d1
+Door coords: 68 260 103 298
+Door sounds: 'SOUND1.EXT' 'SOUND2.EXT'
+Door pan: -0.7
Title: Re: Modular Mainhall.tbl
Post by: Iss Mneur on December 03, 2011, 11:08:20 pm
Things to consider for all three: Would the file extension need to be included? (EX: SOUND or SOUND.EXT) Some of the flags list multiple sounds in one line. Should those get quotes or how would spaces be handled in filenames?
In .tbl files, the extension of a file should not be used (if a .tbl requires an extension for a filename then that is a bug).

As far as I know, spaces are forbidden in file names in FSO.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 04, 2011, 02:30:30 am
Here's a first patch for feature number 1!

Code: [Select]
Index: code/gamesnd/gamesnd.cpp
===================================================================
--- code/gamesnd/gamesnd.cpp (revision 8063)
+++ 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.
Index: code/gamesnd/gamesnd.h
===================================================================
--- code/gamesnd/gamesnd.h (revision 8063)
+++ code/gamesnd/gamesnd.h (working copy)
@@ -28,6 +28,7 @@
 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);
 
Index: code/menuui/mainhallmenu.cpp
===================================================================
--- code/menuui/mainhallmenu.cpp (revision 8063)
+++ code/menuui/mainhallmenu.cpp (working copy)
@@ -1595,8 +1595,41 @@
 int main_hall_id()
 {
  return (Main_hall - &Main_hall_defines[gr_screen.res][0]);
-}
+}
 
+/**
+ *CommanderDJ - stuff a sound index either directly
+ *or by looking up the filename in sounds.tbl
+ *@param to_stuff Variable to be stuffed
+ *@param is_interface_sound Whether the sound is an interface sound or not
+ *
+ *
+*/
+void stuff_sound_by_index_or_filename(int* to_stuff, bool is_interface_sound)
+{
+ char temp[MAX_FILENAME_LEN];
+ stuff_string_white(temp, MAX_FILENAME_LEN);
+
+ //is this an index (integer) or a filename (string)?
+ if(can_construe_as_integer(temp))
+ *to_stuff = atoi(temp);
+ else
+ {
+ //find the appropriate index in sounds.tbl
+ int index = 0;
+ if(is_interface_sound)
+ index = gamesnd_get_by_iface_name(temp);
+ else
+ index = gamesnd_get_by_name(temp);
+
+ //sanity check
+ if(index == -1)
+ Warning(LOCATION, "Invalid sound filename '%s' in mainhall.tbl! No sound will be played!\n", temp);
+
+ *to_stuff = index;
+ }
+}
+
 // read in main hall table
 void main_hall_read_table()
 {
@@ -1661,7 +1694,7 @@
  for(idx=0; idx<m->num_random_intercom_sounds; idx++){
  // intercom sound id
  required_string("+Intercom sound:");
- stuff_int(&m->intercom_sounds[idx]);
+ stuff_sound_by_index_or_filename(&m->intercom_sounds[idx], true);
  }
  for(idx=0; idx<m->num_random_intercom_sounds; idx++){
  // intercom pan
@@ -1717,7 +1750,7 @@
  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]);
+ stuff_sound_by_index_or_filename(&m->misc_anim_special_sounds[idx][s_idx + 1], true);
  }
  }
  for(idx=0; idx<m->num_misc_animations; idx++){
@@ -1762,8 +1795,8 @@
  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]);
+ stuff_sound_by_index_or_filename(&m->door_sounds[idx][0], true);
+ stuff_sound_by_index_or_filename(&m->door_sounds[idx][1], true);
  }
  for(idx=0; idx<m->num_door_animations; idx++){
  // door pan value

As IssMneur said, the code doesn't use file extensions - if they're included then they're ignored with the code above. As far as syntax goes for the patch above, quotes are optional - the way the code treats it is to use everything between quotes as a filename if they're included, and if not then it reads the name until it hits whitespace.

Quick breakdown of the code above: I put everything into a function called stuff_sound_by_index_or_filename() (I wasn't sure which file to put that in so I just left it in mainhallmenu.cpp for now. I'm sure there's a better place for it so if a coder has suggestions please give them). If the table entry has an integer this gets treated as an index, but if it's text then the code treats that text as a filename and locates the corresponding index and uses that (-1 is used if the file doesn't exist). gamesnd_get_by_iface_name() is an almost direct copy and paste of gamesnd_get_by_name() but using the Snds_iface vector as opposed to Snds.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 04, 2011, 03:02:51 am
D-d-d-double post! Here's a potential patch for feature number 2! It's a very small patch, and I'm hoping I've understood everything correctly and haven't done anything inherently dangerous. But that's where coder review comes in!

Code: [Select]
Index: code/freespace2/freespace.cpp
===================================================================
--- code/freespace2/freespace.cpp (revision 8065)
+++ code/freespace2/freespace.cpp (working copy)
@@ -2025,6 +2025,11 @@
  old_alpha_colors_init();
  }
 
+#ifdef NDEBUG
+ //if this is a release build, we only need to read in mainhall.tbl once at startup
+ main_hall_read_table();
+#endif
+
  if (Cmdline_env) {
  ENVMAP = Default_env_map = bm_load("cubemap");
  }
Index: code/menuui/mainhallmenu.cpp
===================================================================
--- code/menuui/mainhallmenu.cpp (revision 8065)
+++ 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,10 @@
  int idx,s_idx;
  char temp[100], whee[100];
 
+#ifndef NDEBUG
  // read in the main hall table
  main_hall_read_table();
+#endif
 
  if(!Num_main_halls) {
  Error(LOCATION, "No main halls were loaded to initialize.");
Index: code/menuui/mainhallmenu.h
===================================================================
--- code/menuui/mainhallmenu.h (revision 8065)
+++ 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 main hall table
+void main_hall_read_table();
+
 // do a frame for the main hall
 void main_hall_do(float frametime);

K. I'm done for the day. I eagerly await feedback!
Title: Re: Modular Mainhall.tbl
Post by: Eli2 on December 04, 2011, 07:51:41 am
@param to_stuff Variable to be stuffed
why use a out variable if your return type is void?
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 04, 2011, 04:33:18 pm
@param to_stuff Variable to be stuffed
why use a out variable if your return type is void?

I don't understand the question. to_stuff is passed by reference and is the actual variable that needs to be given the index into sounds.tbl. Sure, I could have used a return type instead, but I thought it was best to keep the format consistent with the other stuff_... functions.
Title: Re: Modular Mainhall.tbl
Post by: Iss Mneur on December 04, 2011, 04:51:48 pm
Don't mix using braces in conditionals.

stuff_sound_by_index_or_filename may already be implemented by parse_sound. If it does something different that can't or shouldn't be merged into parse_sound, it should be renamed, with comments to point out the difference and put into the same place as parse_sound.
Title: Re: Modular Mainhall.tbl
Post by: Goober5000 on December 04, 2011, 07:22:34 pm
I don't understand the question. to_stuff is passed by reference and is the actual variable that needs to be given the index into sounds.tbl. Sure, I could have used a return type instead, but I thought it was best to keep the format consistent with the other stuff_... functions.
Yes, this is the correct approach.

Also, you should check that there's not some hidden dependency that will be broken by main hall tables being parsed once at startup.  You never know.
Title: Re: Modular Mainhall.tbl
Post by: Iss Mneur on December 04, 2011, 07:59:49 pm

Yes, this is the correct approach.

Also, you should check that there's not some hidden dependency that will be broken by main hall tables being parsed once at startup.  You never know.
That is a good point.  Now that you mention it I think the transition out of the mainhall causes all of the bitmaps and sounds used to be unloaded.
Title: Re: Modular Mainhall.tbl
Post by: Goober5000 on December 04, 2011, 08:28:38 pm
That's distinct from parsing the table though.  If it isn't, it should be.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 04, 2011, 09:33:41 pm
So after discussion with IssMneur I went and implemented a parse_sound_list function and made a useful change to parse_sound as well as some other stuff, but it turns out after all of that Misc Anim Sounds entries aren't really sound lists because of that first number which specified how many sounds follow. There are two options for this that I can see:

1. Change the syntax for mainhall.tbl. I would see this as unacceptable, given the amount of stuff we'd be breaking and the possibly massive rewrites necessary, but it's there.
2. Write a function specifically for parsing Misc Anim Sounds entries. The thing is that neither parse_sound nor parse_sound_list work for Misc Anim Sounds because of the syntax, so I may be forced to write a function just for them. I have no problem doing this, I just wanted to get more experienced input before doing so.

If there are any viable options I've missed then I'm all ears.

On the bright side however is this: the Intercom Sounds flag can be successfully expanded using parse_sound, and the Door Sounds flag can be successfully expanded using parse_sound_list.

That said, here is the patch with the new/updated sound functions!

Code: [Select]
Index: code/gamesnd/gamesnd.cpp
===================================================================
--- code/gamesnd/gamesnd.cpp (revision 8067)
+++ 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.
@@ -85,40 +109,111 @@
 }
 
 /**
- * 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 is_interface_sound Whether the sound is an interface sound or not (defaults to false)
+ * @param buf Buffer holding string to be parsed
  *
- * 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, bool is_interface_sound, char* buf)
 {
- char buf[MAX_FILENAME_LEN];
  int idx;
 
- if(optional_string(tag))
+ if(is_interface_sound)
+ idx = gamesnd_get_by_iface_name(buf);
+ else
+ idx = gamesnd_get_by_name(buf);
+
+ if(idx != -1)
  {
- stuff_string(buf, F_NAME, MAX_FILENAME_LEN);
- idx = gamesnd_get_by_name(buf);
- if(idx != -1)
- (*idx_dest) = idx;
+ (*idx_dest) = idx;
+ }
+ else
+ {
+ if(is_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;
- }
 
- Assert( Snds.size() <= INT_MAX );
- //Ensure sound is in range
- if((*idx_dest) < -1 || (*idx_dest) >= (int)Snds.size())
+ if (idx != -1)
+ (*idx_dest) = idx;
+ }
+
+ int size_to_check = 0;
+
+ if(is_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 (Nonexistant 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 is_interface_sound Whether the sound is an interface sound or not (defaults to false)
+ *
+ */
+void parse_sound(char* tag, int *idx_dest, char* object_name, bool is_interface_sound)
+{
+ if(optional_string(tag))
+ {
+ char buf[MAX_FILENAME_LEN];
+ stuff_string(buf, F_NAME, MAX_FILENAME_LEN);
+
+ parse_sound_core(tag, idx_dest, object_name, is_interface_sound, buf);
+ }
+}
+
+/**
+ * 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 tag Tag
+ * @param idx_dest Destination of first sound index in list
+ * @param object_name Object name being parsed
+ * @param num Number of sounds to parse (defaults to 1)
+ * @param offset Element to start filling at (defaults to 0)
+ * @param is_interface_sound Whether the sound is an interface sound or not (defaults to false)
+ *
+ */
+void parse_sound_list(char* tag, int* idx_dest, char* object_name, int num, int offset, bool is_interface_sound)
+{
+ if(optional_string(tag))
+ {
+ //sanity checks
+ Assert(num>0);
+ Assert(offset>=0);
+
+ for(int i=0; i<num; 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);
+ parse_sound_core(tag, &idx_dest[i+offset], object_name, is_interface_sound, buf);
  }
  }
 }
Index: code/gamesnd/gamesnd.h
===================================================================
--- code/gamesnd/gamesnd.h (revision 8067)
+++ code/gamesnd/gamesnd.h (working copy)
@@ -28,13 +28,16 @@
 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);
 
 //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, bool is_interface_sound = false);
 
+void parse_sound_list(char* tag, int* idx_dest, char* object_name, int num = 1, int offset = 0, bool is_interface_sound = false);
+
 // this is a callback, so it needs to be a real function
 void common_play_highlight_sound();
 

I have tested all of these and they work as intended without breaking existing behaviour, AFAICT. You'll notice I have extended parse_sound a little bit and extracted the functionality it shares with parse_sound_list and put it into a helper function.

And here is the patch for the extension of Intercom Sounds and Door Sounds
Code: [Select]
Index: code/menuui/mainhallmenu.cpp
===================================================================
--- code/menuui/mainhallmenu.cpp (revision 8067)
+++ code/menuui/mainhallmenu.cpp (working copy)
@@ -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:", true);
  }
  for(idx=0; idx<m->num_random_intercom_sounds; idx++){
  // intercom pan
@@ -1761,9 +1760,7 @@
  }
  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]);
+ parse_sound_list("+Door sounds:", &m->door_sounds[idx][0], "+Door sounds:", 2, 0, true);
  }
  for(idx=0; idx<m->num_door_animations; idx++){
  // door pan value

You'll notice I've just used the tag for the name parameter for now. Once mainhalls have names they will be used instead.

Feedback/review welcome/needed!
Title: Re: Modular Mainhall.tbl
Post by: Iss Mneur on December 04, 2011, 09:35:03 pm
That's distinct from parsing the table though.  If it isn't, it should be.
Very true, but stranger things have been done.  Though it explain the current behaviour of parsing every time the mainhall is displayed.

So after discussion with IssMneur I went and implemented a parse_sound_list function and made a useful change to parse_sound as well as some other stuff, but it turns out after all of that Misc Anim Sounds entries aren't really sound lists because of that first number which specified how many sounds follow. There are two options for this that I can see:

1. Change the syntax for mainhall.tbl. I would see this as unacceptable, given the amount of stuff we'd be breaking and the possibly massive rewrites necessary, but it's there.
2. Write a function specifically for parsing Misc Anim Sounds entries. The thing is that neither parse_sound nor parse_sound_list work for Misc Anim Sounds because of the syntax, so I may be forced to write a function just for them. I have no problem doing this, I just wanted to get more experienced input before doing so.

If there are any viable options I've missed then I'm all ears.
Yes you did.  You parse the count first with stuff_int then pass that number to parse_sound_list so it can parse the rest of the line.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 04, 2011, 09:46:12 pm
Yes you did.  You parse the count first with stuff_int then pass that number to parse_sound_list so it can parse the rest of the line.

I tried this. The problem with it is that parse_sound_list (just like parse_sound) parses the tag before it parses the sounds. If it doesn't find the tag it doesn't parse the list.
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on December 04, 2011, 11:45:22 pm
I tested the patches. They definitely load sounds by string which is great! However, I tried to have it load a sound by string that wasn't already in the sounds.tbl and it never worked. It simply played the "mouse-click beep" instead.

Here's the table entry
Code: [Select]
+Door sounds: itemdraw clrsnd
'itemdraw' exists in the sounds.tbl, but 'clrsnd' doesn't. I created a wav file with a simple 3 second tone and saved it to data/sounds as 'clrsnd'. (I also tried .ogg) However, it never played that sound file. Does this patch still require the sounds to be tabled in sounds.tbl?

I've included a log to hopefully help debug.

[attachment deleted by a basterd]
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 05, 2011, 12:13:08 am
Yes, the code as of now still requires the sounds to be in sounds.tbl. To be honest I'm not sure what would be required to get it to search other places. A more experienced coder will have to chime in. Regardless, I'm going to deal with Misc Anim Sounds first before getting to that.

Oh, and regarding parsing mainhall.tbl once at startup, I compiled and tested a release build with the changed behaviour and it seems to work fine. I moved from different game states (loaded a mission etc) and back into the mainhall and everything worked as it should. As far as I can see there's no hidden dependencies, so if mjn.mixael or someone else would like to test and verify the patch then I think we'll be good.
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on December 05, 2011, 08:19:09 am
Yes, the code as of now still requires the sounds to be in sounds.tbl. To be honest I'm not sure what would be required to get it to search other places. A more experienced coder will have to chime in. Regardless, I'm going to deal with Misc Anim Sounds first before getting to that.

Hmm, well the whole reason I wanted to get it to load by filename was to bypass the sounds.tbl in the first place. (Making the mainhall.tbl much more independent from another non-modular table. So I, too, would like to hear what the experienced coders have to say on this. Perhaps it'd be much too difficult or not possible at all, which would make me sad... but I'd understand.

Loading by string is still easier because modders would only need to add the sounds to the sounds.tbl and it wouldn't matter where.

Oh, and regarding parsing mainhall.tbl once at startup, I compiled and tested a release build with the changed behaviour and it seems to work fine. I moved from different game states (loaded a mission etc) and back into the mainhall and everything worked as it should. As far as I can see there's no hidden dependencies, so if mjn.mixael or someone else would like to test and verify the patch then I think we'll be good.

I can test it, just send me the patch.
Title: Re: Modular Mainhall.tbl
Post by: Iss Mneur on December 05, 2011, 11:07:37 am
Hmm, well the whole reason I wanted to get it to load by filename was to bypass the sounds.tbl in the first place. (Making the mainhall.tbl much more independent from another non-modular table. So I, too, would like to hear what the experienced coders have to say on this. Perhaps it'd be much too difficult or not possible at all, which would make me sad... but I'd understand.

Loading by string is still easier because modders would only need to add the sounds to the sounds.tbl and it wouldn't matter where.
I was looking into this last night.  I actually thought that parse_sound implemented the direct load from file behaviour you are looking for, though having looked at it, it obviously can't.  That being said, I am sure that either weapons or ships are able to load untabled sounds so that would be where I would look.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 05, 2011, 04:17:18 pm
Oh, and regarding parsing mainhall.tbl once at startup, I compiled and tested a release build with the changed behaviour and it seems to work fine. I moved from different game states (loaded a mission etc) and back into the mainhall and everything worked as it should. As far as I can see there's no hidden dependencies, so if mjn.mixael or someone else would like to test and verify the patch then I think we'll be good.
I can test it, just send me the patch.

Here it is:

Code: [Select]
Index: code/freespace2/freespace.cpp
===================================================================
--- code/freespace2/freespace.cpp (revision 8067)
+++ code/freespace2/freespace.cpp (working copy)
@@ -2025,6 +2025,11 @@
  old_alpha_colors_init();
  }
 
+#ifdef NDEBUG
+ //if this is a release build, we only need to read in mainhall.tbl once at startup
+ main_hall_read_table();
+#endif
+
  if (Cmdline_env) {
  ENVMAP = Default_env_map = bm_load("cubemap");
  }
Index: code/menuui/mainhallmenu.cpp
===================================================================
--- code/menuui/mainhallmenu.cpp (revision 8067)
+++ 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,10 @@
  int idx,s_idx;
  char temp[100], whee[100];
 
+#ifndef NDEBUG
  // read in the main hall table
  main_hall_read_table();
+#endif
 
  if(!Num_main_halls) {
  Error(LOCATION, "No main halls were loaded to initialize.");
Index: code/menuui/mainhallmenu.h
===================================================================
--- code/menuui/mainhallmenu.h (revision 8067)
+++ 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 main hall table
+void main_hall_read_table();
+
 // do a frame for the main hall
 void main_hall_do(float frametime);
 

Make sure you test a release build though since that's the one with the changed behaviour. Thanks, hopefully once it's verified that nothing's broken this one can get committed.


Hmm, well the whole reason I wanted to get it to load by filename was to bypass the sounds.tbl in the first place. (Making the mainhall.tbl much more independent from another non-modular table. So I, too, would like to hear what the experienced coders have to say on this. Perhaps it'd be much too difficult or not possible at all, which would make me sad... but I'd understand.

Loading by string is still easier because modders would only need to add the sounds to the sounds.tbl and it wouldn't matter where.
I was looking into this last night.  I actually thought that parse_sound implemented the direct load from file behaviour you are looking for, though having looked at it, it obviously can't.  That being said, I am sure that either weapons or ships are able to load untabled sounds so that would be where I would look.

So are we intending to change parse_sound itself so that if it fails to load the sound the usual way it will search folders/VPs? Or just change it specifically for these flags? Personally I'd rather the first one, but I'm unsure of what consequences that might have.
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on December 05, 2011, 04:28:47 pm
I tested the patch. I played some missions, went to the tech room, switched between multiple campaigns/mainhalls. It seems to work as expected. :yes:
Title: Re: Modular Mainhall.tbl
Post by: Goober5000 on December 05, 2011, 05:14:13 pm
Hmm, well the whole reason I wanted to get it to load by filename was to bypass the sounds.tbl in the first place. (Making the mainhall.tbl much more independent from another non-modular table. So I, too, would like to hear what the experienced coders have to say on this. Perhaps it'd be much too difficult or not possible at all, which would make me sad... but I'd understand.
A game sound has to be stored in sounds.tbl; there's no way around that without completely rewriting the sound code.  About the most you could hope for is being able to add sounds to sounds.tbl on-the-fly, but doing that would be approximately as hard as making sounds.tbl modular.  (The two are essentially the same thing, if you think about it.)
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on December 05, 2011, 05:58:34 pm
Hmm, honest question here. How are scripts and the play-sound-from-file sexp able to do it? Is that possibly applicable?
Title: Re: Modular Mainhall.tbl
Post by: Goober5000 on December 05, 2011, 09:09:44 pm
Scripts, I dunno.  But play-sound-from-file uses a special music (not sound) handle that's sort of suspended above the rest of the music system.
Title: Re: Modular Mainhall.tbl
Post by: Iss Mneur on December 06, 2011, 12:50:57 am
Hmm, honest question here. How are scripts and the play-sound-from-file sexp able to do it? Is that possibly applicable?
play-sound-from-file uses the music subsystem (which is why it can only do one at a time).

I can't find where the lua loads a sound file from disk.  I have found where it interacts with the sound.tbl, but which seems to be done by loading from the tbl by index or tabled name.

I have been unable to find the code that I thought existed for weapons.tbl and/or ships.tbl... :(

So I started trawling my archive of incomplete patches, apparently I am conflating reality with a discussion (http://www.hard-light.net/forums/index.php?topic=70457.0) (on the internal SCP board) that Fury, chief1983, and I had over a year ago about sounds.tbl as a result of mantis 2240 (http://scp.indiegames.us/mantis/view.php?id=2240).  In essence it discuses how to make sounds.tbl modular (at least from a tabler perspective).  I still think it is doable, but it would have to go through antipodes because of how fundamentally it would change the internals of the sound manager. It would also be a lot of work.

In the meantime, ComanderDJ, I think it would be best to finish fixing up mainhall.tbl and then tackling the issues with sound so as to avoid feature creep. The proposed format is workable in face of future extension when the sound manager supports it.  So I think you should just leave the parse_sounds behaviour as you have patched and skip the ability to dynamically load sounds that are untabled for the time being. That is, only support sounds loaded by number or by tabled name for now.

If you or anyone else would like to presue modular sounds.tbl I am willing to provide guidance, otherwise it will have to wait until my plate clears.  This feature would probably endanger sound.tbls future existence and  entail removing all hard coded indexes from the interface, including (possibly) the ability to control all "default" sounds via tbl.  Yes, this is a huge order and this is why I keep putting it off/taking so long. It is basically a rewrite on order with the HUD rewrite.
Title: Re: Modular Mainhall.tbl
Post by: jg18 on December 06, 2011, 01:02:19 am
otherwise it will have to wait until my plate clears.
When was the last time that happened? :p
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 06, 2011, 01:57:15 am
In the meantime, ComanderDJ, I think it would be best to finish fixing up mainhall.tbl and then tackling the issues with sound so as to avoid feature creep. The proposed format is workable in face of future extension when the sound manager supports it.  So I think you should just leave the parse_sounds behaviour as you have patched and skip the ability to dynamically load sounds that are untabled for the time being. That is, only support sounds loaded by number or by tabled name for now.

That is what I was planning to do. I will get Misc Anim Sounds working first, then get to the other issues in this thread. We can deal with sounds.tbl later. Sorry mjn, but it seems your request deals with an entirely different area as Iss Mneur and Goober have explained. Perhaps I will see what I can do with his help after all this mainhall stuff is done.


I tested the patch. I played some missions, went to the tech room, switched between multiple campaigns/mainhalls. It seems to work as expected. :yes:

Since mjn.mixael and I have both tested the changed mainhall.tbl parsing, is there any chance that could be committed? Personally I think the changes to the sound parsing are ready as well, but I would want the changes to the mainhall flags to all be committed in one go, so I'd like that to wait until Misc Anim Sounds has been sorted out.
That said, I'll let the powers that be decide when this stuff is ready to go in. If there's anything else you need from me to bring the patches up to scratch I'm more than happy to help.
Title: Re: Modular Mainhall.tbl
Post by: pecenipicek on December 06, 2011, 03:45:05 am
Might i reccomend splitting up the differing release <-> debug behaviour into a cmdline flag? in my mind, both should behave the same, except the debug build yells at you loudly when you **** up?

My reccomendation is basically as follows:

Default behaviour, load mainhalls only once at start, if flag ticked, reparse every time?




Just a slight reccomendation.
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on December 06, 2011, 06:11:31 am
Sadness, but that makes sense IssMneur.

String functionality is still a large step forward since the sounds don't have to be in any particular sound slot this way.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 06, 2011, 06:10:49 pm
Okay, I've figured out a possible solution to Misc Anim Sounds.
Code: [Select]
required_string("+Misc anim sounds:");
stuff_int(&m->misc_anim_special_sounds[idx][0]);
parse_sound_list("", &m->misc_anim_special_sounds[idx][1], "+Misc anim sounds:", m->misc_anim_special_sounds[idx][0], 0, true);

It parses the first entry (which specifies the number of sounds) as usual, then passes the rest of the entries to parse_sound_list. The only drawback is that because I've had to use a blank tag, the warning within parse_sound_list won't specify what exactly has gone wrong if it flares up.

The only other solution I can see without writing new functions is to use parse_sound_core directly (it's part of my sound patch earlier in the thread), which would result in the correct warning message, but it would also be duplicating some code from the parse_sound function, and since parse_sound_core is meant to be a helper function I would prefer the first option. That said, I'll do what SCP feels is best.

With this solution, here's the patch containing all three flag extensions (you'll need the sound patch applied for this to work):
Code: [Select]
Index: code/menuui/mainhallmenu.cpp
===================================================================
--- code/menuui/mainhallmenu.cpp (revision 8068)
+++ code/menuui/mainhallmenu.cpp (working copy)
@@ -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:", true);
  }
  for(idx=0; idx<m->num_random_intercom_sounds; idx++){
  // intercom pan
@@ -1716,9 +1715,7 @@
  // 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]);
- }
+ parse_sound_list("", &m->misc_anim_special_sounds[idx][1], "+Misc anim sounds:", m->misc_anim_special_sounds[idx][0], 0, true);
  }
  for(idx=0; idx<m->num_misc_animations; idx++){
  // anim sound triggers
@@ -1761,9 +1758,7 @@
  }
  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]);
+ parse_sound_list("+Door sounds:", &m->door_sounds[idx][0], "+Door sounds:", 2, 0, true);
  }
  for(idx=0; idx<m->num_door_animations; idx++){
  // door pan value
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 06, 2011, 06:24:06 pm
Sorry guys, my bad, I realised that I had made a fair few changes to the sound patch since the last time it was posted. Here it is:
Code: [Select]
Index: code/gamesnd/gamesnd.cpp
===================================================================
--- code/gamesnd/gamesnd.cpp (revision 8067)
+++ 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.
@@ -85,40 +109,111 @@
 }
 
 /**
- * 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 is_interface_sound Whether the sound is an interface sound or not (defaults to false)
+ * @param buf Buffer holding string to be parsed
  *
- * 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, bool is_interface_sound, char* buf)
 {
- char buf[MAX_FILENAME_LEN];
  int idx;
 
- if(optional_string(tag))
+ if(is_interface_sound)
+ idx = gamesnd_get_by_iface_name(buf);
+ else
+ idx = gamesnd_get_by_name(buf);
+
+ if(idx != -1)
  {
- stuff_string(buf, F_NAME, MAX_FILENAME_LEN);
- idx = gamesnd_get_by_name(buf);
- if(idx != -1)
- (*idx_dest) = idx;
+ (*idx_dest) = idx;
+ }
+ else
+ {
+ if(is_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;
- }
 
- Assert( Snds.size() <= INT_MAX );
- //Ensure sound is in range
- if((*idx_dest) < -1 || (*idx_dest) >= (int)Snds.size())
+ if (idx != -1)
+ (*idx_dest) = idx;
+ }
+
+ int size_to_check = 0;
+
+ if(is_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 (Nonexistant 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 is_interface_sound Whether the sound is an interface sound or not (defaults to false)
+ *
+ */
+void parse_sound(char* tag, int *idx_dest, char* object_name, bool is_interface_sound)
+{
+ if(optional_string(tag))
+ {
+ char buf[MAX_FILENAME_LEN];
+ stuff_string(buf, F_NAME, MAX_FILENAME_LEN);
+
+ parse_sound_core(tag, idx_dest, object_name, is_interface_sound, buf);
+ }
+}
+
+/**
+ * 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 tag Tag
+ * @param idx_dest Destination of first sound index in list
+ * @param object_name Object name being parsed
+ * @param num Number of sounds to parse (defaults to 1)
+ * @param offset Element to start filling at (defaults to 0)
+ * @param is_interface_sound Whether the sound is an interface sound or not (defaults to false)
+ *
+ */
+void parse_sound_list(char* tag, int* idx_dest, char* object_name, int num, int offset, bool is_interface_sound)
+{
+ if(optional_string(tag))
+ {
+ //sanity checks
+ Assert(num>0);
+ Assert(offset>=0);
+
+ for(int i=0; i<num; 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);
+ parse_sound_core(tag, &idx_dest[i+offset], object_name, is_interface_sound, buf);
  }
  }
 }
Index: code/gamesnd/gamesnd.h
===================================================================
--- code/gamesnd/gamesnd.h (revision 8067)
+++ code/gamesnd/gamesnd.h (working copy)
@@ -28,13 +28,16 @@
 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);
 
 //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, bool is_interface_sound = false);
 
+void parse_sound_list(char* tag, int* idx_dest, char* object_name, int num = 1, int offset = 0, bool is_interface_sound = false);
+
 // this is a callback, so it needs to be a real function
 void common_play_highlight_sound();
 

Title: Re: Modular Mainhall.tbl
Post by: Iss Mneur on December 06, 2011, 10:01:50 pm
Default behaviour, load mainhalls only once at start, if flag ticked, reparse every time?
As much as I am opposed to adding more cmdline flags, this does make sense.  If you want to debug the mainhall you use the -reparse_mainhall_everytime.

otherwise it will have to wait until my plate clears.
When was the last time that happened? :p
Its been a while, but either way there is a queue, I will get to it eventually....
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 07, 2011, 07:26:13 am
WARNING: WALL OF TEXT!

I believe that flag extension of Intercom Sound, Misc Anim Sounds and Door Sounds is complete and ready for testing. However, a few significant changes have been made since the last update, so I'll go through them one by one (this next bit is primarily for coders. If you just want to test, scroll down to the bottom of the post and apply the patch in the attachment):

Changes to the sound parsing code
Now, I will be describing all the changes made in detail, even ones covered earlier in the thread, for clarity and completeness' sake. I like my work to be transparent to all involved. Unless any issues come up during testing this will be the final version of the updated sound parsing code. Let's dig right in.

And here is the patch for the sounds changes:
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,131 @@
 }
 
 /**
- * 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 is_interface_sound Whether the sound is an interface sound or not (defaults to false)
+ * @param buf Buffer holding string to be parsed
  *
- * 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, bool is_interface_sound, char* buf)
 {
- char buf[MAX_FILENAME_LEN];
  int idx;
 
+ if(is_interface_sound)
+ idx = gamesnd_get_by_iface_name(buf);
+ else
+ idx = gamesnd_get_by_name(buf);
+
+ if(idx != -1)
+ {
+ (*idx_dest) = idx;
+ }
+ else
+ {
+ if(is_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(is_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 is_interface_sound Whether the sound is an interface sound or not (defaults to false)
+ *
+ */
+void parse_sound(char* tag, int *idx_dest, char* object_name, bool is_interface_sound)
+{
  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, is_interface_sound, buf);
+ }
+}
+
+/**
+ * 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 SCP_format Format of list to be parsed (defaults to false)
+ * False: First entry in list is number of sounds (retail format).
+ * True: Every entry is a sound.
+ * @param is_interface_sound Whether the sound is an interface sound or not (defaults to false)
+ *
+ */
+void parse_sound_list(char* tag, SCP_vector<int>& destination, char* object_name, bool SCP_format, bool is_interface_sound)
+{
+ if(optional_string(tag))
+ {
+ //complain if the list is empty
+ if(!destination.empty())
  {
- idx = gamesnd_get_by_tbl_index(atoi(buf));
- if (idx != -1)
- (*idx_dest) = idx;
+ Warning(LOCATION, "%s in '%s' is not an empty list! Its values will be overwritten!", tag, object_name);
  }
 
- Assert( Snds.size() <= INT_MAX );
- //Ensure sound is in range
- if((*idx_dest) < -1 || (*idx_dest) >= (int)Snds.size())
+ int check=0;
+
+ //if we're using the old format, parse the first entry separately
+ if(!SCP_format)
  {
- (*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());
+ stuff_int(&check);
  }
+
+ //now read the rest of the entries on the line
+ for(int i=0;!check_for_eoln();i++)
+ {
+ char buf[MAX_FILENAME_LEN];
+ stuff_string_white(buf, MAX_FILENAME_LEN);
+ destination.push_back(-1);
+ parse_sound_core(tag, &destination.at(i), object_name, is_interface_sound, buf);
+ }
+
+ //if we're using the old format, double check the size)
+ if(!SCP_format && (destination.size() != (unsigned)check))
+ {
+ Warning(LOCATION, "%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,16 @@
 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);
 
 //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, bool is_interface_sound = false);
 
+void parse_sound_list(char* tag, SCP_vector<int>& destination, char* object_name, bool SCP_format, bool is_interface_sound);
+
 // this is a callback, so it needs to be a real function
 void common_play_highlight_sound();

Changes to general parsing code

Here it is:
Code: [Select]
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);

Changes to mainhall code
Several things to go through here.
Here's the code for these changes:
Code: [Select]
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 = 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;
@@ -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:", true);
  }
  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; //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);
+ parse_sound_list("+Misc anim sounds:", m->misc_anim_special_sounds.at(idx), "+Misc anim sounds:", false, true);
  }
  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; //put *something* in the vector so we don't cause an exception when calling parse_sound_list()
+ m->door_sounds.push_back(temp);
+ parse_sound_list("+Door sounds:", m->door_sounds.at(idx), "+Door sounds:", true, true);
  }
  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");

But wait, there's more! More features to come in the near future! In rough order of completion, you can expect:

That plus the other remaining features named in the thread:
Quote
3. Introducing name identifiers for mainhalls and updating the code to use these to refer to mainhalls rather than indexes, and adding the accompanying +Name flag to mainhall.tbl.
4. Changing the mainhall array to a vector and thus making mainhalls dynamic, and updating the code accordingly.
5. Modularizing mainhall.tbl.
You can expect more progress in the coming days if all goes to plan, I'm loving working on this so far!

Testers!
Want to test the newly extended mainhall.tbl flags without wading through my long-winded explanations or applying patch after patch? Well you're in luck because you only have to apply one! Simply download the attached file, which contains all the necessary code changes, apply it and build! Report back with any findings, good or bad! All feedback is read and taken into account!

EDIT: Just to be clear, no changes are needed to mainhall.tbl syntax to test these, except to replace sound indexes with filenames at the appropriate flags. Include filenames without quotes. Extensions are optional, as they will be ignored anyway.

[attachment deleted by a basterd]
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on December 07, 2011, 08:27:28 am
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.

[attachment deleted by a basterd]
Title: Re: Modular Mainhall.tbl
Post by: pecenipicek on December 07, 2011, 10:28:21 am
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)
Title: Re: Modular Mainhall.tbl
Post by: Sushi on December 07, 2011, 12:08:36 pm
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.
Title: Re: Modular Mainhall.tbl
Post by: pecenipicek on December 07, 2011, 12:28:17 pm
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.
Title: Re: Modular Mainhall.tbl
Post by: Iss Mneur on December 07, 2011, 02:11:11 pm
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.
Title: Re: Modular Mainhall.tbl
Post by: Sushi on December 07, 2011, 04:13:18 pm
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.  :nervous:
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 07, 2011, 06:44:00 pm
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.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 07, 2011, 08:19:12 pm
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.
Title: Re: Modular Mainhall.tbl
Post by: Iss Mneur on December 09, 2011, 12:36:59 am
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.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 09, 2011, 04:40:22 am
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.
Title: Re: Modular Mainhall.tbl
Post by: The E on December 10, 2011, 06:04:35 am
Sorry for being late to the party, but
Quote
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())
Title: Re: Modular Mainhall.tbl
Post by: Iss Mneur on December 10, 2011, 11:17:50 am
Sorry for being late to the party, but
Quote
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
Title: Re: Modular Mainhall.tbl
Post by: Goober5000 on December 11, 2011, 11:22:48 pm
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.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 12, 2011, 12:48:17 am
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?
Title: Re: Modular Mainhall.tbl
Post by: Goober5000 on December 12, 2011, 01:19:48 am
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.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 12, 2011, 03:25:54 am
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 :P.
Title: Re: Modular Mainhall.tbl
Post by: Goober5000 on December 12, 2011, 03:33:28 am
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.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 13, 2011, 12:31:13 am
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]
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on December 13, 2011, 08:59:21 am
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.)
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 13, 2011, 09:45:56 am
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.
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on December 13, 2011, 09:55:26 am
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.
Title: Re: Modular Mainhall.tbl
Post by: Goober5000 on December 13, 2011, 12:35:01 pm
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.)
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on December 13, 2011, 12:53:12 pm
That's +Misc anim mode

Do you you not know that I am the mainhall king? I know all of the flags and how they are used (or not used in this case). Thou shalt not try to correct the mainhall king!   :P

EDIT: Actually, I'm fairtly certain that +Misc anim handles and +Misc anim flags (especially the latter) do nothing and/or aren't important anymore. Perhaps we might think about depreciating those flags alltogether? It would have to be confirmed exactly what they do though.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 13, 2011, 09:33:38 pm
Okay, so after further consideration I've decided to proceed with vectorising the mainhall data structure before adding this new flag. I just have a feeling it'll be easier this way in the long run, and I feel that it should be done sooner rather than later. Feature-wise this doesn't mean any significant changes (apart from the potential removal of some hardcoded limits/magic numbers), but it will increase the code's flexibility and make adding new features easier (and it will go a long way towards preparing for a modular mainhall.tbl, which is after all the ultimate objective of this little project). This will likely take me some time and I'll be looking to the SCP for advice at times, but I'll make sure to keep this thread updated when I can.

So on that note, the first issue with vectorisation is the initialisation of vectors before they're accessed. Curently, only misc_anim_special_sounds and door_sounds are vectors, and we currently have a temporary hack in place (this was mentioned earlier in the thread) to initialise them before they're used. It looks like this:

Code: [Select]
...
SCP_vector<int> temp;
m->misc_anim_special_sounds.push_back(temp);
*code that uses misc_anim_special_sounds*
...

Obviously we want to initialise all the vectors in one spot if possible so we can simply use them without worrying about whether they exist or not. The first relevant question is where to do this. It obviously only has to be done once. main_hall_init isn't an option since the table can be read before that function is called, so we look to main_hall_read_table. I'd be fine with doing it here, but to be honest I'd prefer to spin the initialisation off into its own function and call that from main_hall_read_table. Thoughts/suggestions?

The second relevant question is what exactly the initialisation will do, or more importantly how many elements do we initialise into the vectors? I would argue that using Num Intercom Sounds, Num Misc Animations and Num Door Animations would be a good starting point, but that would mean we would have to wait until each of those are parsed before initialising that section. I'm not exactly sure which option is best, so I look to the SCP for suggestions. Thanks!
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 14, 2011, 07:26:42 am
So I had a sort of O**** moment today when I reread my earlier post and said that the bug mjn.mixael found should have been picked up earlier since it was introduced beforehand (revision 8079 to be exact). This means that trunk currently has this bug (I checked and it does. I also reverted to 8078 and that build is not affected, so it was definitely introduced in my changes in 8079). I think everyone here agrees that this is a bad thing. So I've decided to release that quick fix patch I mentioned before so it could be committed as a stopgap measure before I can get to it properly. I'm responsible for the fact that my code introduced a bug in trunk, and therefore I'm responsible for fixing it as soon as possible. The patch is simple and the changes are small. Here it is:
Code: [Select]
Index: code/menuui/mainhallmenu.cpp
===================================================================
--- code/menuui/mainhallmenu.cpp (revision 8082)
+++ code/menuui/mainhallmenu.cpp (working copy)
@@ -1184,18 +1184,18 @@
  else {
  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;
+ if ((Main_hall_misc_anim[idx].current_frame >= Main_hall->misc_anim_special_trigger[idx][s_idx+1]) && !Main_hall->misc_anim_sound_flag[idx][s_idx+1]) {
+ Main_hall->misc_anim_sound_flag[idx][s_idx+1] = 1;
 
  // if the sound is already playing, then kill it. This is a pretty safe thing to do since we can assume that
  // by the time we get to this point again, the sound will have been long finished
- if (snd_is_playing(Main_hall->misc_anim_sound_handles[idx][s_idx])) {
- snd_stop(Main_hall->misc_anim_sound_handles[idx][s_idx]);
- Main_hall->misc_anim_sound_handles[idx][s_idx] = -1;
+ if (snd_is_playing(Main_hall->misc_anim_sound_handles[idx][s_idx+1])) {
+ snd_stop(Main_hall->misc_anim_sound_handles[idx][s_idx+1]);
+ Main_hall->misc_anim_sound_handles[idx][s_idx+1] = -1;
  }
 
  // play the sound
- 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]);
+ Main_hall->misc_anim_sound_handles[idx][s_idx+1] = snd_play(&Snds_iface[Main_hall->misc_anim_special_sounds.at(idx).at(s_idx)],Main_hall->misc_anim_sound_pan[idx]);
  break;
  }
  }

I promise I will attend to the bug properly when vectorising the relevant data structures. I just haven't gotten to them yet. My sincere apologies to the SCP and anyone who's using affected trunk builds. The fault is mine and I take it very seriously. I will endeavour to test my patches much more thoroughly from now on. I hope this can be committed soon before people complain (I'm lucky we're not releasing nightlies at the moment). It has been tested by both mjn and myself.

On the bright side, I do have something to show for today's work: I've started on the vectorisation of main_hall_defines. The following patch vectorises the intercom_delay, intercom_sounds, and intercom_sound_pan data structures. I also made a function for their initialisation. I'm hoping to tackle the other sets of data structures the same way, but I'm open to the SCP's suggestions on alternatives. This patch does need to be tested by someone other than me. As always, any feedback can be posted here or you can let me know on IRC.

Code: [Select]
Index: code/menuui/mainhallmenu.cpp
===================================================================
--- code/menuui/mainhallmenu.cpp (revision 8092)
+++ code/menuui/mainhallmenu.cpp (working copy)
@@ -72,13 +72,15 @@
  int num_random_intercom_sounds;
 
  // random (min/max) delays between playing intercom sounds
- int intercom_delay[MAX_RANDOM_INTERCOM_SOUNDS][2];
+ SCP_vector<SCP_vector<int> > intercom_delay;
+ //int intercom_delay[MAX_RANDOM_INTERCOM_SOUNDS][2];
 
  // intercom sounds themselves
- int intercom_sounds[MAX_RANDOM_INTERCOM_SOUNDS];
+ SCP_vector<int> intercom_sounds;
+ //int intercom_sounds[MAX_RANDOM_INTERCOM_SOUNDS];
 
  // intercom sound pan values
- float intercom_sound_pan[MAX_RANDOM_INTERCOM_SOUNDS];
+ SCP_vector<float> intercom_sound_pan;
 
 
  // misc animations --------------------
@@ -1396,8 +1398,8 @@
  // if we have no timestamp for the next random sound, then set on
  if((Main_hall_next_intercom_sound_stamp == -1) && (Main_hall_intercom_sound_handle == -1)){
  Main_hall_next_intercom_sound_stamp = timestamp((int)(((float)rand()/(float)RAND_MAX) *
-                                             (float)(Main_hall->intercom_delay[Main_hall_next_intercom_sound][1]
-   - Main_hall->intercom_delay[Main_hall_intercom_sound_handle][0])) );
+                                             (float)(Main_hall->intercom_delay.at(Main_hall_next_intercom_sound).at(1)
+   - Main_hall->intercom_delay.at(Main_hall_next_intercom_sound).at(0))) );
  }
 
  // if the there is no sound playing
@@ -1409,7 +1411,7 @@
  // if the timestamp has popped, play a sound
  if((Main_hall_next_intercom_sound_stamp != -1) && (timestamp_elapsed(Main_hall_next_intercom_sound_stamp))){
  // play the sound
- Main_hall_intercom_sound_handle = snd_play(&Snds_iface[Main_hall->intercom_sounds[Main_hall_next_intercom_sound]]);
+ Main_hall_intercom_sound_handle = snd_play(&Snds_iface.at(Main_hall->intercom_sounds.at(Main_hall_next_intercom_sound)));
 
  // unset the timestamp
  Main_hall_next_intercom_sound_stamp = -1;
@@ -1428,8 +1430,8 @@
 
  // set the timestamp
  Main_hall_next_intercom_sound_stamp = timestamp((int)(((float)rand()/(float)RAND_MAX) *
-                                             (float)(Main_hall->intercom_delay[Main_hall_next_intercom_sound][1]
-   - Main_hall->intercom_delay[Main_hall_next_intercom_sound][0])) );
+                                             (float)(Main_hall->intercom_delay.at(Main_hall_next_intercom_sound).at(1)
+   - Main_hall->intercom_delay.at(Main_hall_next_intercom_sound).at(0))) );
 
  // release the sound handle
  Main_hall_intercom_sound_handle = -1;
@@ -1595,8 +1597,39 @@
 int main_hall_id()
 {
  return (Main_hall - &Main_hall_defines[gr_screen.res][0]);
-}
+}
 
+//CommanderDJ - helper function for initialising intercom sounds vectors based on number of sounds
+//To be called after num_intercom_sounds has been parsed
+void intercom_sounds_init(main_hall_defines &m)
+{
+ if(!m.intercom_delay.empty())
+ {
+ //if one of these isn't empty, none of these should be empty
+ Assert(!m.intercom_sounds.empty());
+ Assert(!m.intercom_sound_pan.empty());
+
+ //since we could be reparsing with a different number of intercom sounds, clear these and reinitialise
+ m.intercom_delay.clear();
+ m.intercom_sounds.clear();
+ m.intercom_sound_pan.clear();
+ }
+
+ for(int idx=0; idx<m.num_random_intercom_sounds; idx++)
+ {
+ SCP_vector<int> temp;
+ m.intercom_delay.push_back(temp);
+
+ //for each delay we want space for two numbers - a min and a max
+ m.intercom_delay.at(idx).push_back(0);
+ m.intercom_delay.at(idx).push_back(0);
+
+ m.intercom_sounds.push_back(-1);
+
+ m.intercom_sound_pan.push_back(0);
+ }
+}
+
 // read in main hall table
 void main_hall_read_table()
 {
@@ -1652,20 +1685,24 @@
  Warning(LOCATION, "Num Intercom Sounds exceeds maximum!");
  m->num_random_intercom_sounds = MAX_RANDOM_INTERCOM_SOUNDS;
  }
+
+ //initialise intercom sounds vectors
+ intercom_sounds_init(*m);
+
  for(idx=0; idx<m->num_random_intercom_sounds; idx++){
  // intercom delay
  required_string("+Intercom delay:");
- stuff_int(&m->intercom_delay[idx][0]);
- stuff_int(&m->intercom_delay[idx][1]);
+ stuff_int(&m->intercom_delay.at(idx).at(0));
+ stuff_int(&m->intercom_delay.at(idx).at(1));
  }
  for(idx=0; idx<m->num_random_intercom_sounds; idx++){
  // intercom sound id
- parse_sound("+Intercom sound:", &m->intercom_sounds[idx], "+Intercom sound:", PARSE_SOUND_INTERFACE_SOUND);
+ parse_sound("+Intercom sound:", &m->intercom_sounds.at(idx), "+Intercom sound:", PARSE_SOUND_INTERFACE_SOUND);
  }
  for(idx=0; idx<m->num_random_intercom_sounds; idx++){
  // intercom pan
  required_string("+Intercom pan:");
- stuff_float(&m->intercom_sound_pan[idx]);
+ stuff_float(&m->intercom_sound_pan.at(idx));
  }
 
  // misc animations
Index: code/menuui/mainhallmenu.h
===================================================================
--- code/menuui/mainhallmenu.h (revision 8092)
+++ code/menuui/mainhallmenu.h (working copy)
@@ -23,6 +23,7 @@
 
 // do a frame for the main hall
 void main_hall_do(float frametime);
+
 // close the main hall proper
 void main_hall_close();
 

Once again, apologies to anyone who was affected by the bug I introduced. I hope you can forgive me :).

P.S. Zacam, if you prefer my patches to be attached, you could have just told me :P.

[attachment deleted by a basterd]
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on December 14, 2011, 08:59:30 am
The intercom stuff still seems to work as I'd expect.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 15, 2011, 05:40:37 am
The intercom stuff still seems to work as I'd expect.

Good to hear. I'm technically not adding new features so it should work exactly as before. Unfortunately my progress may be stalled for a day or two as my system is facing imminent hard drive failure, so until I get that sorted out it's unlikely I'll be doing much coding. In the meantime, can any coder who's reading this thread please commit the bugfix above? I'm hating the idea that the latest trunk build is still affected by this.

EDIT: Stopgap fix kindly committed by Zacam in trunk revision 8093.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 16, 2011, 09:16:15 pm
Update!

I have vectorised more of the mainhall structures: all of the intercom stuff and most of the misc anim stuff.
Code: [Select]
Index: code/menuui/mainhallmenu.cpp
===================================================================
--- code/menuui/mainhallmenu.cpp (revision 8095)
+++ code/menuui/mainhallmenu.cpp (working copy)
@@ -72,13 +72,13 @@
  int num_random_intercom_sounds;
 
  // random (min/max) delays between playing intercom sounds
- int intercom_delay[MAX_RANDOM_INTERCOM_SOUNDS][2];
+ SCP_vector<SCP_vector<int> > intercom_delay;
 
  // intercom sounds themselves
- int intercom_sounds[MAX_RANDOM_INTERCOM_SOUNDS];
+ SCP_vector<int> intercom_sounds;
 
  // intercom sound pan values
- float intercom_sound_pan[MAX_RANDOM_INTERCOM_SOUNDS];
+ SCP_vector<float> intercom_sound_pan;
 
 
  // misc animations --------------------
@@ -90,22 +90,22 @@
  char misc_anim_name[MAX_MISC_ANIMATIONS][MAX_FILENAME_LEN];
 
  // Time until we will next play a given misc animation, min delay, and max delay
- int misc_anim_delay[MAX_MISC_ANIMATIONS][3];
+ SCP_vector<SCP_vector<int> > misc_anim_delay;
 
  // Goober5000, used in preference to the flag in generic_anim
- int misc_anim_paused[MAX_MISC_ANIMATIONS];
+ SCP_vector<int> misc_anim_paused;
 
  // Goober5000, used when we want to play one of several anims
- int misc_anim_group[MAX_MISC_ANIMATIONS];
+ SCP_vector<int> misc_anim_group;
 
  // coords of where to play the misc anim
- int misc_anim_coords[MAX_MISC_ANIMATIONS][2];
+ SCP_vector<SCP_vector<int> > misc_anim_coords;
 
  // misc anim play modes (see MISC_ANIM_MODE_* above)
- int misc_anim_modes[MAX_MISC_ANIMATIONS];
+ SCP_vector<int> misc_anim_modes;
 
  // panning values for each of the misc anims
- float misc_anim_sound_pan[MAX_MISC_ANIMATIONS];
+ SCP_vector<float> misc_anim_sound_pan;
 
  //sounds for each of the misc anims
  SCP_vector<SCP_vector<int> > misc_anim_special_sounds;
@@ -581,15 +581,15 @@
  }
  else {
  //start paused
- if(Main_hall->misc_anim_modes[idx] == MISC_ANIM_MODE_HOLD)
+ if(Main_hall->misc_anim_modes.at(idx) == MISC_ANIM_MODE_HOLD)
  Main_hall_misc_anim[idx].direction |= GENERIC_ANIM_DIRECTION_NOLOOP;
  }
 
  // null out the delay timestamps
- Main_hall->misc_anim_delay[idx][0] = -1;
+ Main_hall->misc_anim_delay.at(idx).at(0) = -1;
 
  // start paused
- Main_hall->misc_anim_paused[idx] = true;
+ Main_hall->misc_anim_paused.at(idx) = true;
  }
 
  // load up the door animations
@@ -1121,13 +1121,13 @@
  // render it
  if (Main_hall_misc_anim[idx].num_frames > 0) {
  // animation is paused
- if (Main_hall->misc_anim_paused[idx]) {
+ if (Main_hall->misc_anim_paused.at(idx)) {
  // if the timestamp is -1, then regenerate it
- if (Main_hall->misc_anim_delay[idx][0] == -1) {
+ if (Main_hall->misc_anim_delay.at(idx).at(0) == -1) {
  int regen_idx = -1;
 
  // if this is part of a group, we should do additional checking
- if (Main_hall->misc_anim_group[idx] >= 0) {
+ if (Main_hall->misc_anim_group.at(idx) >= 0) {
  // make sure we haven't already checked it
  if (!(group_anims_weve_checked & (1<<idx))) {
  int group_indexes[MAX_MISC_ANIMATIONS];
@@ -1136,12 +1136,12 @@
 
  // okay... now we need to make sure all anims in this group are paused and -1
  for (jdx = 0; jdx < Main_hall->num_misc_animations; jdx++) {
- if (Main_hall->misc_anim_group[jdx] == Main_hall->misc_anim_group[idx]) {
+ if (Main_hall->misc_anim_group.at(jdx) == Main_hall->misc_anim_group.at(idx)) {
  group_anims_weve_checked |= (1<<jdx);
  group_indexes[num_group_indexes] = jdx;
  num_group_indexes++;
 
- if (!Main_hall->misc_anim_paused[jdx] || Main_hall->misc_anim_delay[jdx][0] != -1) {
+ if (!Main_hall->misc_anim_paused.at(jdx) || Main_hall->misc_anim_delay.at(jdx).at(0) != -1) {
  all_neg1 = false;
  }
  }
@@ -1160,19 +1160,19 @@
 
  // reset it to some random value (based on MIN and MAX) and continue
  if (regen_idx >= 0) {
- int min = Main_hall->misc_anim_delay[regen_idx][1];
- int max = Main_hall->misc_anim_delay[regen_idx][2];
- Main_hall->misc_anim_delay[regen_idx][0] = timestamp(min + (int) (frand() * (max - min)));
+ int min = Main_hall->misc_anim_delay.at(regen_idx).at(1);
+ int max = Main_hall->misc_anim_delay.at(regen_idx).at(2);
+ Main_hall->misc_anim_delay.at(regen_idx).at(0) = timestamp(min + (int) (frand() * (max - min)));
  }
 
  // if the timestamp is not -1 and has popped, play the anim and make the timestamp -1
- } else if (timestamp_elapsed(Main_hall->misc_anim_delay[idx][0])) {
- Main_hall->misc_anim_paused[idx] = false;
+ } else if (timestamp_elapsed(Main_hall->misc_anim_delay.at(idx).at(0))) {
+ Main_hall->misc_anim_paused.at(idx) = false;
  Main_hall_misc_anim[idx].current_frame = 0;
  Main_hall_misc_anim[idx].anim_time = 0.0;
 
  // kill the timestamp
- Main_hall->misc_anim_delay[idx][0] = -1;
+ Main_hall->misc_anim_delay.at(idx).at(0) = -1;
 
  // reset the "should be playing" flags
  for (s_idx=1;s_idx<10;s_idx++) {
@@ -1195,23 +1195,23 @@
  }
 
  // play the sound
- Main_hall->misc_anim_sound_handles[idx][s_idx+1] = snd_play(&Snds_iface[Main_hall->misc_anim_special_sounds.at(idx).at(s_idx)],Main_hall->misc_anim_sound_pan[idx]);
+ Main_hall->misc_anim_sound_handles[idx][s_idx+1] = snd_play(&Snds_iface[Main_hall->misc_anim_special_sounds.at(idx).at(s_idx)],Main_hall->misc_anim_sound_pan.at(idx));
  break;
  }
  }
 
  // animation has reached the last frame
  if (Main_hall_misc_anim[idx].current_frame == Main_hall_misc_anim[idx].num_frames - 1) {
- Main_hall->misc_anim_delay[idx][0] = -1;
+ Main_hall->misc_anim_delay.at(idx).at(0) = -1;
 
  //this helps the above code reset the timers
  //MISC_ANIM_MODE_HOLD simply stops on the last frame, so we don't care
  //MISC_ANIM_MODE_LOOPED just loops so we don't care either
- if (Main_hall->misc_anim_modes[idx] == MISC_ANIM_MODE_TIMED) {
- Main_hall->misc_anim_paused[idx] = true;
+ if (Main_hall->misc_anim_modes.at(idx) == MISC_ANIM_MODE_TIMED) {
+ Main_hall->misc_anim_paused.at(idx) = true;
  }
  //don't reset sound for MISC_ANIM_MODE_HOLD
- if (Main_hall->misc_anim_modes[idx] != MISC_ANIM_MODE_HOLD) {
+ if (Main_hall->misc_anim_modes.at(idx) != MISC_ANIM_MODE_HOLD) {
  // reset the "should be playing" flags
  for (s_idx=1;s_idx<10;s_idx++) {
  Main_hall->misc_anim_sound_flag[idx][s_idx] = 0;
@@ -1223,7 +1223,7 @@
  if (Main_hall_frame_skip || Main_hall_paused) {
  frametime = 0;
  }
- generic_anim_render(&Main_hall_misc_anim[idx], frametime, Main_hall->misc_anim_coords[idx][0], Main_hall->misc_anim_coords[idx][1]);
+ generic_anim_render(&Main_hall_misc_anim[idx], frametime, Main_hall->misc_anim_coords.at(idx).at(0), Main_hall->misc_anim_coords.at(idx).at(1));
  }
  }
  }
@@ -1396,8 +1396,8 @@
  // if we have no timestamp for the next random sound, then set on
  if((Main_hall_next_intercom_sound_stamp == -1) && (Main_hall_intercom_sound_handle == -1)){
  Main_hall_next_intercom_sound_stamp = timestamp((int)(((float)rand()/(float)RAND_MAX) *
-                                             (float)(Main_hall->intercom_delay[Main_hall_next_intercom_sound][1]
-   - Main_hall->intercom_delay[Main_hall_intercom_sound_handle][0])) );
+                                             (float)(Main_hall->intercom_delay.at(Main_hall_next_intercom_sound).at(1)
+   - Main_hall->intercom_delay.at(Main_hall_next_intercom_sound).at(0))) );
  }
 
  // if the there is no sound playing
@@ -1409,7 +1409,7 @@
  // if the timestamp has popped, play a sound
  if((Main_hall_next_intercom_sound_stamp != -1) && (timestamp_elapsed(Main_hall_next_intercom_sound_stamp))){
  // play the sound
- Main_hall_intercom_sound_handle = snd_play(&Snds_iface[Main_hall->intercom_sounds[Main_hall_next_intercom_sound]]);
+ Main_hall_intercom_sound_handle = snd_play(&Snds_iface.at(Main_hall->intercom_sounds.at(Main_hall_next_intercom_sound)));
 
  // unset the timestamp
  Main_hall_next_intercom_sound_stamp = -1;
@@ -1428,8 +1428,8 @@
 
  // set the timestamp
  Main_hall_next_intercom_sound_stamp = timestamp((int)(((float)rand()/(float)RAND_MAX) *
-                                             (float)(Main_hall->intercom_delay[Main_hall_next_intercom_sound][1]
-   - Main_hall->intercom_delay[Main_hall_next_intercom_sound][0])) );
+                                             (float)(Main_hall->intercom_delay.at(Main_hall_next_intercom_sound).at(1)
+   - Main_hall->intercom_delay.at(Main_hall_next_intercom_sound).at(0))) );
 
  // release the sound handle
  Main_hall_intercom_sound_handle = -1;
@@ -1595,8 +1595,96 @@
 int main_hall_id()
 {
  return (Main_hall - &Main_hall_defines[gr_screen.res][0]);
-}
+}
 
+//CommanderDJ - helper function for initialising intercom sounds vectors based on number of sounds
+//To be called after num_intercom_sounds has been parsed
+void intercom_sounds_init(main_hall_defines &m)
+{
+ if(!m.intercom_delay.empty())
+ {
+ //if one of these isn't empty, none of these should be empty
+ Assert(!m.intercom_sounds.empty());
+ Assert(!m.intercom_sound_pan.empty());
+
+ //since we could be reparsing with a different number of intercom sounds, clear these and reinitialise
+ m.intercom_delay.clear();
+ m.intercom_sounds.clear();
+ m.intercom_sound_pan.clear();
+ }
+
+ SCP_vector<int> temp;
+
+ for(int idx=0; idx<m.num_random_intercom_sounds; idx++)
+ {
+ //intercom_delay
+ m.intercom_delay.push_back(temp);
+
+ //each delay has a min and a max
+ m.intercom_delay.at(idx).push_back(0);
+ m.intercom_delay.at(idx).push_back(0);
+
+ //intercom_sounds
+ m.intercom_sounds.push_back(-1);
+
+ //intercom_sound_pan
+ m.intercom_sound_pan.push_back(0);
+ }
+}
+
+void misc_anim_init(main_hall_defines &m)
+{
+ if(!m.misc_anim_delay.empty())
+ {
+ //if one of these isn't empty, none of these should be
+ Assert(!m.misc_anim_paused.empty());
+ Assert(!m.misc_anim_group.empty());
+ Assert(!m.misc_anim_coords.empty());
+ Assert(!m.misc_anim_modes.empty());
+ Assert(!m.misc_anim_sound_pan.empty());
+
+
+ //since we could be reparsing with a different number of misc anims, clear these and reinitialise
+ m.misc_anim_delay.clear();
+ m.misc_anim_paused.clear();
+ m.misc_anim_group.clear();
+ m.misc_anim_coords.clear();
+ m.misc_anim_modes.clear();
+ m.misc_anim_sound_pan.clear();
+ }
+
+ SCP_vector<int> temp;
+
+ for(int idx=0; idx<m.num_misc_animations; idx++)
+ {
+ //misc_anim_delay
+ m.misc_anim_delay.push_back(temp);
+
+ //-1 default for the first entry, 0 for the others
+ m.misc_anim_delay.at(idx).push_back(-1);
+ m.misc_anim_delay.at(idx).push_back(0);
+ m.misc_anim_delay.at(idx).push_back(0);
+
+ //misc_anim_paused
+ m.misc_anim_paused.push_back(1); //default is paused
+
+ //misc_anim_group
+ m.misc_anim_group.push_back(-1);
+
+ //misc_anim_coords
+ m.misc_anim_coords.push_back(temp);
+
+ m.misc_anim_coords.at(idx).push_back(0);
+ m.misc_anim_coords.at(idx).push_back(0);
+
+ //misc_anim_modes
+ m.misc_anim_modes.push_back(MISC_ANIM_MODE_LOOP);
+
+ //misc_anim_sound_pan
+ m.misc_anim_sound_pan.push_back(0);
+ }
+}
+
 // read in main hall table
 void main_hall_read_table()
 {
@@ -1652,20 +1740,24 @@
  Warning(LOCATION, "Num Intercom Sounds exceeds maximum!");
  m->num_random_intercom_sounds = MAX_RANDOM_INTERCOM_SOUNDS;
  }
+
+ //initialise intercom sounds vectors
+ intercom_sounds_init(*m);
+
  for(idx=0; idx<m->num_random_intercom_sounds; idx++){
  // intercom delay
  required_string("+Intercom delay:");
- stuff_int(&m->intercom_delay[idx][0]);
- stuff_int(&m->intercom_delay[idx][1]);
+ stuff_int(&m->intercom_delay.at(idx).at(0));
+ stuff_int(&m->intercom_delay.at(idx).at(1));
  }
  for(idx=0; idx<m->num_random_intercom_sounds; idx++){
  // intercom sound id
- parse_sound("+Intercom sound:", &m->intercom_sounds[idx], "+Intercom sound:", PARSE_SOUND_INTERFACE_SOUND);
+ parse_sound("+Intercom sound:", &m->intercom_sounds.at(idx), "+Intercom sound:", PARSE_SOUND_INTERFACE_SOUND);
  }
  for(idx=0; idx<m->num_random_intercom_sounds; idx++){
  // intercom pan
  required_string("+Intercom pan:");
- stuff_float(&m->intercom_sound_pan[idx]);
+ stuff_float(&m->intercom_sound_pan.at(idx));
  }
 
  // misc animations
@@ -1675,6 +1767,10 @@
  Warning(LOCATION, "Num Misc Animations exceeds maximum!");
  m->num_misc_animations = MAX_MISC_ANIMATIONS;
  }
+
+ //initialise the misc anim vectors
+ misc_anim_init(*m);
+
  for(idx=0; idx<m->num_misc_animations; idx++){
  // anim names
  required_string("+Misc anim:");
@@ -1683,33 +1779,33 @@
  for(idx=0; idx<m->num_misc_animations; idx++){
  // anim groups, optionally
  if (optional_string("+Misc anim group:")){
- stuff_int(&m->misc_anim_group[idx]);
+ stuff_int(&m->misc_anim_group.at(idx));
  } else {
- m->misc_anim_group[idx] = -1;
+ m->misc_anim_group.at(idx) = -1;
  }
  }
  for(idx=0; idx<m->num_misc_animations; idx++){
  // anim delay
  required_string("+Misc anim delay:");
- stuff_int(&m->misc_anim_delay[idx][0]);
- stuff_int(&m->misc_anim_delay[idx][1]);
- stuff_int(&m->misc_anim_delay[idx][2]);
+ stuff_int(&m->misc_anim_delay.at(idx).at(0));
+ stuff_int(&m->misc_anim_delay.at(idx).at(1));
+ stuff_int(&m->misc_anim_delay.at(idx).at(2));
  }
  for(idx=0; idx<m->num_misc_animations; idx++){
  // anim coords
  required_string("+Misc anim coords:");
- stuff_int(&m->misc_anim_coords[idx][0]);
- stuff_int(&m->misc_anim_coords[idx][1]);
+ stuff_int(&m->misc_anim_coords.at(idx).at(0));
+ stuff_int(&m->misc_anim_coords.at(idx).at(1));
  }
  for(idx=0; idx<m->num_misc_animations; idx++){
  // anim mode
  required_string("+Misc anim mode:");
- stuff_int(&m->misc_anim_modes[idx]);
+ stuff_int(&m->misc_anim_modes.at(idx));
  }
  for(idx=0; idx<m->num_misc_animations; idx++){
  // anim pan
  required_string("+Misc anim pan:");
- stuff_float(&m->misc_anim_sound_pan[idx]);
+ stuff_float(&m->misc_anim_sound_pan.at(idx));
  }
  for(idx=0; idx<m->num_misc_animations; idx++){
  // anim sound id
Index: code/menuui/mainhallmenu.h
===================================================================
--- code/menuui/mainhallmenu.h (revision 8095)
+++ code/menuui/mainhallmenu.h (working copy)
@@ -23,6 +23,7 @@
 
 // do a frame for the main hall
 void main_hall_do(float frametime);
+
 // close the main hall proper
 void main_hall_close();
 

Please apply this to the latest trunk revision (note that this patch overrides the previous intercom vectorisation patch, so please do not have that one applied). This should be tested both with and without the -reparse_mainhall flag. If something goes wrong it'll most likely be pretty obvious (ie the game will assert or throw an exception and crash).

The next patch will include parsing changes (but not table syntax changes) so I'd prefer to get this one committed before the next one surfaces. Thanks to everyone for your patience and in particular to mjn.mixael for his testing!
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on December 16, 2011, 09:22:49 pm
No issues found.
Title: Re: Modular Mainhall.tbl
Post by: Echelon9 on December 21, 2011, 08:52:49 am
Commander, regarding your specific query on min/max naming cross-platform, the issues we've seen previously only relate to the use of the min() and max() functions --- these need to be fully capitalised to work correctly cross platform.

Having variables names min or max doesn't create issues as far as I know.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 21, 2011, 07:58:14 pm
Thank you for the quick response, Echelon9.

The vectorisation patch above has been committed to trunk by Zacam in revision 8106.

It is unlikely I will make further progress until after Christmas, but as always, any updates will be posted here.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 24, 2011, 10:52:18 pm
Hey guys,

Merry Christmas! Just a quick status update to show that I have not stopped working on this and a few questions. So, today I worked more on vectorising stuff, I'm almost done with the misc anim data structures. Once those are finished off I'll post a patch for testing. After that all that will be left to do are the door anim structures and the misc_anim_name structure (which is a two dimensional char array). With luck, I hope to have the vectorisation complete by the new year.

Question for mjn.mixael and others: would there be any demand/use for a Misc Anim Trigger New flag as well as a Misc Anim Sounds New? These would both remove the need for having the first entry in the list be the number of entries following. I do have plans to implement Sounds New but was wondering what the interest would be like for Trigger New.

Question for coders: either the comments in the code are incorrect or I'm not reading something right: the comments for the misc_anim_sound_handles and misc_anim_sound_flag arrays say that the first element in each is the number of handles, yet every mainhall.tbl I've seen only has one entry for each of those, and indeed the parsing code itself only parses one entry per flag. What gives? Also, if indeed we only need one entry per flag, why are two dimensional arrays used for those? Seems unnecessary to me, so I'd like that cleared up so that if there's a waste of space happening there I can rectify it.
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on December 24, 2011, 11:13:31 pm
New flags aren't really very necessary, imo. the syntax of the originals isn't hard to understand. however, if you wanted to add the be flags, I'd them.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 26, 2011, 05:26:48 am
Update!

Several changes in this one: more vectorisation (finished off the misc anim structures, only the door anims left), but that's not the exciting part. The exciting part is that I have completely removed the need for the +Misc Anim Handles and +Misc Anim Flags table flags. Handles have been completely ripped out and aren't used at all, where as Flags are all handled internally by the code. Obviously it's backwards compatible so that retail tables will still work (the game ignores user input for those two flags now), but you no longer need to include those flags in mainhall.tbl with this patch.

Also I've tweaked some of the initialisation code to tie it more closely to the -reparse_mainhall flag (since that's the only time those parts of the code were used).

Please test! Apply straight to trunk, try it without the Flags and Handles table entries, try it with and without -reparse_mainhall. Post any results here.

Code: [Select]
Index: code/menuui/mainhallmenu.cpp
===================================================================
--- code/menuui/mainhallmenu.cpp (revision 8113)
+++ code/menuui/mainhallmenu.cpp (working copy)
@@ -110,16 +110,13 @@
  //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];
+ //frame number triggers for each of the misc anims
+ SCP_vector<SCP_vector<int> > misc_anim_special_trigger;
 
- // [N][0] == # of handles, [N][1-9] == sound handle num
- int misc_anim_sound_handles[MAX_MISC_ANIMATIONS][10];
+ //flags for each of the misc anim sounds
+ SCP_vector<SCP_vector<int> > misc_anim_sound_flag;
 
- // [N][0] == # of handles, [N][1-9] == sound "should" be playing
- int misc_anim_sound_flag[MAX_MISC_ANIMATIONS][10];
 
-
  // door animations --------------------
 
  // # of door animations
@@ -503,7 +500,7 @@
  return;
  }
 
- int idx,s_idx;
+ int idx;
  char temp[100], whee[100];
 
  //reparse the table here if the relevant cmdline flag is set
@@ -639,14 +636,6 @@
  Main_hall_door_sound_handles[idx] = -1;
  }
 
- // zero out the misc anim sounds
- for (idx=0;idx<Main_hall->num_misc_animations;idx++) {
- for (s_idx = 1;s_idx < 10;s_idx++) {
- Main_hall->misc_anim_sound_handles[idx][s_idx] = -1;
- Main_hall->misc_anim_sound_flag[idx][s_idx] = 0;
- }
- }
-
  // skip the first frame
  Main_hall_frame_skip = 1;
 
@@ -1009,10 +998,9 @@
 
  // stop any playing misc animation sounds
  for (idx=0;idx<Main_hall->num_misc_animations;idx++) {
- for (s_idx=1;s_idx<10;s_idx++) {
- if (snd_is_playing(Main_hall->misc_anim_sound_handles[idx][s_idx])) {
- snd_stop(Main_hall->misc_anim_sound_handles[idx][s_idx]);
- Main_hall->misc_anim_sound_handles[idx][s_idx] = -1;
+ for (s_idx=0;(unsigned)s_idx<Main_hall->misc_anim_special_sounds.at(idx).size();s_idx++) {
+ if (snd_is_playing(Main_hall->misc_anim_special_sounds.at(idx).at(s_idx))) {
+ snd_stop(Main_hall->misc_anim_special_sounds.at(idx).at(s_idx));
  }
  }
  }
@@ -1175,28 +1163,29 @@
  // kill the timestamp
  Main_hall->misc_anim_delay.at(idx).at(0) = -1;
 
+
  // reset the "should be playing" flags
- for (s_idx=1;s_idx<10;s_idx++) {
- Main_hall->misc_anim_sound_flag[idx][s_idx] = 0;
+ for (s_idx=0;(unsigned)s_idx<Main_hall->misc_anim_sound_flag.at(idx).size();s_idx++) {
+ Main_hall->misc_anim_sound_flag.at(idx).at(s_idx) = 0;
  }
+
  }
  }
  // animation is not paused
  else {
  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+1]) && !Main_hall->misc_anim_sound_flag[idx][s_idx+1]) {
- Main_hall->misc_anim_sound_flag[idx][s_idx+1] = 1;
+ if ((Main_hall_misc_anim[idx].current_frame >= Main_hall->misc_anim_special_trigger.at(idx).at(s_idx)) && !Main_hall->misc_anim_sound_flag.at(idx).at(s_idx)) {
+ Main_hall->misc_anim_sound_flag.at(idx).at(s_idx) = 1;
 
  // if the sound is already playing, then kill it. This is a pretty safe thing to do since we can assume that
  // by the time we get to this point again, the sound will have been long finished
- if (snd_is_playing(Main_hall->misc_anim_sound_handles[idx][s_idx+1])) {
- snd_stop(Main_hall->misc_anim_sound_handles[idx][s_idx+1]);
- Main_hall->misc_anim_sound_handles[idx][s_idx+1] = -1;
+ if (snd_is_playing(Main_hall->misc_anim_special_sounds.at(idx).at(s_idx))) {
+ snd_stop(Main_hall->misc_anim_special_sounds.at(idx).at(s_idx));
  }
 
  // play the sound
- Main_hall->misc_anim_sound_handles[idx][s_idx+1] = snd_play(&Snds_iface[Main_hall->misc_anim_special_sounds.at(idx).at(s_idx)],Main_hall->misc_anim_sound_pan.at(idx));
+ snd_play(&Snds_iface[Main_hall->misc_anim_special_sounds.at(idx).at(s_idx)],Main_hall->misc_anim_sound_pan.at(idx));
  break;
  }
  }
@@ -1214,8 +1203,8 @@
  //don't reset sound for MISC_ANIM_MODE_HOLD
  if (Main_hall->misc_anim_modes.at(idx) != MISC_ANIM_MODE_HOLD) {
  // reset the "should be playing" flags
- for (s_idx=1;s_idx<10;s_idx++) {
- Main_hall->misc_anim_sound_flag[idx][s_idx] = 0;
+ for (s_idx=0;(unsigned)s_idx<Main_hall->misc_anim_sound_flag.at(idx).size();s_idx++) {
+ Main_hall->misc_anim_sound_flag.at(idx).at(s_idx) = 0;
  }
  }
  }
@@ -1607,12 +1596,8 @@
 //To be called after num_intercom_sounds has been parsed
 void intercom_sounds_init(main_hall_defines &m)
 {
- if (!m.intercom_delay.empty()) {
- //if one of these isn't empty, none of these should be empty
- Assert(!m.intercom_sounds.empty());
- Assert(!m.intercom_sound_pan.empty());
-
- //since we could be reparsing with a different number of intercom sounds, clear these and reinitialise
+ if (Cmdline_reparse_mainhall) {
+ //we could be reparsing with a different number of intercom sounds, so clear these and reinitialise
  m.intercom_delay.clear();
  m.intercom_sounds.clear();
  m.intercom_sound_pan.clear();
@@ -1638,21 +1623,17 @@
 
 void misc_anim_init(main_hall_defines &m)
 {
- if (!m.misc_anim_delay.empty()) {
- //if one of these isn't empty, none of these should be
- Assert(!m.misc_anim_paused.empty());
- Assert(!m.misc_anim_group.empty());
- Assert(!m.misc_anim_coords.empty());
- Assert(!m.misc_anim_modes.empty());
- Assert(!m.misc_anim_sound_pan.empty());
-
- //since we could be reparsing with a different number of misc anims, clear these and reinitialise
+ if (Cmdline_reparse_mainhall) {
+ //we could be reparsing with a different number of intercom sounds, so clear these and reinitialise
  m.misc_anim_delay.clear();
  m.misc_anim_paused.clear();
  m.misc_anim_group.clear();
  m.misc_anim_coords.clear();
  m.misc_anim_modes.clear();
  m.misc_anim_sound_pan.clear();
+ m.misc_anim_special_sounds.clear();
+ m.misc_anim_special_trigger.clear();
+ m.misc_anim_sound_flag.clear();
  }
 
  SCP_vector<int> temp;
@@ -1683,6 +1664,17 @@
 
  //misc_anim_sound_pan
  m.misc_anim_sound_pan.push_back(0);
+
+ //misc_anim_special_sounds
+ m.misc_anim_special_sounds.push_back(temp); //parse_sound_list deals with the rest of the initialisation for this one
+
+ //misc_anim_special_trigger
+ m.misc_anim_special_trigger.push_back(temp);
+
+ m.misc_anim_special_trigger.at(idx).push_back(0);
+
+ //misc_anim_sound_flag
+ m.misc_anim_sound_flag.push_back(temp);
  }
 }
 
@@ -1819,30 +1811,38 @@
 
  for (idx=0; idx<m->num_misc_animations; idx++) {
  // 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);
  }
 
  for (idx=0; idx<m->num_misc_animations; idx++) {
  // anim sound triggers
  required_string("+Misc anim trigger:");
- stuff_int(&m->misc_anim_special_trigger[idx][0]);
- for (s_idx=0; s_idx<m->misc_anim_special_trigger[idx][0]; s_idx++) {
- stuff_int(&m->misc_anim_special_trigger[idx][s_idx + 1]);
+ int temp = 0;
+ stuff_int(&temp);
+ for (s_idx=0; s_idx<temp; s_idx++) {
+ m->misc_anim_special_trigger.at(idx).push_back(0);
+ stuff_int(&m->misc_anim_special_trigger.at(idx).at(s_idx));
  }
  }
 
  for (idx=0; idx<m->num_misc_animations; idx++) {
- // anim sound handles
- required_string("+Misc anim handles:");
- stuff_int(&m->misc_anim_sound_handles[idx][0]);
+ // anim sound handles - deprecated, but deal with it just in case
+ if(optional_string("+Misc anim handles:")) {
+ advance_to_eoln(NULL);
+ }
+
  }
 
  for (idx=0; idx<m->num_misc_animations; idx++) {
- // anim sound flags
- required_string("+Misc anim flags:");
- stuff_int(&m->misc_anim_sound_flag[idx][0]);
+ // anim sound flags - table flag deprecated, so ignore user input
+ if(optional_string("+Misc anim flags:")) {
+ advance_to_eoln(NULL);
+ }
+
+ //we need one flag for each sound
+ for(s_idx=0; (unsigned) s_idx<m->misc_anim_special_sounds.at(idx).size(); s_idx++) {
+ m->misc_anim_sound_flag.at(idx).push_back(0);
+ }
  }
 
  // door animations
Title: Re: Modular Mainhall.tbl
Post by: Iss Mneur on December 26, 2011, 12:20:32 pm
I haven't tried it, but a couple of notes:

Code: [Select]
(unsigned)s_idx<Main_hall->misc_anim_sound_flag.at(idx).size() and other similar lines should be pointless.  Size() returns a size_t which is already unsigned, so casting it again should be unnessary.  What is more likely is you need to cast it to (int) becuase s_idx is also int.

Second thing, is there a specific reason that you are still using indexes at all (especially for loops). Why not use iterators?
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 26, 2011, 06:01:15 pm
I have fixed the casting.

Regarding iterators, there are definitely places I could use them, but I do have reasons not to. Firstly, the entire reason I've been changing all the square bracket indexing to .at() as I've been vectorising is the automatic bounds checking it includes. Switching to iterators would put us right back to square one in that regard and would allow out-of-bounds access without throwing an exception; something I strongly want to avoid as exceptions very clearly identify what's gone wrong. Secondly, in many places the same index is used to access different vectors in the same loop. Iterators would not allow us to do that, and would require a fair bit more work to get going properly. Thirdly, some of the relevant code still uses arrays (both from and not from main_hall_defines) which are indexed by square brackets. Whilst I will reduce the number of arrays as I finish off vectorising, some of them are not part of main_hall_defines and thus will remain arrays when I'm done. Switching to iterators would still require them to be indexed by square brackets, again resulting in more work for (IMHO) negligible gain.

EDIT: and here's the patch with the fixed casting:

Code: [Select]
Index: code/menuui/mainhallmenu.cpp
===================================================================
--- code/menuui/mainhallmenu.cpp (revision 8113)
+++ code/menuui/mainhallmenu.cpp (working copy)
@@ -110,16 +110,13 @@
  //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];
+ //frame number triggers for each of the misc anims
+ SCP_vector<SCP_vector<int> > misc_anim_special_trigger;
 
- // [N][0] == # of handles, [N][1-9] == sound handle num
- int misc_anim_sound_handles[MAX_MISC_ANIMATIONS][10];
+ //flags for each of the misc anim sounds
+ SCP_vector<SCP_vector<int> > misc_anim_sound_flag;
 
- // [N][0] == # of handles, [N][1-9] == sound "should" be playing
- int misc_anim_sound_flag[MAX_MISC_ANIMATIONS][10];
 
-
  // door animations --------------------
 
  // # of door animations
@@ -503,7 +500,7 @@
  return;
  }
 
- int idx,s_idx;
+ int idx;
  char temp[100], whee[100];
 
  //reparse the table here if the relevant cmdline flag is set
@@ -639,14 +636,6 @@
  Main_hall_door_sound_handles[idx] = -1;
  }
 
- // zero out the misc anim sounds
- for (idx=0;idx<Main_hall->num_misc_animations;idx++) {
- for (s_idx = 1;s_idx < 10;s_idx++) {
- Main_hall->misc_anim_sound_handles[idx][s_idx] = -1;
- Main_hall->misc_anim_sound_flag[idx][s_idx] = 0;
- }
- }
-
  // skip the first frame
  Main_hall_frame_skip = 1;
 
@@ -1009,10 +998,9 @@
 
  // stop any playing misc animation sounds
  for (idx=0;idx<Main_hall->num_misc_animations;idx++) {
- for (s_idx=1;s_idx<10;s_idx++) {
- if (snd_is_playing(Main_hall->misc_anim_sound_handles[idx][s_idx])) {
- snd_stop(Main_hall->misc_anim_sound_handles[idx][s_idx]);
- Main_hall->misc_anim_sound_handles[idx][s_idx] = -1;
+ for (s_idx=0;s_idx<(int)Main_hall->misc_anim_special_sounds.at(idx).size();s_idx++) {
+ if (snd_is_playing(Main_hall->misc_anim_special_sounds.at(idx).at(s_idx))) {
+ snd_stop(Main_hall->misc_anim_special_sounds.at(idx).at(s_idx));
  }
  }
  }
@@ -1175,28 +1163,29 @@
  // kill the timestamp
  Main_hall->misc_anim_delay.at(idx).at(0) = -1;
 
+
  // reset the "should be playing" flags
- for (s_idx=1;s_idx<10;s_idx++) {
- Main_hall->misc_anim_sound_flag[idx][s_idx] = 0;
- }
+ for (s_idx=0;s_idx<(int)Main_hall->misc_anim_sound_flag.at(idx).size();s_idx++) {
+ Main_hall->misc_anim_sound_flag.at(idx).at(s_idx) = 0;
+ }
+
  }
  }
  // animation is not paused
  else {
- for (s_idx = 0; (unsigned)s_idx < Main_hall->misc_anim_special_sounds.at(idx).size(); s_idx++) {
+ for (s_idx = 0; s_idx < (int)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+1]) && !Main_hall->misc_anim_sound_flag[idx][s_idx+1]) {
- Main_hall->misc_anim_sound_flag[idx][s_idx+1] = 1;
+ if ((Main_hall_misc_anim[idx].current_frame >= Main_hall->misc_anim_special_trigger.at(idx).at(s_idx)) && !Main_hall->misc_anim_sound_flag.at(idx).at(s_idx)) {
+ Main_hall->misc_anim_sound_flag.at(idx).at(s_idx) = 1;
 
  // if the sound is already playing, then kill it. This is a pretty safe thing to do since we can assume that
  // by the time we get to this point again, the sound will have been long finished
- if (snd_is_playing(Main_hall->misc_anim_sound_handles[idx][s_idx+1])) {
- snd_stop(Main_hall->misc_anim_sound_handles[idx][s_idx+1]);
- Main_hall->misc_anim_sound_handles[idx][s_idx+1] = -1;
+ if (snd_is_playing(Main_hall->misc_anim_special_sounds.at(idx).at(s_idx))) {
+ snd_stop(Main_hall->misc_anim_special_sounds.at(idx).at(s_idx));
  }
 
  // play the sound
- Main_hall->misc_anim_sound_handles[idx][s_idx+1] = snd_play(&Snds_iface[Main_hall->misc_anim_special_sounds.at(idx).at(s_idx)],Main_hall->misc_anim_sound_pan.at(idx));
+ snd_play(&Snds_iface[Main_hall->misc_anim_special_sounds.at(idx).at(s_idx)],Main_hall->misc_anim_sound_pan.at(idx));
  break;
  }
  }
@@ -1214,8 +1203,8 @@
  //don't reset sound for MISC_ANIM_MODE_HOLD
  if (Main_hall->misc_anim_modes.at(idx) != MISC_ANIM_MODE_HOLD) {
  // reset the "should be playing" flags
- for (s_idx=1;s_idx<10;s_idx++) {
- Main_hall->misc_anim_sound_flag[idx][s_idx] = 0;
+ for (s_idx=0;s_idx<(int)Main_hall->misc_anim_sound_flag.at(idx).size();s_idx++) {
+ Main_hall->misc_anim_sound_flag.at(idx).at(s_idx) = 0;
  }
  }
  }
@@ -1607,12 +1596,8 @@
 //To be called after num_intercom_sounds has been parsed
 void intercom_sounds_init(main_hall_defines &m)
 {
- if (!m.intercom_delay.empty()) {
- //if one of these isn't empty, none of these should be empty
- Assert(!m.intercom_sounds.empty());
- Assert(!m.intercom_sound_pan.empty());
-
- //since we could be reparsing with a different number of intercom sounds, clear these and reinitialise
+ if (Cmdline_reparse_mainhall) {
+ //we could be reparsing with a different number of intercom sounds, so clear these and reinitialise
  m.intercom_delay.clear();
  m.intercom_sounds.clear();
  m.intercom_sound_pan.clear();
@@ -1638,21 +1623,17 @@
 
 void misc_anim_init(main_hall_defines &m)
 {
- if (!m.misc_anim_delay.empty()) {
- //if one of these isn't empty, none of these should be
- Assert(!m.misc_anim_paused.empty());
- Assert(!m.misc_anim_group.empty());
- Assert(!m.misc_anim_coords.empty());
- Assert(!m.misc_anim_modes.empty());
- Assert(!m.misc_anim_sound_pan.empty());
-
- //since we could be reparsing with a different number of misc anims, clear these and reinitialise
+ if (Cmdline_reparse_mainhall) {
+ //we could be reparsing with a different number of intercom sounds, so clear these and reinitialise
  m.misc_anim_delay.clear();
  m.misc_anim_paused.clear();
  m.misc_anim_group.clear();
  m.misc_anim_coords.clear();
  m.misc_anim_modes.clear();
  m.misc_anim_sound_pan.clear();
+ m.misc_anim_special_sounds.clear();
+ m.misc_anim_special_trigger.clear();
+ m.misc_anim_sound_flag.clear();
  }
 
  SCP_vector<int> temp;
@@ -1683,6 +1664,17 @@
 
  //misc_anim_sound_pan
  m.misc_anim_sound_pan.push_back(0);
+
+ //misc_anim_special_sounds
+ m.misc_anim_special_sounds.push_back(temp); //parse_sound_list deals with the rest of the initialisation for this one
+
+ //misc_anim_special_trigger
+ m.misc_anim_special_trigger.push_back(temp);
+
+ m.misc_anim_special_trigger.at(idx).push_back(0);
+
+ //misc_anim_sound_flag
+ m.misc_anim_sound_flag.push_back(temp);
  }
 }
 
@@ -1819,30 +1811,38 @@
 
  for (idx=0; idx<m->num_misc_animations; idx++) {
  // 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);
  }
 
  for (idx=0; idx<m->num_misc_animations; idx++) {
  // anim sound triggers
  required_string("+Misc anim trigger:");
- stuff_int(&m->misc_anim_special_trigger[idx][0]);
- for (s_idx=0; s_idx<m->misc_anim_special_trigger[idx][0]; s_idx++) {
- stuff_int(&m->misc_anim_special_trigger[idx][s_idx + 1]);
+ int temp = 0;
+ stuff_int(&temp);
+ for (s_idx=0; s_idx<temp; s_idx++) {
+ m->misc_anim_special_trigger.at(idx).push_back(0);
+ stuff_int(&m->misc_anim_special_trigger.at(idx).at(s_idx));
  }
  }
 
  for (idx=0; idx<m->num_misc_animations; idx++) {
- // anim sound handles
- required_string("+Misc anim handles:");
- stuff_int(&m->misc_anim_sound_handles[idx][0]);
+ // anim sound handles - deprecated, but deal with it just in case
+ if(optional_string("+Misc anim handles:")) {
+ advance_to_eoln(NULL);
+ }
+
  }
 
  for (idx=0; idx<m->num_misc_animations; idx++) {
- // anim sound flags
- required_string("+Misc anim flags:");
- stuff_int(&m->misc_anim_sound_flag[idx][0]);
+ // anim sound flags - table flag deprecated, so ignore user input
+ if(optional_string("+Misc anim flags:")) {
+ advance_to_eoln(NULL);
+ }
+
+ //we need one flag for each sound
+ for(s_idx=0; s_idx<(int)m->misc_anim_special_sounds.at(idx).size(); s_idx++) {
+ m->misc_anim_sound_flag.at(idx).push_back(0);
+ }
  }
 
  // door animations
Title: Re: Modular Mainhall.tbl
Post by: Eli2 on December 26, 2011, 09:45:36 pm
How can an iterator even be out-of-bounds  :confused:
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 27, 2011, 01:28:24 am
You can do pointer arithmetic with iterators (since that's pretty much what they are). So if I had an iterator called v_iter I could do this:
Code: [Select]
std::vector<int>::iterator v_iter = array.begin();
v_iter = v_iter+3;

If the array in question had a size of less than 4, if I then tried to access the element at v_iter, I would be performing an out-of-bounds access operation.
Title: Re: Modular Mainhall.tbl
Post by: Iss Mneur on December 27, 2011, 01:09:12 pm
You can do pointer arithmetic with iterators (since that's pretty much what they are). So if I had an iterator called v_iter I could do this:
Code: [Select]
std::vector<int>::iterator v_iter = array.begin();
v_iter = v_iter+3;

If the array in question had a size of less than 4, if I then tried to access the element at v_iter, I would be performing an out-of-bounds access operation.
Okay, but why would you do such a thing?

I don't recall seeing any iteration in the mainhall.tbl that requires you to do more than adding one.

As for the rest of your points they are valid reasons for not using iterators, but I would still suggest that using iterators is a good idea because it would allow the simplification of some of the code.  Remember that mainhall.tbl is just an interface, it does not need to reflect how the data is stored, processed, or rendered by the engine.

For example, the parallel arrays that are used everywhere, could be replaced by a vector of a custom stuct, doing this would remove most of the need for indexes and when combined with the named items that you want to add later the mainhall code could be free of indexes all together.  Yes, I realize this is a large amount of work but even :v: shouldn't have used the parallel arrays to start with, becuase they unnecessarily complicate working on and modifying the data structures. 

Any way, just something to consider, because after all we are trying to reduce the code smell (http://www.codinghorror.com/blog/2006/05/code-smells.html) as well as add features.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 27, 2011, 07:25:23 pm
I understand. You do make some good points. However, would you allow me to finish off the vectorisation, have that committed, and then perhaps look at adding iterators later? It's technically a separate feature, so I'd like to work on it on it's own rather than doing at the same time as the vectors. It may mesh well with the naming later, so I might work on it at the same time there. I'd just like to finish the vectorisation before starting work on anything else is all.
Title: Re: Modular Mainhall.tbl
Post by: Iss Mneur on December 27, 2011, 07:43:17 pm
Sure, your the one writing the patches, I just don't want to see this turn into mess that is the autopilot code.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 28, 2011, 04:16:15 am
I am pleased to present the final vectorisation patch for main_hall_defines - every single array within the structure has been converted into a vector. I even went and got rid of the old and gross c-style strings and replaced them with SCP_strings.

I'd like to get this evaluated and committed as soon as possible, so please test! Apply straight to trunk (this supersedes the previous patch). Remember to try with -reparse_mainhall, and remember to test that +Misc anim handles and +Misc anim flags have been deprecated - omit them from your tables!

Code: [Select]
Index: code/menuui/mainhallmenu.cpp
===================================================================
--- code/menuui/mainhallmenu.cpp (revision 8113)
+++ code/menuui/mainhallmenu.cpp (working copy)
@@ -59,12 +59,12 @@
 #define NUM_REGIONS 7 // (6 + 1 for multiplayer equivalent of campaign room)
 typedef struct main_hall_defines {
  // bitmap and mask
- char bitmap[MAX_FILENAME_LEN];
- char mask[MAX_FILENAME_LEN];
+ SCP_string bitmap;
+ SCP_string mask;
 
  // music
- char music_name[MAX_FILENAME_LEN];
- char substitute_music_name[MAX_FILENAME_LEN];
+ SCP_string music_name;
+ SCP_string substitute_music_name;
 
  // intercom defines -------------------
 
@@ -87,7 +87,7 @@
  int num_misc_animations;
 
  // filenames of the misc animations
- char misc_anim_name[MAX_MISC_ANIMATIONS][MAX_FILENAME_LEN];
+ SCP_vector<SCP_string> misc_anim_name;
 
  // Time until we will next play a given misc animation, min delay, and max delay
  SCP_vector<SCP_vector<int> > misc_anim_delay;
@@ -110,39 +110,36 @@
  //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];
+ //frame number triggers for each of the misc anims
+ SCP_vector<SCP_vector<int> > misc_anim_special_trigger;
 
- // [N][0] == # of handles, [N][1-9] == sound handle num
- int misc_anim_sound_handles[MAX_MISC_ANIMATIONS][10];
+ //flags for each of the misc anim sounds
+ SCP_vector<SCP_vector<int> > misc_anim_sound_flag;
 
- // [N][0] == # of handles, [N][1-9] == sound "should" be playing
- int misc_anim_sound_flag[MAX_MISC_ANIMATIONS][10];
 
-
  // door animations --------------------
 
  // # of door animations
  int num_door_animations;
 
  // filenames of the door animations
- char door_anim_name[MAX_DOOR_ANIMATIONS][MAX_FILENAME_LEN];
+ SCP_vector<SCP_string> door_anim_name;
 
  // first pair : coords of where to play a given door anim
  // second pair : center of a given door anim in windowed mode
- int door_anim_coords[MAX_DOOR_ANIMATIONS][4];
+ SCP_vector<SCP_vector<int> > door_anim_coords;
 
  // sounds for each region (open/close)
  SCP_vector<SCP_vector<int> > door_sounds;
 
  // pan values for the door sounds
- float door_sound_pan[MAX_DOOR_ANIMATIONS];
+ SCP_vector<float> door_sound_pan;
 
 
  // region descriptions ----------------
 
  // text (tooltip) description
- char *region_descript[NUM_REGIONS];
+ SCP_vector<char*> region_descript;
 
  // y coord of where to draw tooltip text
  int region_yval;
@@ -503,7 +500,7 @@
  return;
  }
 
- int idx,s_idx;
+ int idx;
  char temp[100], whee[100];
 
  //reparse the table here if the relevant cmdline flag is set
@@ -529,19 +526,19 @@
  Main_hall = &Main_hall_defines[gr_screen.res][main_hall_num];
 
  // tooltip strings
- Main_hall->region_descript[0] = XSTR( "Exit FreeSpace 2", 353);
- Main_hall->region_descript[1] = XSTR( "Barracks - Manage your FreeSpace 2 pilots", 354);
- Main_hall->region_descript[2] = XSTR( "Ready room - Start or continue a campaign", 355);
- Main_hall->region_descript[3] = XSTR( "Tech room - View specifications of FreeSpace 2 ships and weaponry", 356);
- Main_hall->region_descript[4] = XSTR( "Options - Change your FreeSpace 2 options", 357);
- Main_hall->region_descript[5] = XSTR( "Campaign Room - View all available campaigns", 358);
- Main_hall->region_descript[6] = XSTR( "Multiplayer - Start or join a multiplayer game", 359);
+ Main_hall->region_descript.at(0) = XSTR( "Exit FreeSpace 2", 353);
+ Main_hall->region_descript.at(1) = XSTR( "Barracks - Manage your FreeSpace 2 pilots", 354);
+ Main_hall->region_descript.at(2) = XSTR( "Ready room - Start or continue a campaign", 355);
+ Main_hall->region_descript.at(3) = XSTR( "Tech room - View specifications of FreeSpace 2 ships and weaponry", 356);
+ Main_hall->region_descript.at(4) = XSTR( "Options - Change your FreeSpace 2 options", 357);
+ Main_hall->region_descript.at(5) = XSTR( "Campaign Room - View all available campaigns", 358);
+ Main_hall->region_descript.at(6) = XSTR( "Multiplayer - Start or join a multiplayer game", 359);
 
  // init tooltip shader // nearly black
  gr_create_shader(&Main_hall_tooltip_shader, 5, 5, 5, 168);
 
  // load the background bitmap
- Main_hall_bitmap = bm_load(Main_hall->bitmap);
+ Main_hall_bitmap = bm_load(const_cast<char*>(Main_hall->bitmap.c_str()));
  if (Main_hall_bitmap < 0) {
  nprintf(("General","WARNING! Couldn't load main hall background bitmap %s\n", Main_hall->bitmap));
  }
@@ -556,7 +553,7 @@
  Main_hall_mask_h = -1;
 
  // load the mask
- Main_hall_mask = bm_load(Main_hall->mask);
+ Main_hall_mask = bm_load(const_cast<char*>(Main_hall->mask.c_str()));
  if (Main_hall_mask < 0) {
  nprintf(("General","WARNING! Couldn't load main hall background mask %s\n", Main_hall->mask));
  if (gr_screen.res == 0) {
@@ -573,10 +570,10 @@
 
  // load up the misc animations, and nullify all the delay timestamps for the misc animations
  for (idx=0;idx<Main_hall->num_misc_animations;idx++) {
- generic_anim_init(&Main_hall_misc_anim[idx], Main_hall->misc_anim_name[idx]);
+ generic_anim_init(&Main_hall_misc_anim[idx], const_cast<char*>(Main_hall->misc_anim_name.at(idx).c_str()));
  Main_hall_misc_anim[idx].ani.bg_type = bg_type;
  if (generic_anim_stream(&Main_hall_misc_anim[idx]) == -1) {
- nprintf(("General","WARNING!, Could not load misc %s anim in main hall\n",Main_hall->misc_anim_name[idx]));
+ nprintf(("General","WARNING!, Could not load misc %s anim in main hall\n",Main_hall->misc_anim_name.at(idx)));
  } else {
  //start paused
  if (Main_hall->misc_anim_modes.at(idx) == MISC_ANIM_MODE_HOLD)
@@ -592,10 +589,10 @@
 
  // load up the door animations
  for (idx=0;idx<Main_hall->num_door_animations;idx++) {
- generic_anim_init(&Main_hall_door_anim[idx], Main_hall->door_anim_name[idx]);
+ generic_anim_init(&Main_hall_door_anim[idx], const_cast<char*>(Main_hall->door_anim_name.at(idx).c_str()));
  Main_hall_door_anim[idx].ani.bg_type = bg_type;
  if (generic_anim_stream(&Main_hall_door_anim[idx]) == -1) {
- nprintf(("General","WARNING!, Could not load door anim %s in main hall\n",Main_hall->door_anim_name[idx]));
+ nprintf(("General","WARNING!, Could not load door anim %s in main hall\n",Main_hall->door_anim_name.at(idx)));
  } else {
  Main_hall_door_anim[idx].direction = GENERIC_ANIM_DIRECTION_BACKWARDS | GENERIC_ANIM_DIRECTION_NOLOOP;
  }
@@ -639,14 +636,6 @@
  Main_hall_door_sound_handles[idx] = -1;
  }
 
- // zero out the misc anim sounds
- for (idx=0;idx<Main_hall->num_misc_animations;idx++) {
- for (s_idx = 1;s_idx < 10;s_idx++) {
- Main_hall->misc_anim_sound_handles[idx][s_idx] = -1;
- Main_hall->misc_anim_sound_flag[idx][s_idx] = 0;
- }
- }
-
  // skip the first frame
  Main_hall_frame_skip = 1;
 
@@ -1009,10 +998,9 @@
 
  // stop any playing misc animation sounds
  for (idx=0;idx<Main_hall->num_misc_animations;idx++) {
- for (s_idx=1;s_idx<10;s_idx++) {
- if (snd_is_playing(Main_hall->misc_anim_sound_handles[idx][s_idx])) {
- snd_stop(Main_hall->misc_anim_sound_handles[idx][s_idx]);
- Main_hall->misc_anim_sound_handles[idx][s_idx] = -1;
+ for (s_idx=0;s_idx<(int)Main_hall->misc_anim_special_sounds.at(idx).size();s_idx++) {
+ if (snd_is_playing(Main_hall->misc_anim_special_sounds.at(idx).at(s_idx))) {
+ snd_stop(Main_hall->misc_anim_special_sounds.at(idx).at(s_idx));
  }
  }
  }
@@ -1048,13 +1036,13 @@
  hall = &Main_hall_defines[gr_screen.res][main_hall_num];
 
  // Goober5000 - try substitute first
- index = event_music_get_spooled_music_index(hall->substitute_music_name);
+ index = event_music_get_spooled_music_index(const_cast<char*>(hall->substitute_music_name.c_str()));
  if ((index >= 0) && (Spooled_music[index].flags & SMF_VALID)) {
  return index;
  }
 
  // now try regular
- index = event_music_get_spooled_music_index(hall->music_name);
+ index = event_music_get_spooled_music_index(const_cast<char*>(hall->music_name.c_str()));
  if ((index >= 0) && (Spooled_music[index].flags & SMF_VALID)) {
  return index;
  }
@@ -1175,28 +1163,29 @@
  // kill the timestamp
  Main_hall->misc_anim_delay.at(idx).at(0) = -1;
 
+
  // reset the "should be playing" flags
- for (s_idx=1;s_idx<10;s_idx++) {
- Main_hall->misc_anim_sound_flag[idx][s_idx] = 0;
- }
+ for (s_idx=0;s_idx<(int)Main_hall->misc_anim_sound_flag.at(idx).size();s_idx++) {
+ Main_hall->misc_anim_sound_flag.at(idx).at(s_idx) = 0;
+ }
+
  }
  }
  // animation is not paused
  else {
- for (s_idx = 0; (unsigned)s_idx < Main_hall->misc_anim_special_sounds.at(idx).size(); s_idx++) {
+ for (s_idx = 0; s_idx < (int)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+1]) && !Main_hall->misc_anim_sound_flag[idx][s_idx+1]) {
- Main_hall->misc_anim_sound_flag[idx][s_idx+1] = 1;
+ if ((Main_hall_misc_anim[idx].current_frame >= Main_hall->misc_anim_special_trigger.at(idx).at(s_idx)) && !Main_hall->misc_anim_sound_flag.at(idx).at(s_idx)) {
+ Main_hall->misc_anim_sound_flag.at(idx).at(s_idx) = 1;
 
  // if the sound is already playing, then kill it. This is a pretty safe thing to do since we can assume that
  // by the time we get to this point again, the sound will have been long finished
- if (snd_is_playing(Main_hall->misc_anim_sound_handles[idx][s_idx+1])) {
- snd_stop(Main_hall->misc_anim_sound_handles[idx][s_idx+1]);
- Main_hall->misc_anim_sound_handles[idx][s_idx+1] = -1;
+ if (snd_is_playing(Main_hall->misc_anim_special_sounds.at(idx).at(s_idx))) {
+ snd_stop(Main_hall->misc_anim_special_sounds.at(idx).at(s_idx));
  }
 
  // play the sound
- Main_hall->misc_anim_sound_handles[idx][s_idx+1] = snd_play(&Snds_iface[Main_hall->misc_anim_special_sounds.at(idx).at(s_idx)],Main_hall->misc_anim_sound_pan.at(idx));
+ snd_play(&Snds_iface[Main_hall->misc_anim_special_sounds.at(idx).at(s_idx)],Main_hall->misc_anim_sound_pan.at(idx));
  break;
  }
  }
@@ -1214,8 +1203,8 @@
  //don't reset sound for MISC_ANIM_MODE_HOLD
  if (Main_hall->misc_anim_modes.at(idx) != MISC_ANIM_MODE_HOLD) {
  // reset the "should be playing" flags
- for (s_idx=1;s_idx<10;s_idx++) {
- Main_hall->misc_anim_sound_flag[idx][s_idx] = 0;
+ for (s_idx=0;s_idx<(int)Main_hall->misc_anim_sound_flag.at(idx).size();s_idx++) {
+ Main_hall->misc_anim_sound_flag.at(idx).at(s_idx) = 0;
  }
  }
  }
@@ -1240,7 +1229,7 @@
  if (Main_hall_door_anim[idx].num_frames > 0) {
  // first pair : coords of where to play a given door anim
  // second pair : center of a given door anim in windowed mode
- generic_anim_render(&Main_hall_door_anim[idx], frametime, Main_hall->door_anim_coords[idx][0], Main_hall->door_anim_coords[idx][1]);
+ generic_anim_render(&Main_hall_door_anim[idx], frametime, Main_hall->door_anim_coords.at(idx).at(0), Main_hall->door_anim_coords.at(idx).at(1));
  }
  }
 }
@@ -1309,7 +1298,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.at(region).at(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.at(region));
 
  //TODO: track current frame
  snd_set_pos(Main_hall_door_sound_handles[region], &Snds_iface[SND_MAIN_HALL_DOOR_CLOSE],
@@ -1338,7 +1327,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.at(region).at(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.at(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) ) {
@@ -1366,8 +1355,8 @@
  }
 
  // set the position of the mouse cursor and the newly clicked region
- int mx = Main_hall->door_anim_coords[new_region][2];
- int my = Main_hall->door_anim_coords[new_region][3];
+ int mx = Main_hall->door_anim_coords.at(new_region).at(2);
+ int my = Main_hall->door_anim_coords.at(new_region).at(3);
  gr_resize_screen_pos( &mx, &my );
  mouse_set_pos( mx, my );
 
@@ -1551,13 +1540,13 @@
  if (!help_overlay_active(Main_hall_overlay_id)) {
  int shader_y = (Main_hall->region_yval) - Main_hall_tooltip_padding[gr_screen.res]; // subtract more to pull higher
  // get the width of the string
- gr_get_string_size(&w, NULL, Main_hall->region_descript[text_index]);
+ gr_get_string_size(&w, NULL, Main_hall->region_descript.at(text_index));
 
  gr_set_shader(&Main_hall_tooltip_shader);
  gr_shade(0, shader_y, gr_screen.clip_width_unscaled, (gr_screen.clip_height_unscaled - shader_y));
 
  gr_set_color_fast(&Color_bright_white);
- gr_string((gr_screen.max_w_unscaled - w)/2, Main_hall->region_yval, Main_hall->region_descript[text_index]);
+ gr_string((gr_screen.max_w_unscaled - w)/2, Main_hall->region_yval, Main_hall->region_descript.at(text_index));
  }
 }
 
@@ -1607,12 +1596,8 @@
 //To be called after num_intercom_sounds has been parsed
 void intercom_sounds_init(main_hall_defines &m)
 {
- if (!m.intercom_delay.empty()) {
- //if one of these isn't empty, none of these should be empty
- Assert(!m.intercom_sounds.empty());
- Assert(!m.intercom_sound_pan.empty());
-
- //since we could be reparsing with a different number of intercom sounds, clear these and reinitialise
+ if (Cmdline_reparse_mainhall) {
+ //we could be reparsing with a different number of intercom sounds, so clear these and reinitialise
  m.intercom_delay.clear();
  m.intercom_sounds.clear();
  m.intercom_sound_pan.clear();
@@ -1636,28 +1621,32 @@
  }
 }
 
+//helper function for initialising misc anim vectors based on number of anims
+//to be called after num_misc_animations has been parsed
 void misc_anim_init(main_hall_defines &m)
 {
- if (!m.misc_anim_delay.empty()) {
- //if one of these isn't empty, none of these should be
- Assert(!m.misc_anim_paused.empty());
- Assert(!m.misc_anim_group.empty());
- Assert(!m.misc_anim_coords.empty());
- Assert(!m.misc_anim_modes.empty());
- Assert(!m.misc_anim_sound_pan.empty());
-
- //since we could be reparsing with a different number of misc anims, clear these and reinitialise
+ if (Cmdline_reparse_mainhall) {
+ //we could be reparsing with a different number of misc anims, so clear these and reinitialise
+ m.misc_anim_name.clear();
  m.misc_anim_delay.clear();
  m.misc_anim_paused.clear();
  m.misc_anim_group.clear();
  m.misc_anim_coords.clear();
  m.misc_anim_modes.clear();
  m.misc_anim_sound_pan.clear();
+ m.misc_anim_special_sounds.clear();
+ m.misc_anim_special_trigger.clear();
+ m.misc_anim_sound_flag.clear();
  }
 
  SCP_vector<int> temp;
+ SCP_string temp_string;
 
  for (int idx=0; idx<m.num_misc_animations; idx++) {
+
+ //misc_anim_name
+ m.misc_anim_name.push_back(temp_string);
+
  //misc_anim_delay
  m.misc_anim_delay.push_back(temp);
 
@@ -1682,15 +1671,71 @@
  m.misc_anim_modes.push_back(MISC_ANIM_MODE_LOOP);
 
  //misc_anim_sound_pan
- m.misc_anim_sound_pan.push_back(0);
+ m.misc_anim_sound_pan.push_back(0.0f);
+
+ //misc_anim_special_sounds
+ m.misc_anim_special_sounds.push_back(temp); //parse_sound_list deals with the rest of the initialisation for this one
+
+ //misc_anim_special_trigger
+ m.misc_anim_special_trigger.push_back(temp);
+
+ m.misc_anim_special_trigger.at(idx).push_back(0);
+
+ //misc_anim_sound_flag
+ m.misc_anim_sound_flag.push_back(temp);
  }
 }
 
+//helper function for initialising door anim vectors based on number of anims
+//to be called after num_door_animations has been parsed
+void door_anim_init(main_hall_defines &m)
+{
+ if (Cmdline_reparse_mainhall) {
+ //since we could be reparsing with a different number of door anims, clear these and reinitialise
+ m.door_anim_name.clear();
+ m.door_anim_coords.clear();
+ m.door_sounds.clear();
+ m.door_sound_pan.clear();
+ m.region_descript.clear();
+ }
+
+ SCP_vector<int> temp;
+ SCP_string temp_string;
+
+ for(int idx=0; idx<m.num_door_animations; idx++)
+ {
+ //door_anim_name
+ m.door_anim_name.push_back(temp_string);
+
+ //door_anim_coords
+ m.door_anim_coords.push_back(temp);
+
+ //we want two pairs of coordinates for each animation
+ m.door_anim_coords.at(idx).push_back(0);
+ m.door_anim_coords.at(idx).push_back(0);
+ m.door_anim_coords.at(idx).push_back(0);
+ m.door_anim_coords.at(idx).push_back(0);
+
+ //door_sounds
+ m.door_sounds.push_back(temp);
+
+ //door_sound_pan
+ m.door_sound_pan.push_back(0.0f);
+ }
+
+ //region_descript
+ for (int idx=0; idx<NUM_REGIONS; idx++) {
+ m.region_descript.push_back(NULL);
+ }
+
+}
+
 // read in main hall table
 void main_hall_read_table()
 {
  main_hall_defines *m, temp;
  int count, idx, s_idx, m_idx, rval;
+ char temp_string[MAX_FILENAME_LEN];
 
  if ((rval = setjmp(parse_abort)) != 0) {
  mprintf(("TABLES: Unable to parse '%s'!  Error code = %i.\n", "mainhall.tbl", rval));
@@ -1706,7 +1751,7 @@
  while (!optional_string("#end")) {
  // read in 2 resolutions
  for (m_idx=0; m_idx<GR_NUM_RESOLUTIONS; m_idx++) {
- // maybe use a temp main hall stuct
+ // maybe use a temp main hall struct
  if (count >= MAIN_HALLS_MAX) {
  m = &temp;
  Warning(LOCATION, "Number of main halls in mainhall.tbl has exceeded max of %d. All further main halls will be ignored.", MAIN_HALLS_MAX);
@@ -1721,17 +1766,21 @@
 
  // bitmap and mask
  required_string("+Bitmap:");
- stuff_string(m->bitmap, F_NAME, MAX_FILENAME_LEN);
+ stuff_string(temp_string, F_NAME, MAX_FILENAME_LEN);
+ m->bitmap = temp_string;
 
  required_string("+Mask:");
- stuff_string(m->mask, F_NAME, MAX_FILENAME_LEN);
+ stuff_string(temp_string, F_NAME, MAX_FILENAME_LEN);
+ m->mask = temp_string;
 
  required_string("+Music:");
- stuff_string(m->music_name, F_NAME, MAX_FILENAME_LEN);
+ stuff_string(temp_string, F_NAME, MAX_FILENAME_LEN);
+ m->music_name = temp_string;
 
  // Goober5000
  if (optional_string("+Substitute Music:")) {
- stuff_string(m->substitute_music_name, F_NAME, MAX_FILENAME_LEN);
+ stuff_string(temp_string, F_NAME, MAX_FILENAME_LEN);
+ m->substitute_music_name = temp_string;
  }
 
  // intercom sounds
@@ -1778,7 +1827,8 @@
  for (idx=0; idx<m->num_misc_animations; idx++) {
  // anim names
  required_string("+Misc anim:");
- stuff_string(m->misc_anim_name[idx], F_NAME, MAX_FILENAME_LEN);
+ stuff_string(temp_string, F_NAME, MAX_FILENAME_LEN);
+ m->misc_anim_name.at(idx) = (SCP_string)temp_string;
  }
 
  for (idx=0; idx<m->num_misc_animations; idx++) {
@@ -1819,30 +1869,38 @@
 
  for (idx=0; idx<m->num_misc_animations; idx++) {
  // 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);
  }
 
  for (idx=0; idx<m->num_misc_animations; idx++) {
  // anim sound triggers
  required_string("+Misc anim trigger:");
- stuff_int(&m->misc_anim_special_trigger[idx][0]);
- for (s_idx=0; s_idx<m->misc_anim_special_trigger[idx][0]; s_idx++) {
- stuff_int(&m->misc_anim_special_trigger[idx][s_idx + 1]);
+ int temp = 0;
+ stuff_int(&temp);
+ for (s_idx=0; s_idx<temp; s_idx++) {
+ m->misc_anim_special_trigger.at(idx).push_back(0);
+ stuff_int(&m->misc_anim_special_trigger.at(idx).at(s_idx));
  }
  }
 
  for (idx=0; idx<m->num_misc_animations; idx++) {
- // anim sound handles
- required_string("+Misc anim handles:");
- stuff_int(&m->misc_anim_sound_handles[idx][0]);
+ // anim sound handles - deprecated, but deal with it just in case
+ if(optional_string("+Misc anim handles:")) {
+ advance_to_eoln(NULL);
+ }
+
  }
 
  for (idx=0; idx<m->num_misc_animations; idx++) {
- // anim sound flags
- required_string("+Misc anim flags:");
- stuff_int(&m->misc_anim_sound_flag[idx][0]);
+ // anim sound flags - table flag deprecated, so ignore user input
+ if(optional_string("+Misc anim flags:")) {
+ advance_to_eoln(NULL);
+ }
+
+ //we need one flag for each sound
+ for(s_idx=0; s_idx<(int)m->misc_anim_special_sounds.at(idx).size(); s_idx++) {
+ m->misc_anim_sound_flag.at(idx).push_back(0);
+ }
  }
 
  // door animations
@@ -1853,25 +1911,27 @@
  m->num_door_animations = MAX_DOOR_ANIMATIONS;
  }
 
+ //initialise the door anim vectors
+ door_anim_init(*m);
+
  for (idx=0; idx<m->num_door_animations; idx++) {
  // door name
  required_string("+Door anim:");
- stuff_string(m->door_anim_name[idx], F_NAME, MAX_FILENAME_LEN);
+ stuff_string(temp_string, F_NAME, MAX_FILENAME_LEN);
+ m->door_anim_name.at(idx) = (SCP_string)temp_string;
  }
 
  for (idx=0; idx<m->num_door_animations; idx++) {
  // door coords
  required_string("+Door coords:");
- stuff_int(&m->door_anim_coords[idx][0]);
- stuff_int(&m->door_anim_coords[idx][1]);
- stuff_int(&m->door_anim_coords[idx][2]);
- stuff_int(&m->door_anim_coords[idx][3]);
+ stuff_int(&m->door_anim_coords.at(idx).at(0));
+ stuff_int(&m->door_anim_coords.at(idx).at(1));
+ stuff_int(&m->door_anim_coords.at(idx).at(2));
+ stuff_int(&m->door_anim_coords.at(idx).at(3));
  }
 
  for (idx=0; idx<m->num_door_animations; idx++) {
  // door open and close sounds
- 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));
  }
 
@@ -1884,9 +1944,6 @@
  // tooltip y location
  required_string("+Tooltip Y:");
  stuff_int(&m->region_yval);
- for (idx=0; idx<NUM_REGIONS; idx++) {
- m->region_descript[idx] = NULL;
- }
  }
 
  if (count < MAIN_HALLS_MAX) {
@@ -1906,11 +1963,11 @@
  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_1024][hall].door_anim_name[OPTIONS_REGION], "2_vhallheads");
+ Main_hall_defines[GR_1024][hall].door_anim_name.at(OPTIONS_REGION) = "2_vhallheads";
 
  // set the background
- strcpy_s(Main_hall_defines[GR_640][hall].bitmap, "vhallhead");
- strcpy_s(Main_hall_defines[GR_1024][hall].bitmap, "2_vhallhead");
+ Main_hall_defines[GR_640][hall].bitmap = "vhallhead";
+ Main_hall_defines[GR_1024][hall].bitmap = "2_vhallhead";
  }
 
  // free up memory from parsing the mainhall tbl
@@ -1926,7 +1983,7 @@
 int main_hall_is_vasudan()
 {
  // kind of a hack for now
- return (!stricmp(Main_hall->music_name, "Psampik") || !stricmp(Main_hall->music_name, "Psamtik"));
+ return (!stricmp(Main_hall->music_name.c_str(), "Psampik") || !stricmp(Main_hall->music_name.c_str(), "Psamtik"));
 }
 
 // silence sounds on mainhall if we hit a pause mode (ie. lost window focus, minimized, etc);

If a coder could take a look at these as soon as possible, that'd be great!
Title: Re: Modular Mainhall.tbl
Post by: Iss Mneur on December 28, 2011, 05:15:41 pm
Please setup/change bm_load to accept a const char* (or even a reference to a SCP_string) just so that the casting is done in one place not in every caller.

Code: [Select]
+ nprintf(("General","WARNING!, Could not load misc %s anim in main hall\n",Main_hall->misc_anim_name.at(idx)));
Assuming misc_anim_name is a SCP_string, you need to call c_str() because gcc will not like you otherwise (this is because var arg functions must only be passed PODs (http://en.wikipedia.org/wiki/Plain_old_data_structure). Visual studio is helpful and does it implicitly).


Please do the same for generic_anim_init() as bm_load().

You should Assert before every cast of a size_t to an int (MAX_INT).


Otherwise, looks good, I will test it later.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 28, 2011, 06:32:02 pm
You should Assert before every cast of a size_t to an int (MAX_INT).

To clarify, what exactly am I Asserting? That the size_t is less than MAX_INT?
Title: Re: Modular Mainhall.tbl
Post by: The E on December 28, 2011, 06:56:09 pm
Nope. You do an assertion on the variable you are going to cast to ensure it is >= 0, because size_t is an unsigned type.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 28, 2011, 07:12:37 pm
I don't understand. I'm casting from a size_t to an int, so won't the resulting int always be positive anyway since size_t is unsigned?
Title: Re: Modular Mainhall.tbl
Post by: Iss Mneur on December 28, 2011, 08:40:04 pm
I don't understand. I'm casting from a size_t to an int, so won't the resulting int always be positive anyway since size_t is unsigned?
Yes, The E misunderstood and got it backwards.

The Assertion should check that the result of the size() (which returns a size_t) call is less than MAX_INT becuase otherwise the int could overflow and be negative.  Admittedly having enough memory to cause this problem is rather rare, but it is hard to say when memory corruption could be caught by it.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 28, 2011, 09:15:57 pm
Ah, okay, that makes sense. Here's the patch with the issues addressed:

Code: [Select]
Index: code/bmpman/bmpman.cpp
===================================================================
--- code/bmpman/bmpman.cpp (revision 8114)
+++ code/bmpman/bmpman.cpp (working copy)
@@ -605,6 +605,14 @@
 }
 
 /**
+ * Same as bm_load above, just with an SCP_string
+ */
+int bm_load(SCP_string& filename)
+{
+ return bm_load(const_cast<char*> (filename.c_str()));
+}
+
+/**
  * Special load function. Basically allows you to load a bitmap which already exists (by filename).
  *
  * This is useful because in some cases we need to have a bitmap which is locked in screen format
Index: code/bmpman/bmpman.h
===================================================================
--- code/bmpman/bmpman.h (revision 8114)
+++ code/bmpman/bmpman.h (working copy)
@@ -89,6 +89,7 @@
 // the bitmap.   On success, it returns the bitmap
 // number.
 int bm_load(char * filename);
+int bm_load(SCP_string& filename);
 
 // special load function. basically allows you to load a bitmap which already exists (by filename).
 // this is useful because in some cases we need to have a bitmap which is locked in screen format
Index: code/graphics/generic.cpp
===================================================================
--- code/graphics/generic.cpp (revision 8114)
+++ code/graphics/generic.cpp (working copy)
@@ -100,6 +100,13 @@
  ga->width = 0;
  ga->bitmap_id = -1;
 }
+/**
+ * CommanderDJ - same as generic_anim_init, just with an SCP_string
+ */
+void generic_anim_init(generic_anim *ga, SCP_string& filename)
+{
+ generic_anim_init(ga, const_cast<char*> (filename.c_str()));
+}
 
 // Goober5000
 void generic_bitmap_init(generic_bitmap *gb, char *filename)
Index: code/graphics/generic.h
===================================================================
--- code/graphics/generic.h (revision 8114)
+++ code/graphics/generic.h (working copy)
@@ -54,6 +54,7 @@
 
 int generic_anim_init_and_stream(generic_anim *anim, char *anim_filename, ubyte bg_type, bool attempt_hi_res);
 void generic_anim_init(generic_anim *ga, char *filename = NULL);
+void generic_anim_init(generic_anim *ga, SCP_string& filename);
 void generic_bitmap_init(generic_bitmap *gb, char *filename = NULL);
 int generic_anim_load(generic_anim *ga);
 int generic_anim_stream(generic_anim *ga);
Index: code/menuui/mainhallmenu.cpp
===================================================================
--- code/menuui/mainhallmenu.cpp (revision 8114)
+++ code/menuui/mainhallmenu.cpp (working copy)
@@ -59,12 +59,12 @@
 #define NUM_REGIONS 7 // (6 + 1 for multiplayer equivalent of campaign room)
 typedef struct main_hall_defines {
  // bitmap and mask
- char bitmap[MAX_FILENAME_LEN];
- char mask[MAX_FILENAME_LEN];
+ SCP_string bitmap;
+ SCP_string mask;
 
  // music
- char music_name[MAX_FILENAME_LEN];
- char substitute_music_name[MAX_FILENAME_LEN];
+ SCP_string music_name;
+ SCP_string substitute_music_name;
 
  // intercom defines -------------------
 
@@ -87,7 +87,7 @@
  int num_misc_animations;
 
  // filenames of the misc animations
- char misc_anim_name[MAX_MISC_ANIMATIONS][MAX_FILENAME_LEN];
+ SCP_vector<SCP_string> misc_anim_name;
 
  // Time until we will next play a given misc animation, min delay, and max delay
  SCP_vector<SCP_vector<int> > misc_anim_delay;
@@ -110,39 +110,36 @@
  //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];
+ //frame number triggers for each of the misc anims
+ SCP_vector<SCP_vector<int> > misc_anim_special_trigger;
 
- // [N][0] == # of handles, [N][1-9] == sound handle num
- int misc_anim_sound_handles[MAX_MISC_ANIMATIONS][10];
+ //flags for each of the misc anim sounds
+ SCP_vector<SCP_vector<int> > misc_anim_sound_flag;
 
- // [N][0] == # of handles, [N][1-9] == sound "should" be playing
- int misc_anim_sound_flag[MAX_MISC_ANIMATIONS][10];
 
-
  // door animations --------------------
 
  // # of door animations
  int num_door_animations;
 
  // filenames of the door animations
- char door_anim_name[MAX_DOOR_ANIMATIONS][MAX_FILENAME_LEN];
+ SCP_vector<SCP_string> door_anim_name;
 
  // first pair : coords of where to play a given door anim
  // second pair : center of a given door anim in windowed mode
- int door_anim_coords[MAX_DOOR_ANIMATIONS][4];
+ SCP_vector<SCP_vector<int> > door_anim_coords;
 
  // sounds for each region (open/close)
  SCP_vector<SCP_vector<int> > door_sounds;
 
  // pan values for the door sounds
- float door_sound_pan[MAX_DOOR_ANIMATIONS];
+ SCP_vector<float> door_sound_pan;
 
 
  // region descriptions ----------------
 
  // text (tooltip) description
- char *region_descript[NUM_REGIONS];
+ SCP_vector<char*> region_descript;
 
  // y coord of where to draw tooltip text
  int region_yval;
@@ -503,7 +500,7 @@
  return;
  }
 
- int idx,s_idx;
+ int idx;
  char temp[100], whee[100];
 
  //reparse the table here if the relevant cmdline flag is set
@@ -529,13 +526,13 @@
  Main_hall = &Main_hall_defines[gr_screen.res][main_hall_num];
 
  // tooltip strings
- Main_hall->region_descript[0] = XSTR( "Exit FreeSpace 2", 353);
- Main_hall->region_descript[1] = XSTR( "Barracks - Manage your FreeSpace 2 pilots", 354);
- Main_hall->region_descript[2] = XSTR( "Ready room - Start or continue a campaign", 355);
- Main_hall->region_descript[3] = XSTR( "Tech room - View specifications of FreeSpace 2 ships and weaponry", 356);
- Main_hall->region_descript[4] = XSTR( "Options - Change your FreeSpace 2 options", 357);
- Main_hall->region_descript[5] = XSTR( "Campaign Room - View all available campaigns", 358);
- Main_hall->region_descript[6] = XSTR( "Multiplayer - Start or join a multiplayer game", 359);
+ Main_hall->region_descript.at(0) = XSTR( "Exit FreeSpace 2", 353);
+ Main_hall->region_descript.at(1) = XSTR( "Barracks - Manage your FreeSpace 2 pilots", 354);
+ Main_hall->region_descript.at(2) = XSTR( "Ready room - Start or continue a campaign", 355);
+ Main_hall->region_descript.at(3) = XSTR( "Tech room - View specifications of FreeSpace 2 ships and weaponry", 356);
+ Main_hall->region_descript.at(4) = XSTR( "Options - Change your FreeSpace 2 options", 357);
+ Main_hall->region_descript.at(5) = XSTR( "Campaign Room - View all available campaigns", 358);
+ Main_hall->region_descript.at(6) = XSTR( "Multiplayer - Start or join a multiplayer game", 359);
 
  // init tooltip shader // nearly black
  gr_create_shader(&Main_hall_tooltip_shader, 5, 5, 5, 168);
@@ -543,7 +540,7 @@
  // load the background bitmap
  Main_hall_bitmap = bm_load(Main_hall->bitmap);
  if (Main_hall_bitmap < 0) {
- nprintf(("General","WARNING! Couldn't load main hall background bitmap %s\n", Main_hall->bitmap));
+ nprintf(("General","WARNING! Couldn't load main hall background bitmap %s\n", Main_hall->bitmap.c_str()));
  }
  bg_type = bm_get_type(Main_hall_bitmap);
 
@@ -558,7 +555,7 @@
  // load the mask
  Main_hall_mask = bm_load(Main_hall->mask);
  if (Main_hall_mask < 0) {
- nprintf(("General","WARNING! Couldn't load main hall background mask %s\n", Main_hall->mask));
+ nprintf(("General","WARNING! Couldn't load main hall background mask %s\n", Main_hall->mask.c_str()));
  if (gr_screen.res == 0) {
  Error(LOCATION,"Could not load in main hall mask '%s'!\n\n(This error most likely means that you are missing required 640x480 interface art.)", Main_hall->mask);
  } else {
@@ -573,10 +570,10 @@
 
  // load up the misc animations, and nullify all the delay timestamps for the misc animations
  for (idx=0;idx<Main_hall->num_misc_animations;idx++) {
- generic_anim_init(&Main_hall_misc_anim[idx], Main_hall->misc_anim_name[idx]);
+ generic_anim_init(&Main_hall_misc_anim[idx], const_cast<char*>(Main_hall->misc_anim_name.at(idx).c_str()));
  Main_hall_misc_anim[idx].ani.bg_type = bg_type;
  if (generic_anim_stream(&Main_hall_misc_anim[idx]) == -1) {
- nprintf(("General","WARNING!, Could not load misc %s anim in main hall\n",Main_hall->misc_anim_name[idx]));
+ nprintf(("General","WARNING!, Could not load misc %s anim in main hall\n",Main_hall->misc_anim_name.at(idx).c_str()));
  } else {
  //start paused
  if (Main_hall->misc_anim_modes.at(idx) == MISC_ANIM_MODE_HOLD)
@@ -592,10 +589,10 @@
 
  // load up the door animations
  for (idx=0;idx<Main_hall->num_door_animations;idx++) {
- generic_anim_init(&Main_hall_door_anim[idx], Main_hall->door_anim_name[idx]);
+ generic_anim_init(&Main_hall_door_anim[idx], const_cast<char*>(Main_hall->door_anim_name.at(idx).c_str()));
  Main_hall_door_anim[idx].ani.bg_type = bg_type;
  if (generic_anim_stream(&Main_hall_door_anim[idx]) == -1) {
- nprintf(("General","WARNING!, Could not load door anim %s in main hall\n",Main_hall->door_anim_name[idx]));
+ nprintf(("General","WARNING!, Could not load door anim %s in main hall\n",Main_hall->door_anim_name.at(idx).c_str()));
  } else {
  Main_hall_door_anim[idx].direction = GENERIC_ANIM_DIRECTION_BACKWARDS | GENERIC_ANIM_DIRECTION_NOLOOP;
  }
@@ -639,14 +636,6 @@
  Main_hall_door_sound_handles[idx] = -1;
  }
 
- // zero out the misc anim sounds
- for (idx=0;idx<Main_hall->num_misc_animations;idx++) {
- for (s_idx = 1;s_idx < 10;s_idx++) {
- Main_hall->misc_anim_sound_handles[idx][s_idx] = -1;
- Main_hall->misc_anim_sound_flag[idx][s_idx] = 0;
- }
- }
-
  // skip the first frame
  Main_hall_frame_skip = 1;
 
@@ -1009,10 +998,13 @@
 
  // stop any playing misc animation sounds
  for (idx=0;idx<Main_hall->num_misc_animations;idx++) {
- for (s_idx=1;s_idx<10;s_idx++) {
- if (snd_is_playing(Main_hall->misc_anim_sound_handles[idx][s_idx])) {
- snd_stop(Main_hall->misc_anim_sound_handles[idx][s_idx]);
- Main_hall->misc_anim_sound_handles[idx][s_idx] = -1;
+
+ //if this goes wrong, the int cast could overflow
+ Assert(Main_hall->misc_anim_special_sounds.at(idx).size() < INT_MAX);
+
+ for (s_idx=0;s_idx<(int)Main_hall->misc_anim_special_sounds.at(idx).size();s_idx++) {
+ if (snd_is_playing(Main_hall->misc_anim_special_sounds.at(idx).at(s_idx))) {
+ snd_stop(Main_hall->misc_anim_special_sounds.at(idx).at(s_idx));
  }
  }
  }
@@ -1048,13 +1040,13 @@
  hall = &Main_hall_defines[gr_screen.res][main_hall_num];
 
  // Goober5000 - try substitute first
- index = event_music_get_spooled_music_index(hall->substitute_music_name);
+ index = event_music_get_spooled_music_index(const_cast<char*>(hall->substitute_music_name.c_str()));
  if ((index >= 0) && (Spooled_music[index].flags & SMF_VALID)) {
  return index;
  }
 
  // now try regular
- index = event_music_get_spooled_music_index(hall->music_name);
+ index = event_music_get_spooled_music_index(const_cast<char*>(hall->music_name.c_str()));
  if ((index >= 0) && (Spooled_music[index].flags & SMF_VALID)) {
  return index;
  }
@@ -1175,28 +1167,30 @@
  // kill the timestamp
  Main_hall->misc_anim_delay.at(idx).at(0) = -1;
 
+
  // reset the "should be playing" flags
- for (s_idx=1;s_idx<10;s_idx++) {
- Main_hall->misc_anim_sound_flag[idx][s_idx] = 0;
+ Assert(Main_hall->misc_anim_sound_flag.at(idx).size() < INT_MAX);
+ for (s_idx=0;s_idx<(int)Main_hall->misc_anim_sound_flag.at(idx).size();s_idx++) {
+ Main_hall->misc_anim_sound_flag.at(idx).at(s_idx) = 0;
  }
  }
  }
  // animation is not paused
  else {
- for (s_idx = 0; (unsigned)s_idx < Main_hall->misc_anim_special_sounds.at(idx).size(); s_idx++) {
+ Assert(Main_hall->misc_anim_special_sounds.at(idx).size() < INT_MAX);
+ for (s_idx = 0; s_idx < (int)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+1]) && !Main_hall->misc_anim_sound_flag[idx][s_idx+1]) {
- Main_hall->misc_anim_sound_flag[idx][s_idx+1] = 1;
+ if ((Main_hall_misc_anim[idx].current_frame >= Main_hall->misc_anim_special_trigger.at(idx).at(s_idx)) && !Main_hall->misc_anim_sound_flag.at(idx).at(s_idx)) {
+ Main_hall->misc_anim_sound_flag.at(idx).at(s_idx) = 1;
 
  // if the sound is already playing, then kill it. This is a pretty safe thing to do since we can assume that
  // by the time we get to this point again, the sound will have been long finished
- if (snd_is_playing(Main_hall->misc_anim_sound_handles[idx][s_idx+1])) {
- snd_stop(Main_hall->misc_anim_sound_handles[idx][s_idx+1]);
- Main_hall->misc_anim_sound_handles[idx][s_idx+1] = -1;
+ if (snd_is_playing(Main_hall->misc_anim_special_sounds.at(idx).at(s_idx))) {
+ snd_stop(Main_hall->misc_anim_special_sounds.at(idx).at(s_idx));
  }
 
  // play the sound
- Main_hall->misc_anim_sound_handles[idx][s_idx+1] = snd_play(&Snds_iface[Main_hall->misc_anim_special_sounds.at(idx).at(s_idx)],Main_hall->misc_anim_sound_pan.at(idx));
+ snd_play(&Snds_iface[Main_hall->misc_anim_special_sounds.at(idx).at(s_idx)],Main_hall->misc_anim_sound_pan.at(idx));
  break;
  }
  }
@@ -1214,8 +1208,9 @@
  //don't reset sound for MISC_ANIM_MODE_HOLD
  if (Main_hall->misc_anim_modes.at(idx) != MISC_ANIM_MODE_HOLD) {
  // reset the "should be playing" flags
- for (s_idx=1;s_idx<10;s_idx++) {
- Main_hall->misc_anim_sound_flag[idx][s_idx] = 0;
+ Assert(Main_hall->misc_anim_sound_flag.at(idx).size() < INT_MAX);
+ for (s_idx=0;s_idx<(int)Main_hall->misc_anim_sound_flag.at(idx).size();s_idx++) {
+ Main_hall->misc_anim_sound_flag.at(idx).at(s_idx) = 0;
  }
  }
  }
@@ -1240,7 +1235,7 @@
  if (Main_hall_door_anim[idx].num_frames > 0) {
  // first pair : coords of where to play a given door anim
  // second pair : center of a given door anim in windowed mode
- generic_anim_render(&Main_hall_door_anim[idx], frametime, Main_hall->door_anim_coords[idx][0], Main_hall->door_anim_coords[idx][1]);
+ generic_anim_render(&Main_hall_door_anim[idx], frametime, Main_hall->door_anim_coords.at(idx).at(0), Main_hall->door_anim_coords.at(idx).at(1));
  }
  }
 }
@@ -1309,7 +1304,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.at(region).at(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.at(region));
 
  //TODO: track current frame
  snd_set_pos(Main_hall_door_sound_handles[region], &Snds_iface[SND_MAIN_HALL_DOOR_CLOSE],
@@ -1338,7 +1333,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.at(region).at(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.at(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) ) {
@@ -1366,8 +1361,8 @@
  }
 
  // set the position of the mouse cursor and the newly clicked region
- int mx = Main_hall->door_anim_coords[new_region][2];
- int my = Main_hall->door_anim_coords[new_region][3];
+ int mx = Main_hall->door_anim_coords.at(new_region).at(2);
+ int my = Main_hall->door_anim_coords.at(new_region).at(3);
  gr_resize_screen_pos( &mx, &my );
  mouse_set_pos( mx, my );
 
@@ -1551,13 +1546,13 @@
  if (!help_overlay_active(Main_hall_overlay_id)) {
  int shader_y = (Main_hall->region_yval) - Main_hall_tooltip_padding[gr_screen.res]; // subtract more to pull higher
  // get the width of the string
- gr_get_string_size(&w, NULL, Main_hall->region_descript[text_index]);
+ gr_get_string_size(&w, NULL, Main_hall->region_descript.at(text_index));
 
  gr_set_shader(&Main_hall_tooltip_shader);
  gr_shade(0, shader_y, gr_screen.clip_width_unscaled, (gr_screen.clip_height_unscaled - shader_y));
 
  gr_set_color_fast(&Color_bright_white);
- gr_string((gr_screen.max_w_unscaled - w)/2, Main_hall->region_yval, Main_hall->region_descript[text_index]);
+ gr_string((gr_screen.max_w_unscaled - w)/2, Main_hall->region_yval, Main_hall->region_descript.at(text_index));
  }
 }
 
@@ -1607,12 +1602,8 @@
 //To be called after num_intercom_sounds has been parsed
 void intercom_sounds_init(main_hall_defines &m)
 {
- if (!m.intercom_delay.empty()) {
- //if one of these isn't empty, none of these should be empty
- Assert(!m.intercom_sounds.empty());
- Assert(!m.intercom_sound_pan.empty());
-
- //since we could be reparsing with a different number of intercom sounds, clear these and reinitialise
+ if (Cmdline_reparse_mainhall) {
+ //we could be reparsing with a different number of intercom sounds, so clear these and reinitialise
  m.intercom_delay.clear();
  m.intercom_sounds.clear();
  m.intercom_sound_pan.clear();
@@ -1636,28 +1627,32 @@
  }
 }
 
+//helper function for initialising misc anim vectors based on number of anims
+//to be called after num_misc_animations has been parsed
 void misc_anim_init(main_hall_defines &m)
 {
- if (!m.misc_anim_delay.empty()) {
- //if one of these isn't empty, none of these should be
- Assert(!m.misc_anim_paused.empty());
- Assert(!m.misc_anim_group.empty());
- Assert(!m.misc_anim_coords.empty());
- Assert(!m.misc_anim_modes.empty());
- Assert(!m.misc_anim_sound_pan.empty());
-
- //since we could be reparsing with a different number of misc anims, clear these and reinitialise
+ if (Cmdline_reparse_mainhall) {
+ //we could be reparsing with a different number of misc anims, so clear these and reinitialise
+ m.misc_anim_name.clear();
  m.misc_anim_delay.clear();
  m.misc_anim_paused.clear();
  m.misc_anim_group.clear();
  m.misc_anim_coords.clear();
  m.misc_anim_modes.clear();
  m.misc_anim_sound_pan.clear();
+ m.misc_anim_special_sounds.clear();
+ m.misc_anim_special_trigger.clear();
+ m.misc_anim_sound_flag.clear();
  }
 
  SCP_vector<int> temp;
+ SCP_string temp_string;
 
  for (int idx=0; idx<m.num_misc_animations; idx++) {
+
+ //misc_anim_name
+ m.misc_anim_name.push_back(temp_string);
+
  //misc_anim_delay
  m.misc_anim_delay.push_back(temp);
 
@@ -1682,15 +1677,71 @@
  m.misc_anim_modes.push_back(MISC_ANIM_MODE_LOOP);
 
  //misc_anim_sound_pan
- m.misc_anim_sound_pan.push_back(0);
+ m.misc_anim_sound_pan.push_back(0.0f);
+
+ //misc_anim_special_sounds
+ m.misc_anim_special_sounds.push_back(temp); //parse_sound_list deals with the rest of the initialisation for this one
+
+ //misc_anim_special_trigger
+ m.misc_anim_special_trigger.push_back(temp);
+
+ m.misc_anim_special_trigger.at(idx).push_back(0);
+
+ //misc_anim_sound_flag
+ m.misc_anim_sound_flag.push_back(temp);
  }
 }
 
+//helper function for initialising door anim vectors based on number of anims
+//to be called after num_door_animations has been parsed
+void door_anim_init(main_hall_defines &m)
+{
+ if (Cmdline_reparse_mainhall) {
+ //since we could be reparsing with a different number of door anims, clear these and reinitialise
+ m.door_anim_name.clear();
+ m.door_anim_coords.clear();
+ m.door_sounds.clear();
+ m.door_sound_pan.clear();
+ m.region_descript.clear();
+ }
+
+ SCP_vector<int> temp;
+ SCP_string temp_string;
+
+ for(int idx=0; idx<m.num_door_animations; idx++)
+ {
+ //door_anim_name
+ m.door_anim_name.push_back(temp_string);
+
+ //door_anim_coords
+ m.door_anim_coords.push_back(temp);
+
+ //we want two pairs of coordinates for each animation
+ m.door_anim_coords.at(idx).push_back(0);
+ m.door_anim_coords.at(idx).push_back(0);
+ m.door_anim_coords.at(idx).push_back(0);
+ m.door_anim_coords.at(idx).push_back(0);
+
+ //door_sounds
+ m.door_sounds.push_back(temp);
+
+ //door_sound_pan
+ m.door_sound_pan.push_back(0.0f);
+ }
+
+ //region_descript
+ for (int idx=0; idx<NUM_REGIONS; idx++) {
+ m.region_descript.push_back(NULL);
+ }
+
+}
+
 // read in main hall table
 void main_hall_read_table()
 {
  main_hall_defines *m, temp;
  int count, idx, s_idx, m_idx, rval;
+ char temp_string[MAX_FILENAME_LEN];
 
  if ((rval = setjmp(parse_abort)) != 0) {
  mprintf(("TABLES: Unable to parse '%s'!  Error code = %i.\n", "mainhall.tbl", rval));
@@ -1706,7 +1757,7 @@
  while (!optional_string("#end")) {
  // read in 2 resolutions
  for (m_idx=0; m_idx<GR_NUM_RESOLUTIONS; m_idx++) {
- // maybe use a temp main hall stuct
+ // maybe use a temp main hall struct
  if (count >= MAIN_HALLS_MAX) {
  m = &temp;
  Warning(LOCATION, "Number of main halls in mainhall.tbl has exceeded max of %d. All further main halls will be ignored.", MAIN_HALLS_MAX);
@@ -1721,17 +1772,21 @@
 
  // bitmap and mask
  required_string("+Bitmap:");
- stuff_string(m->bitmap, F_NAME, MAX_FILENAME_LEN);
+ stuff_string(temp_string, F_NAME, MAX_FILENAME_LEN);
+ m->bitmap = temp_string;
 
  required_string("+Mask:");
- stuff_string(m->mask, F_NAME, MAX_FILENAME_LEN);
+ stuff_string(temp_string, F_NAME, MAX_FILENAME_LEN);
+ m->mask = temp_string;
 
  required_string("+Music:");
- stuff_string(m->music_name, F_NAME, MAX_FILENAME_LEN);
+ stuff_string(temp_string, F_NAME, MAX_FILENAME_LEN);
+ m->music_name = temp_string;
 
  // Goober5000
  if (optional_string("+Substitute Music:")) {
- stuff_string(m->substitute_music_name, F_NAME, MAX_FILENAME_LEN);
+ stuff_string(temp_string, F_NAME, MAX_FILENAME_LEN);
+ m->substitute_music_name = temp_string;
  }
 
  // intercom sounds
@@ -1778,7 +1833,8 @@
  for (idx=0; idx<m->num_misc_animations; idx++) {
  // anim names
  required_string("+Misc anim:");
- stuff_string(m->misc_anim_name[idx], F_NAME, MAX_FILENAME_LEN);
+ stuff_string(temp_string, F_NAME, MAX_FILENAME_LEN);
+ m->misc_anim_name.at(idx) = (SCP_string)temp_string;
  }
 
  for (idx=0; idx<m->num_misc_animations; idx++) {
@@ -1819,30 +1875,39 @@
 
  for (idx=0; idx<m->num_misc_animations; idx++) {
  // 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);
  }
 
  for (idx=0; idx<m->num_misc_animations; idx++) {
  // anim sound triggers
  required_string("+Misc anim trigger:");
- stuff_int(&m->misc_anim_special_trigger[idx][0]);
- for (s_idx=0; s_idx<m->misc_anim_special_trigger[idx][0]; s_idx++) {
- stuff_int(&m->misc_anim_special_trigger[idx][s_idx + 1]);
+ int temp = 0;
+ stuff_int(&temp);
+ for (s_idx=0; s_idx<temp; s_idx++) {
+ m->misc_anim_special_trigger.at(idx).push_back(0);
+ stuff_int(&m->misc_anim_special_trigger.at(idx).at(s_idx));
  }
  }
 
  for (idx=0; idx<m->num_misc_animations; idx++) {
- // anim sound handles
- required_string("+Misc anim handles:");
- stuff_int(&m->misc_anim_sound_handles[idx][0]);
+ // anim sound handles - deprecated, but deal with it just in case
+ if(optional_string("+Misc anim handles:")) {
+ advance_to_eoln(NULL);
+ }
+
  }
 
  for (idx=0; idx<m->num_misc_animations; idx++) {
- // anim sound flags
- required_string("+Misc anim flags:");
- stuff_int(&m->misc_anim_sound_flag[idx][0]);
+ // anim sound flags - table flag deprecated, so ignore user input
+ if(optional_string("+Misc anim flags:")) {
+ advance_to_eoln(NULL);
+ }
+
+ //we need one flag for each sound
+ Assert(m->misc_anim_special_sounds.at(idx).size() < INT_MAX);
+ for(s_idx=0; s_idx<(int)m->misc_anim_special_sounds.at(idx).size(); s_idx++) {
+ m->misc_anim_sound_flag.at(idx).push_back(0);
+ }
  }
 
  // door animations
@@ -1853,25 +1918,27 @@
  m->num_door_animations = MAX_DOOR_ANIMATIONS;
  }
 
+ //initialise the door anim vectors
+ door_anim_init(*m);
+
  for (idx=0; idx<m->num_door_animations; idx++) {
  // door name
  required_string("+Door anim:");
- stuff_string(m->door_anim_name[idx], F_NAME, MAX_FILENAME_LEN);
+ stuff_string(temp_string, F_NAME, MAX_FILENAME_LEN);
+ m->door_anim_name.at(idx) = (SCP_string)temp_string;
  }
 
  for (idx=0; idx<m->num_door_animations; idx++) {
  // door coords
  required_string("+Door coords:");
- stuff_int(&m->door_anim_coords[idx][0]);
- stuff_int(&m->door_anim_coords[idx][1]);
- stuff_int(&m->door_anim_coords[idx][2]);
- stuff_int(&m->door_anim_coords[idx][3]);
+ stuff_int(&m->door_anim_coords.at(idx).at(0));
+ stuff_int(&m->door_anim_coords.at(idx).at(1));
+ stuff_int(&m->door_anim_coords.at(idx).at(2));
+ stuff_int(&m->door_anim_coords.at(idx).at(3));
  }
 
  for (idx=0; idx<m->num_door_animations; idx++) {
  // door open and close sounds
- 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));
  }
 
@@ -1884,9 +1951,6 @@
  // tooltip y location
  required_string("+Tooltip Y:");
  stuff_int(&m->region_yval);
- for (idx=0; idx<NUM_REGIONS; idx++) {
- m->region_descript[idx] = NULL;
- }
  }
 
  if (count < MAIN_HALLS_MAX) {
@@ -1906,11 +1970,11 @@
  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_1024][hall].door_anim_name[OPTIONS_REGION], "2_vhallheads");
+ Main_hall_defines[GR_1024][hall].door_anim_name.at(OPTIONS_REGION) = "2_vhallheads";
 
  // set the background
- strcpy_s(Main_hall_defines[GR_640][hall].bitmap, "vhallhead");
- strcpy_s(Main_hall_defines[GR_1024][hall].bitmap, "2_vhallhead");
+ Main_hall_defines[GR_640][hall].bitmap = "vhallhead";
+ Main_hall_defines[GR_1024][hall].bitmap = "2_vhallhead";
  }
 
  // free up memory from parsing the mainhall tbl
@@ -1926,7 +1990,7 @@
 int main_hall_is_vasudan()
 {
  // kind of a hack for now
- return (!stricmp(Main_hall->music_name, "Psampik") || !stricmp(Main_hall->music_name, "Psamtik"));
+ return (!stricmp(Main_hall->music_name.c_str(), "Psampik") || !stricmp(Main_hall->music_name.c_str(), "Psamtik"));
 }
 
 // silence sounds on mainhall if we hit a pause mode (ie. lost window focus, minimized, etc);

As usual, apply straight to an updated trunk. I'm getting no issues, but of course someone else will need to verify that. :D
Title: Re: Modular Mainhall.tbl
Post by: Iss Mneur on December 29, 2011, 12:25:44 am
committed as 8117.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on December 30, 2011, 12:48:19 am
Hello everyone!
A slightly rushed update as I need to leave for work in about two minutes, but here's a patch which makes the maximum number of intercom sounds, misc anims and door anims dynamic (ie removes the limit completely).

This will need a fair bit of testing; I haven't tested it as thoroughly as I would like, but will do so over the next day or two. One thing in particular: this patch affects how misc anim groups are handled a bit by the code, so these must be tested to make sure they function as they did previously. Retail mainhalls don't use misc anim groups, which means that you'll need to test this particular aspect of the patch by downloading MjnMixael's Beta Mainhall Pack available here (http://www.hard-light.net/forums/index.php?topic=71036.msg1558485#msg1558485). The rest can be tested with just the retail mainhalls. Testers: please pay attention to detail and make sure things function as they should (sounds are playing, animations are playing, sounds and anims are in sync etc). Try coming back to the mainhall after a mission or a trip to the tech room, and please test both with and without the -reparse_mainhall flag. As usual, anything wonky can be reported here.

Code: [Select]
Index: code/menuui/mainhallmenu.cpp
===================================================================
--- code/menuui/mainhallmenu.cpp (revision 8127)
+++ code/menuui/mainhallmenu.cpp (working copy)
@@ -48,9 +48,9 @@
 // ----------------------------------------------------------------------------
 // MAIN HALL DATA DEFINES
 //
-#define MAX_RANDOM_INTERCOM_SOUNDS 10
-#define MAX_MISC_ANIMATIONS 32
-#define MAX_DOOR_ANIMATIONS 10
+//#define MAX_RANDOM_INTERCOM_SOUNDS 10 CommanderDJ - these are dynamic now
+//#define MAX_MISC_ANIMATIONS 32
+//#define MAX_DOOR_ANIMATIONS 10
 
 #define MISC_ANIM_MODE_LOOP 0 // loop the animation
 #define MISC_ANIM_MODE_HOLD 1 // play to the end and hold the animation
@@ -221,7 +221,7 @@
 //
 
 // the misc animations themselves
-generic_anim Main_hall_misc_anim[MAX_MISC_ANIMATIONS];
+SCP_vector<generic_anim> Main_hall_misc_anim;
 
 // handle starting, stopping and randomizing misc animations
 void main_hall_handle_misc_anims();
@@ -240,12 +240,8 @@
 #define DOOR_TEXT_Y 450
 
 // the door animations themselves
-//anim *Main_hall_door_anim[MAX_DOOR_ANIMATIONS];
-generic_anim Main_hall_door_anim[MAX_DOOR_ANIMATIONS];
+SCP_vector<generic_anim> Main_hall_door_anim;
 
-// the instance of a given door animation
-//anim_instance *Main_hall_door_anim_instance[MAX_DOOR_ANIMATIONS];
-
 // render all playing door animations
 void main_hall_render_door_anims(float frametime);
 
@@ -303,9 +299,7 @@
 #define ALLENDER_REGION 4
 
 // handles to the sound instances of the doors opening/closing
-int Main_hall_door_sound_handles[MAX_DOOR_ANIMATIONS] = {
- -1,-1,-1,-1,-1,-1
-};
+SCP_vector<int> Main_hall_door_sound_handles;
 
 // sound handle for looping ambient sound
 int Main_hall_ambient_loop = -1;
@@ -567,17 +561,22 @@
  Main_hall_mask_data = (ubyte*)Main_hall_mask_bitmap->data;
  bm_get_info(Main_hall_mask, &Main_hall_mask_w, &Main_hall_mask_h);
  }
+
+ //In case we're re-entering the mainhall
+ Main_hall_misc_anim.clear();
 
  // load up the misc animations, and nullify all the delay timestamps for the misc animations
  for (idx=0;idx<Main_hall->num_misc_animations;idx++) {
- generic_anim_init(&Main_hall_misc_anim[idx], Main_hall->misc_anim_name.at(idx).c_str());
- Main_hall_misc_anim[idx].ani.bg_type = bg_type;
- if (generic_anim_stream(&Main_hall_misc_anim[idx]) == -1) {
+ generic_anim temp;
+ generic_anim_init(&temp, Main_hall->misc_anim_name.at(idx).c_str());
+ Main_hall_misc_anim.push_back(temp);
+ Main_hall_misc_anim.at(idx).ani.bg_type = bg_type;
+ if (generic_anim_stream(&Main_hall_misc_anim.at(idx)) == -1) {
  nprintf(("General","WARNING!, Could not load misc %s anim in main hall\n",Main_hall->misc_anim_name.at(idx).c_str()));
  } else {
  //start paused
  if (Main_hall->misc_anim_modes.at(idx) == MISC_ANIM_MODE_HOLD)
- Main_hall_misc_anim[idx].direction |= GENERIC_ANIM_DIRECTION_NOLOOP;
+ Main_hall_misc_anim.at(idx).direction |= GENERIC_ANIM_DIRECTION_NOLOOP;
  }
 
  // null out the delay timestamps
@@ -587,14 +586,19 @@
  Main_hall->misc_anim_paused.at(idx) = true;
  }
 
+ //In case we're re-entering the mainhall
+ Main_hall_door_anim.clear();
+
  // load up the door animations
  for (idx=0;idx<Main_hall->num_door_animations;idx++) {
- generic_anim_init(&Main_hall_door_anim[idx], Main_hall->door_anim_name.at(idx));
- Main_hall_door_anim[idx].ani.bg_type = bg_type;
- if (generic_anim_stream(&Main_hall_door_anim[idx]) == -1) {
+ generic_anim temp;
+ generic_anim_init(&temp, Main_hall->door_anim_name.at(idx));
+ Main_hall_door_anim.push_back(temp);
+ Main_hall_door_anim.at(idx).ani.bg_type = bg_type;
+ if (generic_anim_stream(&Main_hall_door_anim.at(idx)) == -1) {
  nprintf(("General","WARNING!, Could not load door anim %s in main hall\n",Main_hall->door_anim_name.at(idx).c_str()));
  } else {
- Main_hall_door_anim[idx].direction = GENERIC_ANIM_DIRECTION_BACKWARDS | GENERIC_ANIM_DIRECTION_NOLOOP;
+ Main_hall_door_anim.at(idx).direction = GENERIC_ANIM_DIRECTION_BACKWARDS | GENERIC_ANIM_DIRECTION_NOLOOP;
  }
  }
 
@@ -631,9 +635,10 @@
 
  strcpy_s(Main_hall_campaign_cheat, "");
 
- // zero out the door sounds
+ // initialise door sound handles
+ Main_hall_door_sound_handles.clear();
  for (idx=0;idx<Main_hall->num_door_animations;idx++) {
- Main_hall_door_sound_handles[idx] = -1;
+ Main_hall_door_sound_handles.push_back(-1);
  }
 
  // skip the first frame
@@ -976,23 +981,23 @@
 
  // free up any (possibly) playing misc animation handles
  for (idx=0;idx<Main_hall->num_misc_animations;idx++) {
- if (Main_hall_misc_anim[idx].num_frames > 0) {
- generic_anim_unload(&Main_hall_misc_anim[idx]);
+ if (Main_hall_misc_anim.at(idx).num_frames > 0) {
+ generic_anim_unload(&Main_hall_misc_anim.at(idx));
  }
  }
 
  // free up any (possibly) playing door animation handles
  for (idx=0;idx<Main_hall->num_door_animations;idx++) {
- if (Main_hall_door_anim[idx].num_frames > 0) {
- generic_anim_unload(&Main_hall_door_anim[idx]);
+ if (Main_hall_door_anim.at(idx).num_frames > 0) {
+ generic_anim_unload(&Main_hall_door_anim.at(idx));
  }
  }
 
  // stop any playing door sounds
  for (idx=0;idx<Main_hall->num_door_animations-2;idx++) { // don't cut off the glow sounds (requested by Dan)
- if ( (Main_hall_door_sound_handles[idx] != -1) && snd_is_playing(Main_hall_door_sound_handles[idx]) ) {
- snd_stop(Main_hall_door_sound_handles[idx]);
- Main_hall_door_sound_handles[idx] = -1;
+ if ( (Main_hall_door_sound_handles.at(idx) != -1) && snd_is_playing(Main_hall_door_sound_handles.at(idx)) ) {
+ snd_stop(Main_hall_door_sound_handles.at(idx));
+ Main_hall_door_sound_handles.at(idx) = -1;
  }
  }
 
@@ -1105,14 +1110,16 @@
 // render all playing misc animations
 void main_hall_render_misc_anims(float frametime)
 {
- int idx, s_idx;
- Assert(MAX_MISC_ANIMATIONS <= 32); // otherwise the bitfield trick won't work and we'll need to use an array of booleans
- int jdx, group_anims_weve_checked = 0;
+ int idx, s_idx, jdx;
+ SCP_vector<bool> group_anims_weve_checked;
 
  // render all misc animations
  for (idx = 0; idx < Main_hall->num_misc_animations; idx++) {
+ //give it a spot in the vector
+ group_anims_weve_checked.push_back(false);
+
  // render it
- if (Main_hall_misc_anim[idx].num_frames > 0) {
+ if (Main_hall_misc_anim.at(idx).num_frames > 0) {
  // animation is paused
  if (Main_hall->misc_anim_paused.at(idx)) {
  // if the timestamp is -1, then regenerate it
@@ -1121,19 +1128,25 @@
 
  // if this is part of a group, we should do additional checking
  if (Main_hall->misc_anim_group.at(idx) >= 0) {
+
  // make sure we haven't already checked it
- if (!(group_anims_weve_checked & (1<<idx))) {
- int group_indexes[MAX_MISC_ANIMATIONS];
- int num_group_indexes = 0;
+ if (group_anims_weve_checked.at(idx) == false) {
+ SCP_vector<int> group_indexes; //stores indexes of which anims are part of a group
  bool all_neg1 = true;
 
  // okay... now we need to make sure all anims in this group are paused and -1
  for (jdx = 0; jdx < Main_hall->num_misc_animations; jdx++) {
  if (Main_hall->misc_anim_group.at(jdx) == Main_hall->misc_anim_group.at(idx)) {
- group_anims_weve_checked |= (1<<jdx);
- group_indexes[num_group_indexes] = jdx;
- num_group_indexes++;
+ Assert(group_anims_weve_checked.size() < INT_MAX);
+ if((int)group_anims_weve_checked.size() <= jdx) {
+ group_anims_weve_checked.push_back(true);
+ }
+ else {
+ group_anims_weve_checked.at(jdx) = true;
+ }
 
+ group_indexes.push_back(jdx);
+
  if (!Main_hall->misc_anim_paused.at(jdx) || Main_hall->misc_anim_delay.at(jdx).at(0) != -1) {
  all_neg1 = false;
  }
@@ -1142,7 +1155,8 @@
 
  // if the entire group is paused and off, pick a random one to regenerate
  if (all_neg1) {
- regen_idx = group_indexes[rand() % num_group_indexes];
+ Assert(group_indexes.size() < INT_MAX);
+ regen_idx = group_indexes[rand() % (int)group_indexes.size()];
  }
  }
  }
@@ -1161,8 +1175,8 @@
  // if the timestamp is not -1 and has popped, play the anim and make the timestamp -1
  } else if (timestamp_elapsed(Main_hall->misc_anim_delay.at(idx).at(0))) {
  Main_hall->misc_anim_paused.at(idx) = false;
- Main_hall_misc_anim[idx].current_frame = 0;
- Main_hall_misc_anim[idx].anim_time = 0.0;
+ Main_hall_misc_anim.at(idx).current_frame = 0;
+ Main_hall_misc_anim.at(idx).anim_time = 0.0;
 
  // kill the timestamp
  Main_hall->misc_anim_delay.at(idx).at(0) = -1;
@@ -1180,7 +1194,7 @@
  Assert(Main_hall->misc_anim_special_sounds.at(idx).size() < INT_MAX);
  for (s_idx = 0; s_idx < (int)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.at(idx).at(s_idx)) && !Main_hall->misc_anim_sound_flag.at(idx).at(s_idx)) {
+ if ((Main_hall_misc_anim.at(idx).current_frame >= Main_hall->misc_anim_special_trigger.at(idx).at(s_idx)) && !Main_hall->misc_anim_sound_flag.at(idx).at(s_idx)) {
  Main_hall->misc_anim_sound_flag.at(idx).at(s_idx) = 1;
 
  // if the sound is already playing, then kill it. This is a pretty safe thing to do since we can assume that
@@ -1196,7 +1210,7 @@
  }
 
  // animation has reached the last frame
- if (Main_hall_misc_anim[idx].current_frame == Main_hall_misc_anim[idx].num_frames - 1) {
+ if (Main_hall_misc_anim.at(idx).current_frame == Main_hall_misc_anim.at(idx).num_frames - 1) {
  Main_hall->misc_anim_delay.at(idx).at(0) = -1;
 
  //this helps the above code reset the timers
@@ -1219,7 +1233,7 @@
  if (Main_hall_frame_skip || Main_hall_paused) {
  frametime = 0;
  }
- generic_anim_render(&Main_hall_misc_anim[idx], frametime, Main_hall->misc_anim_coords.at(idx).at(0), Main_hall->misc_anim_coords.at(idx).at(1));
+ generic_anim_render(&Main_hall_misc_anim.at(idx), frametime, Main_hall->misc_anim_coords.at(idx).at(0), Main_hall->misc_anim_coords.at(idx).at(1));
  }
  }
  }
@@ -1231,11 +1245,12 @@
  int idx;
 
  // render all door animations
- for (idx=0;idx<MAX_DOOR_ANIMATIONS;idx++) {
- if (Main_hall_door_anim[idx].num_frames > 0) {
+ Assert(Main_hall_door_anim.size() < INT_MAX);
+ for (idx=0;idx<(int)Main_hall_door_anim.size();idx++) {
+ if (Main_hall_door_anim.at(idx).num_frames > 0) {
  // first pair : coords of where to play a given door anim
  // second pair : center of a given door anim in windowed mode
- generic_anim_render(&Main_hall_door_anim[idx], frametime, Main_hall->door_anim_coords.at(idx).at(0), Main_hall->door_anim_coords.at(idx).at(1));
+ generic_anim_render(&Main_hall_door_anim.at(idx), frametime, Main_hall->door_anim_coords.at(idx).at(0), Main_hall->door_anim_coords.at(idx).at(1));
  }
  }
 }
@@ -1296,21 +1311,21 @@
  }
 
  //run backwards and stop at the first frame
- Main_hall_door_anim[region].direction = GENERIC_ANIM_DIRECTION_BACKWARDS | GENERIC_ANIM_DIRECTION_NOLOOP;
+ Main_hall_door_anim.at(region).direction = GENERIC_ANIM_DIRECTION_BACKWARDS | GENERIC_ANIM_DIRECTION_NOLOOP;
 
  // check for door sounds, ignoring the OPTIONS_REGION (which isn't a door)
- if ((Main_hall_door_anim[region].num_frames > 0)) {
+ if ((Main_hall_door_anim.at(region).num_frames > 0)) {
  // don't stop the toaster oven or microwave regions from playing all the way through
- if (Main_hall_door_sound_handles[region] != -1) {
- snd_stop(Main_hall_door_sound_handles[region]);
+ if (Main_hall_door_sound_handles.at(region) != -1) {
+ snd_stop(Main_hall_door_sound_handles.at(region));
  }
- Main_hall_door_sound_handles[region] = snd_play(&Snds_iface[Main_hall->door_sounds.at(region).at(1)], Main_hall->door_sound_pan.at(region));
+ Main_hall_door_sound_handles.at(region) = snd_play(&Snds_iface[Main_hall->door_sounds.at(region).at(1)], Main_hall->door_sound_pan.at(region));
 
  //TODO: track current frame
- snd_set_pos(Main_hall_door_sound_handles[region], &Snds_iface[SND_MAIN_HALL_DOOR_CLOSE],
- (float)((Main_hall_door_anim[region].keyframe) ? Main_hall_door_anim[region].keyframe :
- Main_hall_door_anim[region].num_frames - Main_hall_door_anim[region].current_frame) /
- (float)Main_hall_door_anim[region].num_frames, 1);
+ snd_set_pos(Main_hall_door_sound_handles.at(region), &Snds_iface[SND_MAIN_HALL_DOOR_CLOSE],
+ (float)((Main_hall_door_anim.at(region).keyframe) ? Main_hall_door_anim.at(region).keyframe :
+ Main_hall_door_anim.at(region).num_frames - Main_hall_door_anim.at(region).current_frame) /
+ (float)Main_hall_door_anim.at(region).num_frames, 1);
  }
 }
 
@@ -1322,23 +1337,23 @@
  }
 
  //run forwards
- Main_hall_door_anim[region].direction = GENERIC_ANIM_DIRECTION_FORWARDS;
+ Main_hall_door_anim.at(region).direction = GENERIC_ANIM_DIRECTION_FORWARDS;
  //stay on last frame if we have no keyframe
- if (!Main_hall_door_anim[region].keyframe) {
- Main_hall_door_anim[region].direction += GENERIC_ANIM_DIRECTION_NOLOOP;
+ if (!Main_hall_door_anim.at(region).keyframe) {
+ Main_hall_door_anim.at(region).direction += GENERIC_ANIM_DIRECTION_NOLOOP;
  }
 
  // check for opening/starting sounds
  // kill the currently playing sounds if necessary
- if (Main_hall_door_sound_handles[region] != -1) {
- snd_stop(Main_hall_door_sound_handles[region]);
+ if (Main_hall_door_sound_handles.at(region) != -1) {
+ snd_stop(Main_hall_door_sound_handles.at(region));
  }
- Main_hall_door_sound_handles[region] = snd_play(&Snds_iface[Main_hall->door_sounds.at(region).at(0)],Main_hall->door_sound_pan.at(region));
+ Main_hall_door_sound_handles.at(region) = snd_play(&Snds_iface[Main_hall->door_sounds.at(region).at(0)],Main_hall->door_sound_pan.at(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) ) {
- snd_set_pos(Main_hall_door_sound_handles[region],&Snds_iface[SND_MAIN_HALL_DOOR_OPEN],
- (float)Main_hall_door_anim[region].current_frame / (float)Main_hall_door_anim[region].num_frames,1);
+ if ( (Main_hall_door_anim.at(region).num_frames > 0) && (Main_hall_door_anim.at(region).current_frame != -1) ) {
+ snd_set_pos(Main_hall_door_sound_handles.at(region),&Snds_iface[SND_MAIN_HALL_DOOR_OPEN],
+ (float)Main_hall_door_anim.at(region).current_frame / (float)Main_hall_door_anim.at(region).num_frames,1);
  }
 }
 
@@ -1384,9 +1399,10 @@
 {
  int idx;
  // basically just set the handle of any finished sound to be -1, so that we know its free any where else in the code we may need it
- for (idx=0;idx<Main_hall->num_door_animations;idx++) {
- if ( (Main_hall_door_sound_handles[idx] != -1) && !snd_is_playing(Main_hall_door_sound_handles[idx]) ) {
- Main_hall_door_sound_handles[idx] = -1;
+ Assert(Main_hall_door_sound_handles.size() < INT_MAX);
+ for (idx=0;idx<(int)Main_hall_door_sound_handles.size();idx++) {
+ if ( (Main_hall_door_sound_handles.at(idx) != -1) && !snd_is_playing(Main_hall_door_sound_handles.at(idx)) ) {
+ Main_hall_door_sound_handles.at(idx) = -1;
  }
  }
 }
@@ -1795,11 +1811,6 @@
  required_string("+Num Intercom Sounds:");
  stuff_int(&m->num_random_intercom_sounds);
 
- if (m->num_random_intercom_sounds > MAX_RANDOM_INTERCOM_SOUNDS) {
- Warning(LOCATION, "Num Intercom Sounds exceeds maximum!");
- m->num_random_intercom_sounds = MAX_RANDOM_INTERCOM_SOUNDS;
- }
-
  //initialise intercom sounds vectors
  intercom_sounds_init(*m);
 
@@ -1824,10 +1835,6 @@
  // misc animations
  required_string("+Num Misc Animations:");
  stuff_int(&m->num_misc_animations);
- if (m->num_misc_animations > MAX_MISC_ANIMATIONS) {
- Warning(LOCATION, "Num Misc Animations exceeds maximum!");
- m->num_misc_animations = MAX_MISC_ANIMATIONS;
- }
 
  //initialise the misc anim vectors
  misc_anim_init(*m);
@@ -1914,10 +1921,6 @@
  // door animations
  required_string("+Num Door Animations:");
  stuff_int(&m->num_door_animations);
- if (m->num_door_animations > MAX_DOOR_ANIMATIONS) {
- Warning(LOCATION, "Num Door Animations exceeds maximum!");
- m->num_door_animations = MAX_DOOR_ANIMATIONS;
- }
 
  //initialise the door anim vectors
  door_anim_init(*m);

Apply patch straight to latest trunk. Thanks in advance!

EDIT: Silly CommanderDJ, there was a bug in your code! Fixed now (the just above has been updated to reflect the fixes). I have tested it more, including with misc anim groups and it all works as expected. If Mjn's tests turn out fine, I think this will be commit-ready.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on January 02, 2012, 09:19:12 am
Please note the above post was edited to reflect changes in the patch.

Now, I will probably not start work on the next change (introducing name identifiers for mainhalls) until after the 17th of January, as from the 8th I'm going on an interstate holiday until then. Name identifiers will without a doubt be the biggest change this project will introduce to the code, since they'll be replacing the mainhall number as the code's identifier for a particular mainhall. This will affect (obviously) the mainhall code, it will affect scripting, it will affect the pilot code, it will affect parts of the UI. Whilst I'm by now very familiar with the mainhall (and associated parsing) code, I've not yet touched the other three (and there's most likely other areas which I'm not yet aware that I'll hit with this). Such a widespread change will likely take some time to implement by itself, not to mention keeping backwards compatibility so I don't accidentally the game.

The parsing of the name identifier itself and its storage in the mainhall struct will be pretty easy, so I'll try and get that written up and hopefully committed before I leave on the 8th. We'll see how we go.
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on January 02, 2012, 01:59:11 pm
Built on latest Trunk. CTDs on startup. Log attached.

[attachment deleted by a ninja]
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on January 07, 2012, 07:13:45 am
Hello.
I'm leaving for an interstate trip tomorrow morning for 9 days. mjn.mixael's crashes still have not been resolved as I cannot seem to reproduce the problem. It does seem he's hitting an out_of_range exception thrown by an invalid .at() access, from what I can tell, but it still boggles me that I don't hit the same thing, even when we're both using retail tables. If anyone is interested, please do test to see if you also get the crash. I think it'd be helpful if we got a third opinion here (well, not opinion, but you know what I mean). I've attached a slightly updated patch to provide a little bit more detailed logging to the fs2_open.log, but it hasn't helped much in identifying the problem, unfortunately. If no one else tests by the time I get back I will do my best to debug the problem with mjn, though I'd appreciate it if a coder (or anyone, really) helped out. Thank you all for your patience.


[attachment deleted by a ninja]
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on January 07, 2012, 10:31:55 am
Good news! I'm not getting any crashes with this patch. Tried with -reparse_mainhall as well. I also tested removing +Misc Anim Handles and +Misc Anim Flags. No issues found anywhere as far as I can tell. Everything works as I would expect.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on January 09, 2012, 01:20:19 am
Oh that's brilliant! Really good news. Before I left I tested it on my end and it all worked for me too, so that's good to hear. Hopefully a coder will take a look at the patch soon and evaluate it.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on January 18, 2012, 10:18:19 am
So I got back home last night. I will hopefully if I'm not too lazy start work on this again in the next few days. The above patch still works fine against latest trunk, so if a coder has a spare moment could it please be committed? I'll be on IRC when I'm free so I'll bug whoever's on there eventually :P.

EDIT: Limit removal patch committed by Zacam to trunk in r8267.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on January 25, 2012, 12:09:31 am
Status update: I've decided to combine the last two features of dynamic mainhalls into one release. If you recall from waaay back in the thread:
Quote
3. Introducing name identifiers for mainhalls and updating the code to use these to refer to mainhalls rather than indexes, and adding the accompanying +Name flag to mainhall.tbl.
4. Changing the mainhall array to a vector and thus making mainhalls dynamic, and updating the code accordingly.

I've decided to work on both of these simultaneously since number 4 makes number 3 a lot easier in some departments, and yet I can't introduce number 4 without having number 3 in the first place because of the inherent danger in assuming specific indices for mainhalls which, when Main_hall_defines is a vector, cannot be guaranteed. I'm slowly becoming more familiar with the areas of the code I need to work with, but am still a way off from a workable understanding it. I do have a patch that can be tested, but please keep in mind this is not a finished patch. Also PLEASE NOTE that this is an ANTIPODES patch, so it must be applied to antipodes and tested there. I moved work to antipodes as per Zacam's recommendation due to the pilot and campaign file code (which I'm going to have to work with) there being much more robust. And since we're not a long way off from the pilot code being moved into trunk, why not?

Also, I'd really appreciate coder feedback on this patch so far. As I said, I'm still a bit unfamiliar, so I'd like to know if I've done anything I shouldn't or made assumptions or whatever. This patch vectorises Main_hall_defines, and introduces tabling and parsing of an optional +Name identifier for mainhall entries. Names cannot have duplicates within the same resolution. So, for example, you could have both the 640x480 and 1024x768 entries for the Aquitaine mainhall have the name "Aquitaine", but you couldn't name the Psamptik the same thing. The code will deal with it if you do make that mistake, but it will also yell at you a little bit.

Here it is:
Code: [Select]
Index: code/freespace2/freespace.cpp
===================================================================
--- code/freespace2/freespace.cpp (revision 8370)
+++ code/freespace2/freespace.cpp (working copy)
@@ -158,8 +158,6 @@
 #include "fs2netd/fs2netd_client.h"
 #include "pilotfile/pilotfile.h"
 
-#include "globalincs/pstypes.h"
-
 #include <stdexcept>
 
 extern int Om_tracker_flag; // needed for FS2OpenPXO config
Index: code/menuui/mainhallmenu.cpp
===================================================================
--- code/menuui/mainhallmenu.cpp (revision 8370)
+++ code/menuui/mainhallmenu.cpp (working copy)
@@ -58,100 +58,10 @@
 #define MISC_ANIM_MODE_TIMED 2 // uses timestamps to determine when a finished anim should be checked again
 
 #define NUM_REGIONS 7 // (6 + 1 for multiplayer equivalent of campaign room)
-typedef struct main_hall_defines {
- // bitmap and mask
- SCP_string bitmap;
- SCP_string mask;
 
- // music
- SCP_string music_name;
- SCP_string substitute_music_name;
+SCP_vector<SCP_vector<main_hall_defines> > Main_hall_defines;
+main_hall_defines *Main_hall = NULL;
 
- // intercom defines -------------------
-
- // # of intercom sounds
- int num_random_intercom_sounds;
-
- // random (min/max) delays between playing intercom sounds
- SCP_vector<SCP_vector<int> > intercom_delay;
-
- // intercom sounds themselves
- SCP_vector<int> intercom_sounds;
-
- // intercom sound pan values
- SCP_vector<float> intercom_sound_pan;
-
-
- // misc animations --------------------
-
- // # of misc animations
- int num_misc_animations;
-
- // filenames of the misc animations
- SCP_vector<SCP_string> misc_anim_name;
-
- // Time until we will next play a given misc animation, min delay, and max delay
- SCP_vector<SCP_vector<int> > misc_anim_delay;
-
- // Goober5000, used in preference to the flag in generic_anim
- SCP_vector<int> misc_anim_paused;
-
- // Goober5000, used when we want to play one of several anims
- SCP_vector<int> misc_anim_group;
-
- // coords of where to play the misc anim
- SCP_vector<SCP_vector<int> > misc_anim_coords;
-
- // misc anim play modes (see MISC_ANIM_MODE_* above)
- SCP_vector<int> misc_anim_modes;
-
- // panning values for each of the misc anims
- SCP_vector<float> misc_anim_sound_pan;
-
- //sounds for each of the misc anims
- SCP_vector<SCP_vector<int> > misc_anim_special_sounds;
-
- //frame number triggers for each of the misc anims
- SCP_vector<SCP_vector<int> > misc_anim_special_trigger;
-
- //flags for each of the misc anim sounds
- SCP_vector<SCP_vector<int> > misc_anim_sound_flag;
-
-
- // door animations --------------------
-
- // # of door animations
- int num_door_animations;
-
- // filenames of the door animations
- SCP_vector<SCP_string> door_anim_name;
-
- // first pair : coords of where to play a given door anim
- // second pair : center of a given door anim in windowed mode
- SCP_vector<SCP_vector<int> > door_anim_coords;
-
- // sounds for each region (open/close)
- SCP_vector<SCP_vector<int> > door_sounds;
-
- // pan values for the door sounds
- SCP_vector<float> door_sound_pan;
-
-
- // region descriptions ----------------
-
- // text (tooltip) description
- SCP_vector<char*> region_descript;
-
- // y coord of where to draw tooltip text
- int region_yval;
-
-} main_hall_defines;
-
-
-// use main hall 0 by default
-main_hall_defines Main_hall_defines[GR_NUM_RESOLUTIONS][MAIN_HALLS_MAX];
-main_hall_defines *Main_hall = &Main_hall_defines[0][0];
-
 int Num_main_halls = 0;
 
 int Vasudan_funny = 0;
@@ -518,7 +428,7 @@
 
  // assign the proper main hall data
  Assert((main_hall_num >= 0) && (main_hall_num < Num_main_halls));
- Main_hall = &Main_hall_defines[gr_screen.res][main_hall_num];
+ Main_hall = &Main_hall_defines.at(gr_screen.res).at(main_hall_num);
 
  // tooltip strings
  Main_hall->region_descript.at(0) = XSTR( "Exit FreeSpace 2", 353);
@@ -604,7 +514,7 @@
  }
 
  // load in help overlay bitmap
- if (Main_hall == &Main_hall_defines[gr_screen.res][0]) {
+ if (Main_hall == &Main_hall_defines.at(gr_screen.res).at(0)) {
  Main_hall_overlay_id = MH_OVERLAY;
  } else {
  //Assert(Main_hall == &Main_hall_defines[gr_screen.res][1]);
@@ -1036,7 +946,7 @@
  return -1;
  }
 
- hall = &Main_hall_defines[gr_screen.res][main_hall_num];
+ hall = &Main_hall_defines.at(gr_screen.res).at(main_hall_num);
 
  // Goober5000 - try substitute first
  index = event_music_get_spooled_music_index(hall->substitute_music_name);
@@ -1073,7 +983,7 @@
  }
 
  // get music
- index = main_hall_get_music_index(Main_hall-Main_hall_defines[gr_screen.res]);
+ index = main_hall_get_music_index(main_hall_id());
  if (index < 0) {
  nprintf(("Warning", "No music file exists to play music at the main menu!\n"));
  return;
@@ -1601,12 +1511,42 @@
  gr_string((gr_screen.max_w_unscaled - w)/2, Main_hall_tooltip_padding[gr_screen.res] /*- y_anim_offset*/, str);
 }
 
+/**
+ * CommanderDJ - finds the mainhall struct whose name is equal to the passed string
+ * @param name_to_find Name of mainhall we're searching for
+ * @param size Size of array to search
+ *
+ * \return pointer to mainhall if one with a matching name is found
+ * \return NULL otherwise
+ */
+main_hall_defines* main_hall_get_by_name(SCP_string name_to_find, int size)
+{
+ for(int i=0; i<size; i++)
+ {
+ if(Main_hall_defines.at(gr_screen.res).at(i).name == name_to_find)
+ return &Main_hall_defines.at(gr_screen.res).at(i);
+ }
+ return NULL;
+}
+
 // what main hall we're on
 int main_hall_id()
 {
  return (Main_hall - &Main_hall_defines[gr_screen.res][0]);
 }
 
+//helper function for initialising the Main_hall_defines vector
+//call before parsing mainhall.tbl
+void main_hall_defines_init()
+{
+ SCP_vector<main_hall_defines> temp;
+ //for each resolution we just want to put in a blank vector
+ for(int i=0; i<GR_NUM_RESOLUTIONS; i++)
+ {
+ Main_hall_defines.push_back(temp);
+ }
+}
+
 //CommanderDJ - helper function for initialising intercom sounds vectors based on number of sounds
 //To be called after num_intercom_sounds has been parsed
 void intercom_sounds_init(main_hall_defines &m)
@@ -1752,6 +1692,7 @@
 // read in main hall table
 void main_hall_read_table()
 {
+ SCP_vector<main_hall_defines> temp_vector;
  main_hall_defines *m, temp;
  int count, idx, s_idx, m_idx, rval;
  char temp_string[MAX_FILENAME_LEN];
@@ -1773,11 +1714,14 @@
 
  reset_parse();
 
+ main_hall_defines_init();
+
  // go for it
  count = 0;
  while (!optional_string("#end")) {
  // read in 2 resolutions
  for (m_idx=0; m_idx<GR_NUM_RESOLUTIONS; m_idx++) {
+ /*
  // maybe use a temp main hall struct
  if (count >= MAIN_HALLS_MAX) {
  m = &temp;
@@ -1786,11 +1730,33 @@
  break;
  } else {
  m = &Main_hall_defines[m_idx][count];
- }
+ }*/
 
+ Main_hall_defines.at(m_idx).push_back(temp);
+ m = &Main_hall_defines.at(m_idx).at(count);
+
  // ready
  required_string("$Main Hall");
 
+ //name. if no name is included, just give it a number
+ itoa(count, temp_string, 10);
+ m->name = temp_string;
+
+ if(optional_string("+Name:"))
+ {
+ stuff_string(temp_string, F_RAW, MAX_FILENAME_LEN);
+
+ //we can't have two mainhalls with the same name
+ if(main_hall_get_by_name(temp_string, count) == NULL)
+ {
+ m->name = temp_string;
+ }
+ else
+ {
+ Warning(LOCATION, "A mainhall with the name '%s' already exists. All mainhalls must have unique names. Setting name to %i.", temp_string, count);
+ }
+ }
+
  // bitmap and mask
  required_string("+Bitmap:");
  stuff_string(temp_string, F_NAME, MAX_FILENAME_LEN);
@@ -1959,10 +1925,8 @@
  required_string("+Tooltip Y:");
  stuff_int(&m->region_yval);
  }
-
- if (count < MAIN_HALLS_MAX) {
- count++;
- }
+
+ count++;
  }
 
  Num_main_halls = count;
@@ -1971,17 +1935,17 @@
  if (Vasudan_funny) {
  int hall = main_hall_id();
 
- 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;
+ Main_hall_defines.at(GR_640).at(hall).door_sounds.at(OPTIONS_REGION).at(0) = SND_VASUDAN_BUP;
+ Main_hall_defines.at(GR_640).at(hall).door_sounds.at(OPTIONS_REGION).at(1) = SND_VASUDAN_BUP;
+ Main_hall_defines.at(GR_1024).at(hall).door_sounds.at(OPTIONS_REGION).at(0) = SND_VASUDAN_BUP;
+ Main_hall_defines.at(GR_1024).at(hall).door_sounds.at(OPTIONS_REGION).at(1) = SND_VASUDAN_BUP;
 
  // set head anim. hehe
- Main_hall_defines[GR_1024][hall].door_anim_name.at(OPTIONS_REGION) = "2_vhallheads";
+ Main_hall_defines.at(GR_1024).at(hall).door_anim_name.at(OPTIONS_REGION) = "2_vhallheads";
 
  // set the background
- Main_hall_defines[GR_640][hall].bitmap = "vhallhead";
- Main_hall_defines[GR_1024][hall].bitmap = "2_vhallhead";
+ Main_hall_defines.at(GR_640).at(hall).bitmap = "vhallhead";
+ Main_hall_defines.at(GR_1024).at(hall).bitmap = "2_vhallhead";
  }
 
  // free up memory from parsing the mainhall tbl
Index: code/menuui/mainhallmenu.h
===================================================================
--- code/menuui/mainhallmenu.h (revision 8370)
+++ code/menuui/mainhallmenu.h (working copy)
@@ -7,14 +7,109 @@
  *
 */
 
+#include "globalincs/globals.h"
+#include "globalincs/pstypes.h"
 
-
 #ifndef _MAIN_HALL_MENU_HEADER_FILE
 #define _MAIN_HALL_MENU_HEADER_FILE
 
 // the # of main halls we're supporting
 #define MAIN_HALLS_MAX 10 // Goober5000 - bumped down to 10; don't go above 256 (size of ubyte)
 
+typedef struct main_hall_defines {
+ // mainhall name identifier
+ SCP_string name;
+
+ // bitmap and mask
+ SCP_string bitmap;
+ SCP_string mask;
+
+ // music
+ SCP_string music_name;
+ SCP_string substitute_music_name;
+
+ // intercom defines -------------------
+
+ // # of intercom sounds
+ int num_random_intercom_sounds;
+
+ // random (min/max) delays between playing intercom sounds
+ SCP_vector<SCP_vector<int> > intercom_delay;
+
+ // intercom sounds themselves
+ SCP_vector<int> intercom_sounds;
+
+ // intercom sound pan values
+ SCP_vector<float> intercom_sound_pan;
+
+
+ // misc animations --------------------
+
+ // # of misc animations
+ int num_misc_animations;
+
+ // filenames of the misc animations
+ SCP_vector<SCP_string> misc_anim_name;
+
+ // Time until we will next play a given misc animation, min delay, and max delay
+ SCP_vector<SCP_vector<int> > misc_anim_delay;
+
+ // Goober5000, used in preference to the flag in generic_anim
+ SCP_vector<int> misc_anim_paused;
+
+ // Goober5000, used when we want to play one of several anims
+ SCP_vector<int> misc_anim_group;
+
+ // coords of where to play the misc anim
+ SCP_vector<SCP_vector<int> > misc_anim_coords;
+
+ // misc anim play modes (see MISC_ANIM_MODE_* above)
+ SCP_vector<int> misc_anim_modes;
+
+ // panning values for each of the misc anims
+ SCP_vector<float> misc_anim_sound_pan;
+
+ //sounds for each of the misc anims
+ SCP_vector<SCP_vector<int> > misc_anim_special_sounds;
+
+ //frame number triggers for each of the misc anims
+ SCP_vector<SCP_vector<int> > misc_anim_special_trigger;
+
+ //flags for each of the misc anim sounds
+ SCP_vector<SCP_vector<int> > misc_anim_sound_flag;
+
+
+ // door animations --------------------
+
+ // # of door animations
+ int num_door_animations;
+
+ // filenames of the door animations
+ SCP_vector<SCP_string> door_anim_name;
+
+ // first pair : coords of where to play a given door anim
+ // second pair : center of a given door anim in windowed mode
+ SCP_vector<SCP_vector<int> > door_anim_coords;
+
+ // sounds for each region (open/close)
+ SCP_vector<SCP_vector<int> > door_sounds;
+
+ // pan values for the door sounds
+ SCP_vector<float> door_sound_pan;
+
+
+ // region descriptions ----------------
+
+ // text (tooltip) description
+ SCP_vector<char*> region_descript;
+
+ // y coord of where to draw tooltip text
+ int region_yval;
+
+} main_hall_defines;
+
+extern SCP_vector<SCP_vector<main_hall_defines> > Main_hall_defines;
+
 // initialize the main hall proper
 void main_hall_init(int main_hall_num);
 
Index: code/menuui/playermenu.cpp
===================================================================
--- code/menuui/playermenu.cpp (revision 8370)
+++ code/menuui/playermenu.cpp (working copy)
@@ -1236,7 +1236,7 @@
  if (Dc_arg_type & ARG_INT) {
  int idx = Dc_arg_int;
 
- if (idx < 0 || idx >= MAIN_HALLS_MAX) {
+ if (idx < 0 || idx >= Main_hall_defines.at(gr_screen.res).size()) {
  dc_printf("Main hall index out of range\n");
  } else {
  Player_select_force_main_hall = idx;

Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on January 25, 2012, 08:37:30 am
Things are working as I'd expect.
Title: Re: Modular Mainhall.tbl
Post by: Iss Mneur on January 25, 2012, 12:40:35 pm
Delete code that is redundant, don't comment it out, and especially don't comment it out with /* */, use #if 0 #endif because the preprocessor macros can be recursive, multi-line comments cannot.

Code: [Select]
int main_hall_id()
 {
  return (Main_hall - &Main_hall_defines[gr_screen.res][0]);
 }
I don't think this does the same thing that it did when Main_hall_defines was a struct.

Code: [Select]
+typedef struct main_hall_defines {
...
+} main_hall_defines;
+
+extern SCP_vector<SCP_vector<main_hall_defines> > Main_hall_defines;
Symbols that only differ by case strike me as a bad idea.  Also, this is C++ code, the typedef is redundant.

Otherwise, it looks okay stylewise. I haven't tested it yet.
Title: Re: Modular Mainhall.tbl
Post by: Goober5000 on January 25, 2012, 07:10:52 pm
Code: [Select]
int main_hall_id()
 {
  return (Main_hall - &Main_hall_defines[gr_screen.res][0]);
 }
I don't think this does the same thing that it did when Main_hall_defines was a struct.

Yeah, results are probably undefined if Main_hall_defines is a vector.  You could replace it with an indexof on the Main_hall variable.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on January 26, 2012, 12:42:38 am
I have rectified the main_hall_id() function.

As for the typedef, if I remove it the compiler absolutely screams at me.

Delete code that is redundant, don't comment it out, and especially don't comment it out with /* */, use #if 0 #endif because the preprocessor macros can be recursive, multi-line comments cannot.

I'm not sure I understand you on this point. Could you please elaborate? That said, I have deleted the code that is no longer being used because of my changes.

For Main_hall_defines, what would you suggest I rename it to?

In other news, I have successfully removed the need for the player::main_hall variable. Mainhalls are now loaded purely based on what campaign file is being used. All I have to do now is start enforcing use of the name identifier for them, made slightly easier by a few helper functions I've written.

Also, in removing the player::main_hall var, I broke a lua function (getMainHall I believe). Will need to see about fixing that, for now it's just commented out.
Title: Re: Modular Mainhall.tbl
Post by: Iss Mneur on January 26, 2012, 10:13:30 am
As for the typedef, if I remove it the compiler absolutely screams at me.
Screams about what?

Delete code that is redundant, don't comment it out, and especially don't comment it out with /* */, use #if 0 #endif because the preprocessor macros can be recursive, multi-line comments cannot.
I'm not sure I understand you on this point. Could you please elaborate? That said, I have deleted the code that is no longer being used because of my changes.
I am referring to these two added comment characters.
Code: [Select]
for (m_idx=0; m_idx<GR_NUM_RESOLUTIONS; m_idx++) {
+ /*
  // maybe use a temp main hall struct
  if (count >= MAIN_HALLS_MAX) {
  m = &temp;
@@ -1786,11 +1730,33 @@
  break;
  } else {
  m = &Main_hall_defines[m_idx][count];
- }
+ }*/
Unless I misunderstand what you want elaboration on.


For Main_hall_defines, what would you suggest I rename it to?
Rename the type to main_hall_defines_t or something.
Title: Re: Modular Mainhall.tbl
Post by: Goober5000 on January 27, 2012, 12:09:31 am
For Main_hall_defines, what would you suggest I rename it to?
Follow the example used in the code.  For example, you have ship and Ships.  So use main_hall_define and Main_hall_defines.  This differentiates using both case and pluralization.

Quote
All I have to do now is start enforcing use of the name identifier for them, made slightly easier by a few helper functions I've written.
Um, no.  You want to remain compatible with all those mods that used main hall indexes.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on January 27, 2012, 12:42:18 am
Quote
All I have to do now is start enforcing use of the name identifier for them, made slightly easier by a few helper functions I've written.
Um, no.  You want to remain compatible with all those mods that used main hall indexes.

Of course I do, however the whole reason name identifiers were suggested before making mainhalls dynamic is that indexes will no longer be a reliable method of accessing mainhalls. I'm going to be implementing the methods suggested earlier in the thread (that is, if no name is supplied, the mainhall's index will be set as its name). This maintains backward and retail compatibility (since for example with FSPort, Galatea will be given the name "0" and Bastion will be given "1").
Title: Re: Modular Mainhall.tbl
Post by: Goober5000 on January 27, 2012, 01:24:28 am
And what will you do with campaign files that only refer to the mainhall index?

EDIT: Okay, I see what you mean.  That should work, provided the main halls are always loaded in a predictable order.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on February 01, 2012, 06:18:01 am
Hey guys. Bad news. The more I try and wrangle with this, the more I feel like this time I've gotten in over my head. Here's the situation: so far, my changes since the last commit have introduced mainhall name identifiers and a few helper functions to go with them, turned Main_hall_defines into a vector, removed the player::mainhall var and started using the cmission::mainhall one exclusively, and made changes to the campaign editor window to accommodate for the name identifiers. Oh and I broke a LUA/scripting function which I don't know how to fix, so I've commented it out for now.

Right now things seem to work in-game, though I won't be completely sure of this until I actually play a campaign through and get to a mainhall change, but campaign/pilot switching seems to work correctly. The main problem I'm having is with FRED: it doesn't seem to want to save the new mainhall names to campaign files. I thought I had made the necessary changes, but the campaign file always ends up with an empty string for the mainhall name after saves, and I can't seem to find what I'm doing wrong. So I'm appealing to the SCP for help. Any information/advice you could give me would be awesome because I'm kinda stuck. I do want to get this finished as it's something that's been in the works for a while and I don't want to disappoint mjn.mixael and everyone else.

I will attach the patch I have so far. It's quite large as the changes are quite widespread across the code as I mentioned they would be. I'm posting it primarily for code review, and hopefully advice on what I'm missing to make FRED play nice. People can test it if they want, but I do not guarantee that everything will function correctly and I don't take responsibility for any explosions or Shivan invasions that may result. Also, no idea why there's so many fred.rc "changes" in the patch, I only edited two or three dialogs.

If a coder has any insights I'm all ears. Thank you for your time.

[attachment deleted by a ninja]
Title: Re: Modular Mainhall.tbl
Post by: Iss Mneur on February 02, 2012, 01:06:10 am
Don't use Int3 ever.  Make it an Error with a useful message (like say the actual value of m_idx).  An Int3 will cause FSO on a machine without a debugger to just crash to desktop.

You can't just not read or write a value in the pilot file without changing the file version.  You need to write a sane safe value and read then throw away.

I don't think there you can convert a CString to a SCP_string like this:
Code: [Select]
Campaign.missions[Cur_campaign_mission].main_hall = (SCP_string) str;
You probably need to use something like this:
Code: [Select]
Campaign.missions[Cur_campaign_mission].main_hall = SCP_string(str.c_str());
I didn't have time tonight to fight with the campaign editor to get to actually test this hypothesis though.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on February 02, 2012, 11:05:12 pm
Don't use Int3 ever.  Make it an Error with a useful message (like say the actual value of m_idx).  An Int3 will cause FSO on a machine without a debugger to just crash to desktop.
Rectified.

You can't just not read or write a value in the pilot file without changing the file version.  You need to write a sane safe value and read then throw away.
Do you mean that I shouldn't remove the mainhall var from the player struct, and just put something there even though it will never be used? Or am I misunderstanding you?

I don't think there you can convert a CString to a SCP_string like this:
Code: [Select]
Campaign.missions[Cur_campaign_mission].main_hall = (SCP_string) str;
You probably need to use something like this:
Code: [Select]
Campaign.missions[Cur_campaign_mission].main_hall = SCP_string(str.c_str());
I didn't have time tonight to fight with the campaign editor to get to actually test this hypothesis though.
CStrings don't seem to have a c_str() member function, so this method won't work. Is this the same CString type as this one (http://msdn.microsoft.com/en-us/library/awkwbzyc%28v=vs.80%29.aspx)?
Title: Re: Modular Mainhall.tbl
Post by: Zacam on February 03, 2012, 10:24:19 am
With the pilot code still in Antipodes, changing what data (for now) that is read to or from the pilot file is perfectly acceptable.

Once it goes to Trunk, that is another matter because then MORE people will be using it and we don't want breakage by corruption.

Currently, as the Pilot code is in testing, changes are expected.
Title: Re: Modular Mainhall.tbl
Post by: Iss Mneur on February 03, 2012, 01:43:07 pm
You can't just not read or write a value in the pilot file without changing the file version.  You need to write a sane safe value and read then throw away.
Do you mean that I shouldn't remove the mainhall var from the player struct, and just put something there even though it will never be used? Or am I misunderstanding you?
No. I mean that you shouldn't remove the mainhall value from the pilot file reading and writing code. When you change the number of fields or size of fields in a binary file format you have to convert the fileformat to the new version so that one of the reads doesn't read in garbage or its neighbours value.

However, as Zacam noted, I hadn't considered that we are talking about antipodes here, so its not as big of deal if strange behaviour and/or crashes result from changing the pilot file format, so don't worry about it (though if you are still not sure what I am getting at I will elaborate with an example, just so that we are on the same page in the future).  I would however note in the commit message that this does change the format of the pilot file.

CStrings don't seem to have a c_str() member function, so this method won't work. Is this the same CString type as this one (http://msdn.microsoft.com/en-us/library/awkwbzyc%28v=vs.80%29.aspx)?
That is the same CString.  So according to msdn what my suggested fix would actually be is:
Code: [Select]
Campaign.missions[Cur_campaign_mission].main_hall = SCP_string((LPCTSTR)str);

Unfortunately, I didn't get a chance to test my hypothesis last night so I don't know if that is the actual fix or not.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on February 03, 2012, 10:16:11 pm


Well, I think I may have some good news, finally. IssMneur's suggestion (which I have implemented and which seems to work) got me thinking about another part of the code that was giving me an access violation bug, and I realised that I had to convert from an SCP_string to a C-style string before writing to the campaign file during the save process. Once I did that, lo and behold, everything worked! At least everything I have tested so far works. I'll put the patch I'm using up so that people can test, because this will need rigorous testing by at least two other people apart from me. I know mjn.mixael will participate, but I'd like another viewpoint as well just in case.

NOTE: This is an ANTIPODES patch. Do NOT apply this patch to trunk!

EDIT: What this patch actually does as far as the modder is concerned: Firstly, we can now have a dynamic number of mainhalls, so potentially as many as you want. Secondly, name identifiers have been added, so you can add +Name entries to your mainhall.tbl entries (the names for both resolutions of the same entry must match). Syntax is just +Name: (string), added directly under the $Main Hall string in mainhall.tbl. You only need to add a name to the 640x480 entry and it will be automatically applied to the 1024x768 entry. You will notice the campaign editor in FRED has changed slightly and you must now use the name identifiers to distinguish your mainhalls. Mainhalls without names will be given numbers automatically, so using 0 and 1 for the retail mainhalls as was previously done will still work.

What you should be testing for: first, when playing retail or existing mods, everything should function as it did previously. This goes for both in-game and FRED-side, though obviously the UI has changed slightly as mentioned before. In particular, a mainhall changing campaign needs to be tested to see if the mainhalls do change correctly during the course of the campaign.
Name identifiers should be tested. Try omitting names, try giving different resolutions of the same mainhall different names (the code should warn you and compensate). Try using your mainhall names in the campaign editor and see if the game brings up the correct mainhall.
Lastly, Mjn's probably the only one in a position to test this, but try testing with a large number of mainhalls in mainhall.tbl, seeing as we should now be able to have as many as we want.

More info will be posted as I think of it, but that's the main part.

[attachment deleted by a ninja]
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on February 03, 2012, 11:36:56 pm
All tests done on a mainhall.tbl with 10 mainhalls.

Nice freaking work.  :yes: :yes:
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on February 04, 2012, 01:15:54 am
  • Fourth test: Added +Name to all mainhalls to see what would load when mainhall 0 is called. FSO couldn't find '0' and tried to load '0' then crashed. It might be worth putting error checking in here. If no Mainhall '0' is found, load the first available mainhall.
  • Ninth test: Tried loading a mainhall.tbl with duplicate entries. FSO forced the duplicate to '0' but then '0' wouldn't load, so it seems the forced rename didn't stick.

These two problems have been fixed. Changed the behaviour to load the first available mainhall as a failsafe rather than searching for '0'. Will fix the rest of the bugs later tonight.
Title: Re: Modular Mainhall.tbl
Post by: Goober5000 on February 05, 2012, 11:30:32 pm
Third test: Added +Name to the first mainhall to see what would load when mainhall 0 is called. It loaded the next mainhall that wasn't named.
Ackpth.  This isn't what I would expect.  If none of the main halls had a name of '0', I would expect the code to load the 0th main hall (i.e. the first one) in the table.
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on February 06, 2012, 12:12:12 am
That has been changed, but that behavior is exactly what I would expect originally. Because the first unnamed mainhall gets autonamed to '0'. So when it tries to find '0' no matter the reason, that's the one that loads.

However, CommanderDJ has already changed that check for '0' to just load the first one.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on February 06, 2012, 12:27:51 am
That has been changed, but that behavior is exactly what I would expect originally. Because the first unnamed mainhall gets autonamed to '0'. So when it tries to find '0' no matter the reason, that's the one that loads.

However, CommanderDJ has already changed that check for '0' to just load the first one.

This. Yeah, I realised after mjn's testing that it was kinda stupid to default to '0'.

Anyway, the only bug remaining is the end-campaign thing. Will probably work on that today or tomorrow.
Title: Re: Modular Mainhall.tbl
Post by: Goober5000 on February 06, 2012, 01:57:27 am
Because the first unnamed mainhall gets autonamed to '0'.
This is what I disagree with.  I say that the name should default to the index of all main halls, not the index of just the unnamed ones.  Take this list for example:

0: Name A
1: Name B
2: Name C
3: <unnamed>
4: Name D

If someone specified Main Hall 0, I would expect the game to load Name A.  And if someone specified Main Hall 3, I would expect the game to load the unnamed main hall between C and D.  This is the behavior that is most consistent with established main hall usage.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on February 06, 2012, 05:42:12 am
Oh, I see. Yes, that sounds more logical. I'll work that in.
Title: Re: Modular Mainhall.tbl
Post by: Goober5000 on February 06, 2012, 12:02:15 pm
Yay. :)  This will also allow FSPort to maintain backwards compatibility with previous releases.
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on February 07, 2012, 01:46:34 am
So wait.. under this method, you could reference mainhalls by name OR by index? Then I suppose if a mainhall is called by name, but that name doesn't exist in the table, then it just loads the first mainhall.. so that code could have remained the same (calling '0')... Am I understanding correctly?
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on February 07, 2012, 04:34:59 am
No. Once names are in, in any new mods mainhalls should not be referred to by index because once mainhall.tbl is modular you cannot guarantee that a particular mainhall will end up at a particular index. If a mainhall doesn't have a name, the index it gets upon parse is given to it as a name, and this is done only to maintain backward and retail compatibility. If a mainhall has a name, trying to call it by index will not work. If it doesn't, then you can try and hope that it gets that index every time you load your mod, but I strongly discourage this. The whole point of names is that mainhalls aren't referred to by index anymore, except in cases like retail where we know exactly what mainhalls are going to be loaded and where.

EDIT: Final bug almost fixed, just trying to figure out a way to check for end of campaign that isn't a damn hack.
Title: Re: Modular Mainhall.tbl
Post by: Goober5000 on February 07, 2012, 02:37:52 pm
:nod:
Title: Re: Modular Mainhall.tbl
Post by: Aardwolf on February 07, 2012, 03:25:21 pm
Huzzah!  :yes:
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on February 07, 2012, 09:05:22 pm
Okay, so here's the latest patch. I believe it fixes all the bugs mjn.mixael pointed out as well as implements the behaviour changes mentioned earlier. Mjn, if you would be kind enough to run it through your previous set of tests, that would be brilliant.

EDIT: Holy ****sauce, I'm now SCP! Thanks guys!

EDIT2: Patch updated.

EDIT3: Patch updated again.

EDIT4: Slight clean up to the patch

[attachment deleted by a ninja]
Title: Re: Modular Mainhall.tbl
Post by: Goober5000 on February 07, 2012, 09:15:39 pm
You are earning it in splendid fashion.  Well done. :)
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on February 07, 2012, 10:32:16 pm
Tested.

When a mainhall is called for that isn't found, it loads the first mainhall instead of '0' as expected. Found no issues here.

End-Campaign worked without errors.

The new index ordering works correctly.

Resolution name conflicts result in a warning and FSO not continuing. I assume that's desired behavior. I am curious if it might be better for it to give the warning and then simply ignore the 1024 +Name and run like usual. This will result on loading the mainhall by the 640 +Name if called.. and if the 1024 +Name is called it will just load the first available mainhall instead. Just a thought, the warning and not continuing certainly means that it will be fixed, so I'm cool with that too. The Warning message is clear and precise, so this is good. Also worth noting here is that no where in the warning or in the debug log, does it specify which mainhall entry has the issue. Might specify by the entry's 640 name in the log?

Duplicate mainhalls resulted FSO warning and closing, which is good. The warning message even tells you which mainhall +Name is duplicated.

 :yes:
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on February 07, 2012, 10:45:58 pm
Resolution name conflicts result in a warning and FSO not continuing. I assume that's desired behavior.
Yes it is.

I am curious if it might be better for it to give the warning and then simply ignore the 1024 +Name and run like usual. This will result on loading the mainhall by the 640 +Name if called.. and if the 1024 +Name is called it will just load the first available mainhall instead.
The issue here is that we want the modder to avoid getting the wrong mainhall if at all possible. If we change the name (or ignore it), the modder might still be referring to the incorrect name in their campaign files, leading to the first available mainhall being loaded by default (with an annoying warning each time this happens). Given that the fix for name conflicts is incredibly simple (remove the 1024 name), I'd rather not give the modder the option.

Also worth noting here is that no where in the warning or in the debug log, does it specify which mainhall entry has the issue. Might specify by the entry's 640 name in the log?

Oh? When resolution conflicts arise, this is the error that I've set it to give:
Code: [Select]
The mainhall '%s' has different names for different resolutions. Both resolutions must have the same name. Either remove the hi-res entry's name entirely or set it to match the lo-res entry's name.
Where %s is replaced by the 640 entry's name. Is this the error you're getting, or are we thinking of different things?

Sounds like everything else is working fine, though.
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on February 07, 2012, 11:03:05 pm
Agreed. Also, the updated patch fixes the resolution warning issue. :yes:
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on February 08, 2012, 05:27:42 am
Zacam kindly also tested the patch and cleaned it up a little. His modified one is attached to my earlier post. It seems I don't have commit access yet, but Zacam did say he was going to take another look at this in the morning, so hopefully this will hit Antipodes soon.

EDIT: Slightly modified your modified patch, Zacam.

EDIT2: If this goes in, I've prepared a commit log message:
Code: [Select]
Introduces +Name identifiers for mainhall.tbl entries:
- +Name identifiers only need to be added to the lo-res (640x480) entry of the mainhall. If you really must add a +Name to both res versions, the two resolutions must have the exact same name.
- +Name identifiers are added immediately under the $Main Hall string
Vectorises Main_hall_defines, thus removing the MAIN_HALLS_MAX limit.
Main halls are now loaded based purely on campaign files. Hall indexes are no longer stored in the player struct, and thus this patch changes the pilot file version.

[attachment deleted by a ninja]
Title: Re: Modular Mainhall.tbl
Post by: Zacam on February 08, 2012, 08:42:53 pm
So far, everything looks good from my end. MSVC Analyze does't have anything to complain about either. General building does though, in relation to line 528 of the patch, where it encounters a signed/unsigned mismatch in 'SCP_string main_hall_get_name(int index)'. Given that I can't think of a case where we won't ever have a main hall for it to ever be less than 0, maybe making index an unsigned or size_t or something may be in order and pulling out the 'index<0' bit.

Speaking of, I happen to see a lot of usage of INT. And while that is somewhat common to use, it is certainly easy, how many of them actually -need- to be INT's? It's good that we have a dynamic limit, but I can't help but wonder if other types might not be more suitable in some areas than others. Especially in cases where we don't need negative integers because we will always have or should have -something- present, even if just the default initialized value.

I'm not suggesting that we hold up putting in until those are addressed mind you. I'm in favor for it going in as is and getting tuned and tightened up once it is, so long as we don't forget. So, as long as nobody else see's anything worth correcting (other than that signed/unsigned mentioned earlier) I'm all for putting this in.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on February 09, 2012, 12:35:57 am
Sounds good. I'm all for going through it later and optimising it a bit more. For now I've changed the index to an unsigned int to get rid of the compiler warning and removed the index<0 bit.

Latest patch is attached.

[attachment deleted by a ninja]
Title: Re: Modular Mainhall.tbl
Post by: Zacam on February 09, 2012, 08:45:48 am
Put into Antipodes r8453.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on February 10, 2012, 07:40:17 am
And so we come to the final stage of this project, the thing that the thread originally asked for: modular mainhall.tbl!
The patch to allow modular tables here is now available for testing. Once again we're working in antipodes, so don't apply this to trunk!
Modular mainhall table filenames should end in "-hall.tbm", and as with the regular table, must start with "$Main Hall" and end with "#End".
I myself will do some more thorough testing on this in the next few days, but I felt like getting this patch out there for feedback and code review purposes.



[attachment deleted by a ninja]
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on February 10, 2012, 08:21:15 am
Tested. Didn't even load a mainhall.tbl (save for retail). Separated 8/10 mainhalls into separate tbms. Kept two in one tbm to test that as well. Called them all by +Name and it worked. I didn't test calling by the automatically assigned +Name/index stuff (if +Name isn't present), but can if you think it's necessary.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on February 10, 2012, 08:27:46 am
Tested. Didn't even load a mainhall.tbl (save for retail). Separated 8/10 mainhalls into separate tbms. Kept two in one tbm to test that as well. Called them all by +Name and it worked.
That's brilliant news! I have done some more testing on this and all seems to be well. So I guess all that's left is to wait for other coders to review this and make suggestions, and hopefully after that commit it.

I didn't test calling by the automatically assigned +Name/index stuff (if +Name isn't present), but can if you think it's necessary.
I think we'll be fine. I ran through it anyway on my end and it seemed to go okay.
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on February 10, 2012, 08:58:16 am
Yeah. I didn't want to figure the tbm load order for 10 tbms to load them by index....
Title: Re: Modular Mainhall.tbl
Post by: Zacam on February 10, 2012, 02:38:02 pm
I really don't think we need to embed yet ANOTHER table into the code. Embedding "default" tables should only be for cases where we've created a new table IMO. Any TC will have its own TBL file (which is necessary) and any mod would read from the Retail one anyway.
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on February 10, 2012, 06:25:00 pm
Latest patch removing the embedded table and allowing modular files to be used without a mainhall.tbl.

[attachment deleted by a ninja]
Title: Re: Modular Mainhall.tbl
Post by: Zacam on February 10, 2012, 07:36:41 pm
Committed to Antipodes r8461
Title: Re: Modular Mainhall.tbl
Post by: mjn.mixael on February 10, 2012, 07:42:21 pm
:yes:

Many, many thanks!
Title: Re: Modular Mainhall.tbl
Post by: CommanderDJ on February 10, 2012, 09:27:30 pm
Brilliant! Thanks for the commit. Also, I want to thank all the SCP coders who gave me advice, reviewed my code and helped me along the way. Also immense thanks to mjn.mixael for his speedy, reliable and committed testing of all the stages in this project, as well as the encouragement he gave me to continue on.

If any bugs come up later related to this, I'm happy to let this thread serve as a discussion ground for them, but hopefully that won't happen. :D
Title: Re: Modular Mainhall.tbl
Post by: Goober5000 on February 11, 2012, 02:37:21 am
Well done!  This has been one of the best-engineered upgrades to FSO I've ever witnessed. :yes:
Title: Re: Modular Mainhall.tbl
Post by: chief1983 on February 13, 2012, 12:52:37 am
I'm particularly a big fan of the iterative process.  I know that it's not always as easy to do as it was in this case, but taking the effort to make it happen instead of trying to lump it all in at one time should really pay off.