yairm210 / Unciv

Open-source Android/Desktop remake of Civ V
Mozilla Public License 2.0
8.34k stars 1.56k forks source link

Feature request: Tolerance for missing civs #9619

Closed hackedpassword closed 1 year ago

hackedpassword commented 1 year ago

Twice now I've lost a game to a mod update where some component was changed in the mod, both cases specific to a civ/CS being removed, which invalidated the autosave upon updating the mod. Unrecoverable. I don't blame the modder whatsoever, if anything, the logical or common sense thing to do is not update the mod til the next game. But I want to keep up with exciting mod updates!

I was tempted to make this a bug report so I had to weigh user error over game error, and I lost that debate for the above reasons. So, I'd like to see if it is possible to add a feature that would make this scenario tolerated, heal the save instead of dump it.

As it stands, even if this feature was made available, I can't even commit a hard save for this particular game because the autosave can't be loaded, so by starting a new game, 10 turns in, it's gone. For my purposes this is unfortunate as I was on the brink of having had a feature in my test map set up with everything/everyone in place, only to think it was a good idea to update the mod. That's on me. Lesson learned.

Screenshot here in GUR's discord thread. https://cdn.discordapp.com/attachments/1071804673956266096/1119856338219044966/Screenshot_20230618_000758.jpg

SomeTroglodyte commented 1 year ago

You can copy an existing save to clipboard as far as I can remember... Shouldn't that do something useful on mobiles too?

Best idea I have points to an open wound - we still should decide on how to tackle mod compatibility in general. We have the permanent audiovisual switch on mods that got no media, we have no way for a mod to say "I need Deciv" other than external comments, and so on.

If we had such a frame, some declaration option in there like "Replace [Rio De Janeiro] with [Rio de Janeiro]" could be offered. The modder would need to set it the moment they rename. The implementation would still need to loop over a lot of structures, and do so before setTransients and without tripping lazies, but - I could imagine a chance that could work. Removing a civ with all references might be much harder. Same for fixing the Json before parsing it - that you should better do in a text editor. Which is possible on a mobile without root, too, afaik - see above and combine with load from custom location?

hackedpassword commented 1 year ago

I feel there's a regex opportunity here to parse and cleanse defunct json, as an external tool. Regex is my specialty.

I should have remembered: [laptop_name][phone_name]\Internal storage\Android\data\com.unciv.app\files\SaveFiles

Had I did I'd have salvaged the savegame for dissection. Now I'm 70 turns into a new one lol. Damn. I don't know enough about git but I'm sure there's a way to fork a previous version of a repo. I could synthesize a savegame with the last tGUR. Hmm I'll talk to gpt about how to accomplish this.

I noticed in Deciv's description, dated 5/19, goes out of its way to inform the user not to do what I did. Maybe instead of a complex healing procedure, a compatibility check against the current Autosave when updating a mod? I'm still not against healing the savegame so no one is off the hook yet! ;) Since there's a mod checker, an Autosave game check seems like a reasonable thing to do.

hackedpassword commented 1 year ago

I successfully produced a v1.9 of tGUR (mod: tGUR-1.9-experiment), ran a short game, then stepped the mod up to current tGUR and re-d/l'd the update. Game loads normally. Checking with discord/A1y0sh4

yairm210 commented 1 year ago

For 95% of missing/broken objects we can deal with it by just saying "okay then remove this thing from everywhere" For missing civs, wholesale removing the civ would be impossible, but replacing is definitely feasible. In fact a simple replace on the jsonified gameInfo could do the trick, so we don't need to figure out all the places where a civ is referenced by name (because there are a lot of places)

hackedpassword commented 1 year ago

That's where I was hoping some regex magic would save the day. You guys have far more sophisticated ways to handle this from inside the code. I'm not giving up yet.

Just went through my saves, found one! Converted to structured json:

China - 105 getting involved, bad idea.json.txt

This save complains that Sydney is not present. Let's grep number of Sydney instances: 2850!!!!!! Huh. That could be a problem.

SomeTroglodyte commented 1 year ago

are a lot of places

And because there are places like PopupAlert.value - you'll always feel like you missed some.

The regex approach, however, could work. Just tell the victim user "no guarantee". Brute'ing the replacement - could it backfire? Like what about the instances where nation name and capital name are the same? Sounds like OK, as long as they were same nation. Use in Notification icons? Hmmm... OK..? As I said - upfront disclaimer.

Regex is my specialty

Oh? :partying_face: Then please go over the monsters in UiElementDocsWriter and tell me where they're bad.

hackedpassword commented 1 year ago

Oh? :partying_face: Then please go over the monsters in UiElementDocsWriter and tell me where they're bad.

Hit me up, somewhere, and help me understand the problem. @wphoenix on discord

SomeTroglodyte commented 1 year ago

I'm the no-Discord person around here... The login works by now after years of active privacy tool blocking by them, but now I missed the train and am too lazy to catch up.

hackedpassword commented 1 year ago

I'm the no-Discord person around here...

Well how am I supposed to get the 411 on said UiElementThingy? I'm in need of some rigorous regex exercise.

SomeTroglodyte commented 1 year ago

