yairm210 / Unciv

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

This is a complex bug that causes the game to crash when destroying cities in multiplayer mode #9907

Closed AutumnPizazz closed 3 months ago

AutumnPizazz commented 1 year ago

Platform Both Android and Windows11

Version 4.7.13

Describe the bug As the title. There's some questioned saves. ( I change it as txt and it could be none '.xxx'' file. ) These two save files require my mod "The Coexistence in harmony".

Autosave-Nation_Qin-24.txt Autosave-Nation_Greece-14.txt

Based on the tests conducted, this issue rarely occurs (or I haven't observed it) when not using any mods. However, it tends to occur more frequently after applying certain mods, although occasionally the problem does not manifest.

Screenshots image image

SomeTroglodyte commented 1 year ago

The "Qin" one loads just fine here, while the "Greece" one is indeed broken - when loaded with an editor that's able to control encoding, and loaded as UTF-8, it says ,"baseRuleset":"the Coexistence in harmony - xxxx","mods":["the Coexistence in harmony - music"], - those 'xxxx' characters are hex B2 E2 CA D4 B0 E6 , not valid UTF-8 and when (incorrectly) treated as such they break the JSON rules by hiding the closing quote/comma. Forcibly loaded as Chinese-Big5 charset, the xxxx become "聆彸唳", likely as intended. Problem is - the proper UTF-8 encoding of that would be hex E8 81 86 E5 BD B8 E5 94 B3.

We've been there before - you system, together with your Java, writes a local encoding when it should be writing UTF-8. It happens way under the hood out of our control. And you're getting problematic text into places we don't usually need to worry about because they're read from the file system - remember the chinese-named Images where the atlas loader can't cope with these names?

Now that does not necessarily correlate with your uninitilized lateinit ruleset observation, but that's not impossible either. And since we can't reproduce...

I'm sorry, somehow it's the industry's fault for not tackling consistent international encoding much earlier, so we still get these cases decades after the standard that should solve them was finalized... Not yours, not ours.

You can, of course, and I know you already know, help avoid these - don't use chinese file names anywhere where that name is somehow maybe passed on as string, such as used mods in a game.... And/or set your system to use UTF-8 as system encoding (no idea whether Windows even allows that, but with *nix distros it's pretty much standard).

AutumnPizazz commented 1 year ago

Your response is very profound, to the extent that I may not fully understand your meaning (due to my limited English proficiency). However, I can provide some information to the best of my ability.

AutumnPizazz commented 1 year ago

The names "the Coexistence in harmony - xxx" and "... - music" that you mentioned indeed appeared in my previous mod. The actual meaning of "xxx" or "聆彸唳" is "beta version." However, these two mods are only locally isolated and saved in a non-testing version of Unciv, which means they are not used for testing purposes.

AutumnPizazz commented 1 year ago

As an addition, when I test the beta version locally and upload it to GitHub, I remove the "beta version" label. As for the "music" component, it has indeed been uploaded to GitHub, but it is merely a music expansion pack that I believe is unrelated to the game.

AutumnPizazz commented 1 year ago

Regarding the issue of non-UTF-8 encoding, what I need to inform you is that I have been using VScode for editing files like JSON and Properties. In the editing interface, I can see that the encoding format is indeed UTF-8. image

SomeTroglodyte commented 1 year ago

On a tangent: We can have non-western - no, wrong wording: We can have characters that need different representations in the commonly used system encodings: Renamed cities and units. Those can break if the libraries assume the wrong encoding, even without any mods or translation trouble.

Maybe a good reason to re-check: Can we force the underlying libraries to UTF-8? The atlas loader was impossible to force without forking Gdx, but game saving/loading - maybe we should avoid all Json functions taking a FileHandle and manage the binary buffers + encoding mapping ourselves?

when I test the beta version locally and upload it to GitHub, I remove the "beta version" label

Perfectly fine.

But understand what I said up there: Mod names - in case of locally created ones that's folder names verbatim unless it ends in a space - get to be part of the game data, and as such get serialized. That means problems become unavoidable when the bit representation assumed by one system is different from another system and you save with one and load with another. Won't hit you if you only use bit patterns that have the same meaning in all those encodings (like 0x61 is the same latin lowercase "a" in ASCII, in ISO-8859-15, in Big5 or in UTF-8). Won't hit you if all systems agree to UTF-8 and play nice. And that's difficult because it's the libraries that should do so, promise to do so, except when.... And there you get into the Java/OS/Version/Local settings chaos.

At the moment the best idea I have - best not as in "best for the world" but "best for a reliable result" - is detecting system encoding/java encoding disparity and refusing to run 😢

I can see that the encoding format is indeed UTF-8.

