use proper stats in barracks

[notimaginative]

The barracks should use the actual all-time/multi stats from the pilot file instead of the active campaign stats, so skip the savefile load.

[wookieejedi]

Fixes the problem in my tests, thanks!

[Goober5000]

I'm not convinced this is the correct fix.

Setting aside the all-time-stats fix for the moment, the game does need to check some information from the campaign file, including the currently allowed ships/weapons/intel entries and the player's current squadron and squadron logo.

Also, take a look at #1451. One of the things Pilot.load_savefile() does is call csg_reset_data(). One of the things #1451 does is to remove p->stats.init(); from csg_reset_data(). Is it possible that the root cause here isn't loading the campaign file per se, but rather that statement buried deep in the call stack?

The root design issue, as I see it (separate from the root cause of this particular bug) is that the pilot and campaign code are too intertwined and tangled together. Pilot code often calls campaign code and campaign code often calls pilot code. A proper design fix would be to refactor these sections of code to be clearly distinct.

[notimaginative]

Setting aside the all-time-stats fix for the moment, the game does need to check some information from the campaign file, including the currently allowed ships/weapons/intel entries and the player's current squadron and squadron logo.

I wasn't aware that the squad name and logo were in separate files as it wasn't part of the original code. So that's good to know.

And I would like to say that the currently allowed ships/weapons don't matter in this context, since the barracks doesn't need this info and when accepting a new pilot that pilot's campaign is loaded, but it points out a different bug in the code. You can't unaccept a pilot. Pressing ESC in the barracks will exit, accepting to currently displayed pilot, but not loading the campaign. So it ends up in a bad state if the savefile hasn't been loaded.

So there is definitely an extra bug at play here. Reworking the barracks code a bit more may be the best answer to both problems.

Also, take a look at #1451. One of the things Pilot.load_savefile() does is call csg_reset_data(). One of the things #1451 does is to remove p->stats.init(); from csg_reset_data(). Is it possible that the root cause here isn't loading the campaign file per se, but rather that statement buried deep in the call stack?

Honestly I believe that the p->stats.init() is still needed. Note that the content of p->stats is never saved to the plr file. The plr file intentionally only updates its stats from the just-played mission. However p->stats is important for a campaign as the progressive stats are saved for each mission. So it's very important that these remain in a sane state over time that matches the campaign progress state. When a plr is loaded it fills in p->stats with all-time/multi stats as a default. So the p->stats.init() is there to make sure none of that info isn't carried into the campaign data by mistake.

The root design issue, as I see it (separate from the root cause of this particular bug) is that the pilot and campaign code are too intertwined and tangled together. Pilot code often calls campaign code and campaign code often calls pilot code. A proper design fix would be to refactor these sections of code to be clearly distinct.

There is some definition things here that may cause me to misinterpret what you're saying. So.. the "Pilot" is to me a combination of the "Player" (plr) file and the "Campaign savefile" (csg). The general campaign code has always been intertwined with the savefile (csg) and the player (plr) stats. Those things are not now, nor have they ever really been, distinct. Having single and multi player files combined, plus keeping track info information across different mods does complication the code. Hence why it was combined under the "Pilot" umbrella. The pieces are separate, but they are also interdependent.

So I don't see this as a design issue. An incomplete solution, I can see that, but not much more. But like I said, the language/definition of things might be causing me to read something different to what you're saying.

[wookieejedi]

Just to check though, there are indeed fields that are duplicated in the campaign CSG and the player JSON, though? Such as HUD gauge color settings and the items in the settings seciton of the JSON, such as game_skill_level, joy_sensitivity, and aster_voice_volume (yes it is indeed aster_voice_volume and that typo was likely never fixed b/c it would have meant a pilot file bump :D )

[notimaginative]

Just to check though, there are indeed fields that are duplicated in the campaign CSG and the player JSON, though?

Yes, since those preferences can vary between campaigns, particularly with totally different mods. But they aren't needed in the barracks. However if you accept a new pilot in the barracks all of that info will be loaded in when you're sent back to the mainhall.

But if pressing ESC to leave the barracks, things get screwed up. So that's a new bug (to me).

[Goober5000]

I wasn't aware that the squad name and logo were in separate files as it wasn't part of the original code. So that's good to know.

