I've given it some more though, and I think rP26298 was a mistake, unfortunately. Here's my arguments:
#1 -> This leads to weird code, because:
- Maps don't care about non-JSON data
- GUI cares about JSON data and the 'Identity' part of the XML data, but only that part.
- Sim mostly doesn't care about JSON data.
Having a single function to handle all of those cases is impractical.
Furthermore, basically all callsites have been complicated by the diff instead of simplified, which shows IMO that something is awry.
#2 -> The civ .xml files dictate how a Civ plays, but the civ name is still moddable. So the 'history' component of it seems not actually tied to the civ, but to the player. This makes some sense from a modding/mapmaking perspective -> you want the same Civ gameplay, but different civ names & such. So moving the history data to the gameplay-XML seems illogical.
See point below, but I think we could give Player entities an Identity component without actually putting that data in the XML.
#3 -> Civ XML is only necessary for gameplay-changes. You can actually make a civ with just the JSON data and the fallback XML, though this will now warn. I think that's also a bad move -> the JSON is self-sufficient (and in fact, barely necessary itself). Forcing an XML makes modding harder.
#4 -> We have precedent for JSON-only data, such as resources.
Quick note -> I think the better move is to give the player-entity an Identity, but still load that from JSON instead of the civ XML. Then if you need identity-stuff, you query that, gut 'GetCiv' is still on CmpPlayer. I think doing that would be easier on top of this reversion diff though.
Incidentally, this diff preserves the Identity SetName changes.
How was this diff created:
- I reverted to rP26298
- I reverted that diff
- I reapplied all further commits, fixing merge conflicts as I went.
- I tweaked a few things for minimal diffing changes.