...If you trust VS code - yes, reasonable. But file encoding of mod jsons is a different matter vs. serialization encoding (and yet separate from filename encoding). Try to open your saves linked above in your VS - either some content will be wrong (look at the notifications near start - cityname starving - or the baseRuleset property near the end in that Greece one) or it will show they're not UTF-8.

AutumnPizazz commented 1 year ago

Try to open your saves linked above in your VS - either some content will be wrong.

Indeed, there is this issue. image

SomeTroglodyte commented 1 year ago

Oh my, I just duckduck'ed a bit - Big5 isn't even a proper standard, much less Windows codepage 950.. And issues such as this one do not sound like some major company is learning the unicode lesson properly, even if they were once at the very front (Remember Windows NT 3.1? Entirely and properly unicode under the hood)...

Indeed, there is

Exactly. Tell VS to treat the file as Big5, and that becomes readable.

In case unclear: Unciv in-memory is encoding-neutral. Java libraries should use UTF-8 for all file text translation unless ordered otherwise, but some Java implementations wrongly use the "System" encoding as queried somehow from the OS instead. Fix Java - or make sure that queried encoding says UTF-8 too... Which under Windu might be labeled "Beta: Use UTF-8 encoding for non-unicode programs" - Unciv is NOT a non-unicode program, but your Java might just try to be...

Now I'm very interested whether in your case toggling that option would help - and remember when it happens: Those saves are "beyond saving", for them it's in the past, so to test you need to go forward, saving from a known-valid state... But even a bad save, generating a new notification with chinese characters properly displaying, and saving that could prove a point if that notification arrives in the save file as proper UTF-8............... And don't forget under W everything needs a reboot, 'specially that option.

AutumnPizazz commented 1 year ago

On a tangent: I'm just a student who has recently finished the Chinese National College Entrance Examination (Gaokao), which means I don't possess the basic skills of a programmer. I only have a basic understanding of the C programming language, and beyond that, I have no knowledge of programming. I can only look at the numerous technical terms you mentioned in the previous text without understanding them. I can hardly understand the task you want me to accomplish. QAQ

SomeTroglodyte commented 1 year ago

The "tangent" above was meant internally, as in - are there defensive-coding options to avoid that Java misbehaviour and would it be worth while.

And "that option" was what the linked superuser.com question documents - well hidden in control panel, and as that is so varied between Windows versions I can't help any better that that link. Had to search and interpret quite some to find it on this box. But it's indeed marked beta and demands a reboot right away...

don't possess the basic skills of a programmer

Don't let a professor tell you otherwise: the most basic skill of a programmer is curiosity.

SomeTroglodyte commented 1 year ago

@AutumnPizazz - 4.7.16 comes with the PR mentioned, which likely means it will affect your older saves. Also, if you manage to provoke a crash, you'll get a line on the crash screen explicitly stating what encoding Java sees as "system default" - sorry I can think of no direct way to get Unciv to display that at the moment. Should be "Big5" in your case, and would react if you change your Windows regional settings "beta UTF-8 for legacy programs". But - that PR also aims at having the libraries ignore that setting where possible and always behave like it were on...

You're welcome to comment on how that affects you.

Note - the one place that can't reach is the atlas loader - so from image file name to atlas (text file) to Unciv would still have the potential for encoding issues (I believe upstream Gdx main branch has fixed that, at least they're aware of the issue).

AutumnPizazz commented 1 year ago

Thank you for your concern. My locally isolated Unciv version, specifically used for testing, crashed. However, the standalone Unciv program without "-beta version" can still run smoothly.

image image image

AutumnPizazz commented 1 year ago

Looking back at this conversation, it seems like we have deviated from our original topic. The initial topic was about me and my friend playing together using my mod, but when one of us deliberately destroyed the city, my save file crashed in the next turn. To address this issue, I presented two save files (the first one being the multiplayer save file, and the second one being a separate version with the suffix "- beta version" which I tested and also crashed). It seems that the efforts we have made during this period have not resolved the issue I originally raised. QAQ

AutumnPizazz commented 1 year ago

My locally isolated Unciv version, specifically used for testing, crashed

If I simply change the Chinese "-beta version" to English, the crashing error will be resolved.

SomeTroglodyte commented 1 year ago

Yes and no.... The changes in #9927 should improve your chances - but only if you start out with it. Because now, when dealing with files, the API is instructed to use UTF-8 encoding practically everywhere. Before that, some API's would internally default to UTF-8 others to "System" - potential for errors.

Now what I'd like to know is: Scenario: you have your mod with chinese characters in the name, on disk. Your Windows Explorer shows the name correctly. Hopefully, Unciv shows it correctly too. (At this point we know encoding from file name to internal String works, as our display routines are unicode and do it properly - as far as we know). Start a game with the mod and save. Open in your VSCode and verify - at the end of the json - that chinese name part was properly written as UTF-8 into the serialized mod list. I'm sure it is, it was not before.

