Author Topic: Modular Mainhall.tbl  (Read 29310 times)

0 Members and 1 Guest are viewing this topic.

Offline Aardwolf

  • 211
  • Posts: 16,384
    • Minecraft
Huzzah!  :yes:

 

Offline CommanderDJ

  • Software engineer
  • 210
    • Minecraft
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]
« Last Edit: February 08, 2012, 01:21:06 am by Zacam »
[16:57] <CommanderDJ> What prompted the decision to split WiH into acts?
[16:58] <battuta> it was long, we wanted to release something
[16:58] <battuta> it felt good to have a target to hit
[17:00] <RangerKarl> not sure if talking about strike mission, or jerking off
[17:00] <CommanderDJ> WUT
[17:00] <CommanderDJ> hahahahaha
[17:00] <battuta> hahahaha
[17:00] <RangerKarl> same thing really, if you think about it

 

Offline Goober5000

  • HLP Loremaster
  • Moderator
  • 214
    • Goober5000 Productions
You are earning it in splendid fashion.  Well done. :)

 

Offline mjn.mixael

  • Cutscene Master
  • 212
  • Chopped liver
    • Steam
    • Twitter
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:
Cutscene Upgrade Project - Mainhall Remakes - Between the Ashes
Youtube Channel - P3D Model Box
Between the Ashes is looking for committed testers, PM me for details.
Freespace Upgrade Project See what's happening.

 

Offline CommanderDJ

  • Software engineer
  • 210
    • Minecraft
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.
[16:57] <CommanderDJ> What prompted the decision to split WiH into acts?
[16:58] <battuta> it was long, we wanted to release something
[16:58] <battuta> it felt good to have a target to hit
[17:00] <RangerKarl> not sure if talking about strike mission, or jerking off
[17:00] <CommanderDJ> WUT
[17:00] <CommanderDJ> hahahahaha
[17:00] <battuta> hahahaha
[17:00] <RangerKarl> same thing really, if you think about it

  

Offline mjn.mixael

  • Cutscene Master
  • 212
  • Chopped liver
    • Steam
    • Twitter
Agreed. Also, the updated patch fixes the resolution warning issue. :yes:
Cutscene Upgrade Project - Mainhall Remakes - Between the Ashes
Youtube Channel - P3D Model Box
Between the Ashes is looking for committed testers, PM me for details.
Freespace Upgrade Project See what's happening.

 

Offline CommanderDJ

  • Software engineer
  • 210
    • Minecraft
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]
« Last Edit: February 08, 2012, 05:38:42 am by CommanderDJ »
[16:57] <CommanderDJ> What prompted the decision to split WiH into acts?
[16:58] <battuta> it was long, we wanted to release something
[16:58] <battuta> it felt good to have a target to hit
[17:00] <RangerKarl> not sure if talking about strike mission, or jerking off
[17:00] <CommanderDJ> WUT
[17:00] <CommanderDJ> hahahahaha
[17:00] <battuta> hahahaha
[17:00] <RangerKarl> same thing really, if you think about it

 

Offline Zacam

  • Magnificent Bastard
  • Administrator
  • 211
  • I go Sledge-O-Matic on Spammers
    • Minecraft
    • Steam
    • Twitter
    • ModDB Feature
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.
Report MediaVP issues, now on the MediaVP Mantis! Read all about it Here!
Talk with the community on Discord
"If you can keep a level head in all this confusion, you just don't understand the situation"

¤[D+¬>

[08/01 16:53:11] <sigtau> EveningTea: I have decided that I am a 32-bit registerkin.  Pronouns are eax, ebx, ecx, edx.
[08/01 16:53:31] <EveningTea> dhauidahh
[08/01 16:53:32] <EveningTea> sak
[08/01 16:53:40] * EveningTea froths at the mouth
[08/01 16:53:40] <sigtau> i broke him, boys

 

Offline CommanderDJ

  • Software engineer
  • 210
    • Minecraft
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]
[16:57] <CommanderDJ> What prompted the decision to split WiH into acts?
[16:58] <battuta> it was long, we wanted to release something
[16:58] <battuta> it felt good to have a target to hit
[17:00] <RangerKarl> not sure if talking about strike mission, or jerking off
[17:00] <CommanderDJ> WUT
[17:00] <CommanderDJ> hahahahaha
[17:00] <battuta> hahahaha
[17:00] <RangerKarl> same thing really, if you think about it

 

Offline Zacam

  • Magnificent Bastard
  • Administrator
  • 211
  • I go Sledge-O-Matic on Spammers
    • Minecraft
    • Steam
    • Twitter
    • ModDB Feature
Put into Antipodes r8453.
Report MediaVP issues, now on the MediaVP Mantis! Read all about it Here!
Talk with the community on Discord
"If you can keep a level head in all this confusion, you just don't understand the situation"

¤[D+¬>

[08/01 16:53:11] <sigtau> EveningTea: I have decided that I am a 32-bit registerkin.  Pronouns are eax, ebx, ecx, edx.
[08/01 16:53:31] <EveningTea> dhauidahh
[08/01 16:53:32] <EveningTea> sak
[08/01 16:53:40] * EveningTea froths at the mouth
[08/01 16:53:40] <sigtau> i broke him, boys

 

Offline CommanderDJ

  • Software engineer
  • 210
    • Minecraft
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]
« Last Edit: February 10, 2012, 07:44:42 am by CommanderDJ »
[16:57] <CommanderDJ> What prompted the decision to split WiH into acts?
[16:58] <battuta> it was long, we wanted to release something
[16:58] <battuta> it felt good to have a target to hit
[17:00] <RangerKarl> not sure if talking about strike mission, or jerking off
[17:00] <CommanderDJ> WUT
[17:00] <CommanderDJ> hahahahaha
[17:00] <battuta> hahahaha
[17:00] <RangerKarl> same thing really, if you think about it

 

