[Goober5000]

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.

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.

[CommanderDJ]

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").

[Goober5000]

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.

[CommanderDJ]

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]

[Iss Mneur]

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.

[CommanderDJ]

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?

[Zacam]

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.

[Iss Mneur]

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?

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.

[CommanderDJ]

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]

[mjn.mixael]

All tests done on a mainhall.tbl with 10 mainhalls.

• First test: No changes to my mainhall.tbl. Switching mainhalls by switching campaigns. Worked as expected.
• Second test: No changes to my mainhall.tbl. Switching mainhalls by campaign progres. Worked as expected.
• 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.EDIT: This got me thinking though. What would happen if you had mainhalls with names interspersed.. like this list.
  
  • +Name: Yarmehearties
  • No +Name
  • +Name:ImAFish
  • No +Name
  I'm fairly confident that it would number both 'no-names' incrementally, but I haven't tested this yet.
• 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.
• Fifth test: Changed all campaigns to call mainhalls by name.  Switching mainhalls by switching campaigns. Worked as expected.
• Sixth test: Changed all campaigns to call mainhalls by name. Switching mainhalls by campaign progress. Worked as expected. This time though, I thought to finish the campaign. At the end of the campaign and trying to return to the mainhall, I was hit with "main_hall_init() was passed a blank mainhall name, setting to '0'..." Might be worth making sure that the end-campaign sexps tell FSO to return to either the first mainhall of the campaign OR stay at the most recent mainhall. After this it crashed again, because I have no mainhall '0'. See Fourth test.
• Seventh test: Tried loading a mainhall that had resolution +Name conflicts by the 640 name. It loaded fine, no warnings anywhere though.
• Eighth test: Tried loading a mainhall that had resolution +Name conflicts by the 1024 name. It never checked the 1024 name and just loaded mainhall '0' because it couldn't find the name I specified. No crash is good, but for this one and the Seventh test a warning (in the log or popup) is probably good.
• 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.
  

Nice freaking work. 

[CommanderDJ]

• 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.

[Goober5000]

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.

[mjn.mixael]

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.

[CommanderDJ]

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.

[Goober5000]

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.

[CommanderDJ]

Oh, I see. Yes, that sounds more logical. I'll work that in.

[Goober5000]

Yay.   This will also allow FSPort to maintain backwards compatibility with previous releases.

[mjn.mixael]

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?

[CommanderDJ]

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.

[Goober5000]