Looking more closely, although the squad name and logo are read from the csg file, they are assigned to the player struct, not the campaign struct. In fact, from a design perspective, the squadron info shouldn't be read from or written to the csg file at all. Instead, the code should inspect the currently loaded campaign, check the #Mission Info section of the current campaign mission, and obtain the squadron info from there, since that is the only place that the game designer communicates that info to the game.

And I would like to say that the currently allowed ships/weapons don't matter in this context, since the barracks doesn't need this info

Fair enough.

and when accepting a new pilot that pilot's campaign is loaded, but it points out a different bug in the code. You can't unaccept a pilot. Pressing ESC in the barracks will exit, accepting to currently displayed pilot, but not loading the campaign. So it ends up in a bad state if the savefile hasn't been loaded.

Oof.

Reworking the barracks code a bit more may be the best answer to both problems.

Agreed, and in fact, the more I read, the more I think we really need to rework the player/campaign design. This is what I had to do with the model movement system, because of the assumptions, inconsistencies, and flat-out contradictions in the original code. Lafiel's animation system would not have been possible without that rewrite.

Honestly I believe that the p->stats.init() is still needed. Note that the content of p->stats is never saved to the plr file. The plr file intentionally only updates its stats from the just-played mission. However p->stats is important for a campaign as the progressive stats are saved for each mission. So it's very important that these remain in a sane state over time that matches the campaign progress state. When a plr is loaded it fills in p->stats with all-time/multi stats as a default. So the p->stats.init() is there to make sure none of that info isn't carried into the campaign data by mistake.

Well, hmm. Ok.

There is some definition things here that may cause me to misinterpret what you're saying. So.. the "Pilot" is to me a combination of the "Player" (plr) file and the "Campaign savefile" (csg). The general campaign code has always been intertwined with the savefile (csg) and the player (plr) stats. Those things are not now, nor have they ever really been, distinct. Having single and multi player files combined, plus keeping track info information across different mods does complication the code. Hence why it was combined under the "Pilot" umbrella. The pieces are separate, but they are also interdependent.

So I don't see this as a design issue. An incomplete solution, I can see that, but not much more. But like I said, the language/definition of things might be causing me to read something different to what you're saying.

To elaborate, the last time I took a close look at the plr/csg code, in the course of writing #1451, it seemed to be a tangle of overlapping stats management, with player code calling campaign code and campaign code calling player code, and blurred division of responsibilities. You've looked at the code more recently and also more thoroughly, so I may be completely off base, but my recollection is that the design was very, very messy -- to the extent that it was difficult to not only fix the bug but even to figure out where the bug was coming from in the first place.

[notimaginative]

I know it's a complicated setup but a lot of it is intentional. Definitely not preferred obviously, just how it needed to be to work. There were just so many bugs to solve and corrupted pilot files to deal with and so many edge cases to handle. And the two big rewrites that followed built upon that.

But things are in a significantly better state these days with regards to pilot files (just files in general, not code) and mods. So a fresh mind, not encumbered by the horrors of pilot files long since past, could almost certainly come up with a much better solution. (seriously, it all just needs to be sqlite3)

I don't have the will to do it, and I honestly don't remember such a big chunk of two-decades-old code well enough to be of much help. But I will gladly Thoughts and Prayers™ some brave person's way to a better solution.

[Goober5000]

Well, I don't have the appetite to rewrite it either, nor the time. If we can figure out how to deal with the all-time stats bug, as well as the newly known ESC-in-barracks bug, are there any other known bugs?

EDIT: Also, is #1451 usable, in whole or in part? Possibly after modifying it to retain p->stats.init()?

[notimaginative]

Well, I don't have the appetite to rewrite it either, nor the time. If we can figure out how to deal with the all-time stats bug, as well as the newly known ESC-in-barracks bug, are there any other known bugs?

My plan is to go back to loading the campaign savefile and using my original attempt at a fix to load the proper stats separately. That should get the stats working without loosing the different squad name/logo.

The bigger part of the change is to keep all pilot loading local to the barracks code rather than let it modify the global Player struct. That should take care of the ESC problem since nothing outside of the barracks should change unless the user intentionally does so (change pilot/squad image, add/delete pilot, set new pilot as active).

EDIT: Also, is #1451 usable, in whole or in part? Possibly after modifying it to retain p->stats.init()?

Yes, I think so. It was only the p->stats.init() change which gave me pause since there was some obvious (and understandable) confusion about it's purpose and the content of p->stats. But I liked the rest of the changes.

[Goober5000]

Ok, sounds good. I've edited #1451.

[Goober5000]

Closing in favor of #6419.