That's the only way to achieve interoperability between systems behaving differently with encodings - control them. That's the required basis for you to be able to play with a friend running Linux, or with a friend running Windows with that certain option turned on.

Recovering older saves that use non-UTF-8 encoding is very hard to almost impossible automatically (reliable detection of Big5 vs. Unicode vs Shift-Jis vs whatever - I know of no libraries for that). And semiauto, letting the user control the conversion - sorry border case not worth it. You could do it if really important in Notepad++ and likely VSCode too.......

So, "deviated from our original topic" - "playing together" -> I focused on the prerequisite that all "playing together" systems must interpret exchanged files the same way, and that was not the case. Now it is - hopefully - if I caught all places.

SomeTroglodyte commented 1 year ago

Note: There's exactly one place in Unciv where I didn't enforce UTF-8, and that's intentional: The system info on the crash screen, on Windows only, pulls OS info from the registry by executing a mini shell script. The output stream of that is still interpreted in system default, as I deemed it likely the source will be "speaking" the same encoding. Just to emphasize that PR was not trivially easy to do, I tried to be as thorough as possible and scan all code that does Java File, Gdx FileHandle, Gdx Json or anything 'Stream'....

AutumnPizazz commented 1 year ago

It seems that our efforts in this regard have not alleviated this breakdown problem. I got up from bed in the middle of the night to turn on the light and test it, repeating that action twice. One time, it malfunctioned and the other time, it did not. Unfortunately, I am unable to upload the video I recorded. Please wait a moment, I will find a way to send it out in a while. @SomeTroglodyte

SomeTroglodyte commented 1 year ago

Hey, global channels ignore time zones, that's normal??? Sorry for waking you, but - me, I got "Do Not Disturb" on automatically from 22 local to 07 local, and K9 respects it...

AutumnPizazz commented 1 year ago

No crashing.txt Crashing.txt Okay, I'll use the same old trick and convert the MP4 file to txt. After you receive it, you should be able to browse it correctly by changing the file extension. @SomeTroglodyte

AutumnPizazz commented 10 months ago

I believe it is necessary to bring up this agenda again now because this bug has not been resolved in the new version.

yairm210 commented 7 months ago

Can't download the mods for this, is this still a problem?

AutumnPizazz commented 6 months ago

Yes, the bug never goes away.

SomeTroglodyte commented 6 months ago

I declare this issue too unclear and too difficult to find and categorize information. I ask for a start-over. I don't even know anymore what exactly you mean with "this bug". (Does the issue title still fit? Are any saves still relevant? Are the mods changed to the point of making the original observation irrelevant? Confused. I remember I could never reproduce...)

We need to separate:

As for encoding, I guarantee you wouldn't have problems if:

That's a little more than necessary, but to differentiate when and how you can get away with loosening these rules is more complicated than simply following them. I know you know, and I know you know how to limit display text to the translation files. But it's easy to slip and let one exception through...

If you can demonstrate an encoding-related bug while following the above rules (for all devices involved in a game), superb, we'd love to get to the bottom of that, so report as new issue please - but I doubt it is still possible.

If you wish to demonstrate an encoding-related bug where some of these rules couldn't be followed, then please do a new issue as well, and point out where exactly you didn't, show us the OS/Java header of a crash (if in doubt, the Debug options has an "intentional crash" button), and please re-summarize what the important aspects are - while keeping it as small and simple as possible.

Non-encoding related bugs - same, please re-do as new issue(s), which could also serve to concentrate the info to the relevant bits.

AutumnPizazz commented 6 months ago

I'm sorry that I didn't make it clear. The core of this issue is that when using server online and mods, the act of occupying enemy cities has a probability (emphasis, sometimes it happens and sometimes it doesn't when I try to reproduce it) of causing the game to crash.

AutumnPizazz commented 6 months ago

However, I noticed that when there are more and more buildings in the occupied city, the possibility of causing a crash is higher, which may be related to the performance of the running device - but this does not happen when playing offline - I don't understand it.

SomeTroglodyte commented 6 months ago

The core of this issue is that when using server online and mods, the act of occupying enemy cities has a probability of causing the game to crash.

Thanks! Succinct and clear.

Also sad - random factor, online and mods all add up to "no way to debug". So please keep your sensors peeled and the moment you get that - copy and supply the entire crash screen text. System info, stack trace (at least up to the first line mentioning concurrency), exact version. Add the system/version info of all players, and a save (includes mod names) and some info on whether these mods were current or maybe not identical over the player base. Then we have a chance to narrow it down. Of course, a single-player no-mod demo would be best :zany_face:

github-actions[bot] commented 3 months ago

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 15 days.

AutumnPizazz commented 3 months ago

This bug has been solved in 4.11.12 by my test.