Well... "around here"? Edit: ... sorry for teasing a little, but - why not. You don't need to learn kotlin to get an idea from that piece of source - maybe. Context: That thing goes over Unciv's own sources and looks for calls to two specific functions that manage "skinnable UI", then tries to auto-write documentation. Uses kotlin's "raw strings" - these are delimited by triple double-quotes and are meant to allow anything in between verbatim including line breaks.

SomeTroglodyte commented 1 year ago

I'm not giving up yet.

So where do we go from here? Technically, I see two problems: One, the current exception handling and where exactly to catch the exception Two - what to replace the missing civ with? Finding something unused in the target ruleset might be a challenge, so I'm all for giving all their cities and units to the Barbarians instead. Ugh, no, that would pretty likely cause duplicate key errors in some diplomacy structures. Create a mock one -> would need a repeat each single time you load that game afterwards... Maybe declare a "Bad Kludge" city-state, exempt from being chosen at newgame and hidden from pedia, beforehand. But that would break for base ruleset mods that happen to not do the same. ...

hackedpassword commented 1 year ago

Actually I was thinking of this last night ... when I went through and deleted all my old saves that were rendered useless because Can't Load [asset].

In the code for religion, there several instances of fake religion TheLegend27 to handle certain cases. Leading to ...

I agree with the initial thought of a mock civ, but as I go through working out the details, I'm coming up with issues also. However, a new idea is this: Upon finding a civ or CS is unavailable, prompt the user for a replacement civ or CS. From what I understand all units and buildings, even unique, have some counterpart across the board. If none are ultimately available, default to the same asset that aligns with the current era. I imagine this isn't perfect but it keeps the system moving. Unit promotions would either be 1:1 or reset, too bad. Same with build states. Align or reset. What this approach comes down to is competition attitude vs causal/fun attitude - the competitor types will scoff and move on cause someone moved the ball on the table, while the casuals will let it go and play on.

Or: how does the code handle a defeated player? Could the offending civ just be declared defeated?

SomeTroglodyte commented 1 year ago

No. Think of it as SQL, referential integrity and a unique constraint.... And we lack cascade delete. That the reference is a key / pointer into a collection is the problem - since under normal circumstances that referential integrity is guaranteed - It will always find the Nation to my civName - all the code relies on that. Additionally, we demand 1:1 relations - though a certain value to a 1:n design is thinkable, Unciv was built for no two players can both play China. So we need an existing but free object in the Nation table. And Defeat has no correlation with the referential integrity requirement.

I actually think a dummy mod with a nation with a recognizable "placeholder" character could be cleanest: The new mod is added to the save, so will cover the future loads... It could even be an internal invisible mod without github repository. I can see that working and would guess less than 100 lines, but above average effort for testing. More if covering more object types, not only nations. Sounds like 60 to 40 in favour of "not worth it" up here.

hackedpassword commented 1 year ago

In conclusion, I believe this request to be infeasible with 4.x. However, this should be a feature on the unciv roadmap as more players will be modding, and user-gamers will eventually have wtf's on why their game is defunct cause of an upgrade.

SomeTroglodyte commented 1 year ago

I had the perfect inspiration for the perfect solution:

An external service independent of Unciv.

Someone writes a generator for compatibility mods, in python, rust, javascript running in your browser, whatever. It asks you for the missing civ name, and whether to make it a citystate or not. Result has a Nations.json with one entry, which you must then install, as a mod, by whatever means (if that service is hosted, it could produce a link that goes into the mod manager's load from url - if we patch that with the any-zip-url-allowed kludge from my stash).

Second service, could again be hosted or not, takes your json string, patches the mod list, and spits it out again in whatever form. Done.

hackedpassword commented 1 year ago

I had another idea, low success probability.

UI: "Could not load game! [nation] not found! Would you like to try a previous version of [mod]?" [y/n] y "Version: {2.0} (1.9} {1.8} ..." [select] 2.0 --> Pulls asset file for previous release making this a new mod so whatever is already in place to get mods working locally just snaps this version in (as ex. [mod]-2.0).

The mod itself could have a .json field specifying previous release versions so seek-and-find is minimized:

ex. ModOptions.json:

{
  "previousVersions": "2.0","1.9","1.8",...;
  "2.0": "https://github.com/A1y0sh4/The-Great-Unciv-Rework/archive/refs/tags/v2.0.zip";
  "1.9": "https://github.com/A1y0sh4/The-Great-Unciv-Rework/archive/refs/tags/v1.9.zip";
  "1.8": "https://github.com/A1y0sh4/The-Great-Unciv-Rework/archive/refs/tags/v1.8.zip";
  ...
}

That would beat trying to reparse a diff which was my other idea. The-Great-Unciv-Rework/compare/v1.3...v2.1 Not a good idea.

I think the coding effort would be minimized, responsibility to provide compatibility asset links are on the modder, accessibility is maximized. Worth a consideration.

hackedpassword commented 1 year ago

I think the best solution possible atm is using git to pull a previous version, make that version another mod, try to play from there.

Any other ideas or close this?

yairm210 commented 1 year ago

I think close it :/ There's no good immediate solution for this that I can think of