Offline mjn.mixael

  • Cutscene Master
  • 212
  • Chopped liver
    • Steam
    • Twitter
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.
Cutscene Upgrade Project - Mainhall Remakes - Between the Ashes
Youtube Channel - P3D Model Box
Between the Ashes is looking for committed testers, PM me for details.
Freespace Upgrade Project See what's happening.

 

Offline CommanderDJ

  • Software engineer
  • 210
    • Minecraft
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.
[16:57] <CommanderDJ> What prompted the decision to split WiH into acts?
[16:58] <battuta> it was long, we wanted to release something
[16:58] <battuta> it felt good to have a target to hit
[17:00] <RangerKarl> not sure if talking about strike mission, or jerking off
[17:00] <CommanderDJ> WUT
[17:00] <CommanderDJ> hahahahaha
[17:00] <battuta> hahahaha
[17:00] <RangerKarl> same thing really, if you think about it

 

Offline mjn.mixael

  • Cutscene Master
  • 212
  • Chopped liver
    • Steam
    • Twitter
Yeah. I didn't want to figure the tbm load order for 10 tbms to load them by index....
Cutscene Upgrade Project - Mainhall Remakes - Between the Ashes
Youtube Channel - P3D Model Box
Between the Ashes is looking for committed testers, PM me for details.
Freespace Upgrade Project See what's happening.

 

Offline Zacam

  • Magnificent Bastard
  • Administrator
  • 211
  • I go Sledge-O-Matic on Spammers
    • Minecraft
    • Steam
    • Twitter
    • ModDB Feature
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.
Report MediaVP issues, now on the MediaVP Mantis! Read all about it Here!
Talk with the community on Discord
"If you can keep a level head in all this confusion, you just don't understand the situation"

¤[D+¬>

[08/01 16:53:11] <sigtau> EveningTea: I have decided that I am a 32-bit registerkin.  Pronouns are eax, ebx, ecx, edx.
[08/01 16:53:31] <EveningTea> dhauidahh
[08/01 16:53:32] <EveningTea> sak
[08/01 16:53:40] * EveningTea froths at the mouth
[08/01 16:53:40] <sigtau> i broke him, boys

 

Offline CommanderDJ

  • Software engineer
  • 210
    • Minecraft
Latest patch removing the embedded table and allowing modular files to be used without a mainhall.tbl.

[attachment deleted by a ninja]
[16:57] <CommanderDJ> What prompted the decision to split WiH into acts?
[16:58] <battuta> it was long, we wanted to release something
[16:58] <battuta> it felt good to have a target to hit
[17:00] <RangerKarl> not sure if talking about strike mission, or jerking off
[17:00] <CommanderDJ> WUT
[17:00] <CommanderDJ> hahahahaha
[17:00] <battuta> hahahaha
[17:00] <RangerKarl> same thing really, if you think about it

 

Offline Zacam

  • Magnificent Bastard
  • Administrator
  • 211
  • I go Sledge-O-Matic on Spammers
    • Minecraft
    • Steam
    • Twitter
    • ModDB Feature
Committed to Antipodes r8461
Report MediaVP issues, now on the MediaVP Mantis! Read all about it Here!
Talk with the community on Discord
"If you can keep a level head in all this confusion, you just don't understand the situation"

¤[D+¬>

[08/01 16:53:11] <sigtau> EveningTea: I have decided that I am a 32-bit registerkin.  Pronouns are eax, ebx, ecx, edx.
[08/01 16:53:31] <EveningTea> dhauidahh
[08/01 16:53:32] <EveningTea> sak
[08/01 16:53:40] * EveningTea froths at the mouth
[08/01 16:53:40] <sigtau> i broke him, boys

 

Offline mjn.mixael

  • Cutscene Master
  • 212
  • Chopped liver
    • Steam
    • Twitter
:yes:

Many, many thanks!
Cutscene Upgrade Project - Mainhall Remakes - Between the Ashes
Youtube Channel - P3D Model Box
Between the Ashes is looking for committed testers, PM me for details.
Freespace Upgrade Project See what's happening.

 

Offline CommanderDJ

  • Software engineer
  • 210
    • Minecraft
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
[16:57] <CommanderDJ> What prompted the decision to split WiH into acts?
[16:58] <battuta> it was long, we wanted to release something
[16:58] <battuta> it felt good to have a target to hit
[17:00] <RangerKarl> not sure if talking about strike mission, or jerking off
[17:00] <CommanderDJ> WUT
[17:00] <CommanderDJ> hahahahaha
[17:00] <battuta> hahahaha
[17:00] <RangerKarl> same thing really, if you think about it

 

Offline Goober5000

  • HLP Loremaster
  • Moderator
  • 214
    • Goober5000 Productions
Well done!  This has been one of the best-engineered upgrades to FSO I've ever witnessed. :yes: