Brutal-OXCE 9.2.6

[Meridian]

Wait what? i thought the "2" was the amount of bytes and the actual contents (turns) was inside of the "binTiles:"-line.

The content (number of turns) inside the "binTiles:" is already correct, no change needed there.

We are talking about the change in those 3 variables with metadata, containing the amount of bytes (2), see attached.

[Xilmi]

I changed it now as you suggested.

I still have some weird effects.

I see 4 cases:

1. New saves created with 9.2.3:
Loading is fine. Gameplay is fine.

2. Saves from 9.2.2:
Loads very slow and AI/Autoplay-turns are slow for some reason.

3. Saves from <9.2.1 but >2.0.0 or whenever I introduced the turnLastExplored:
Loads quickly BUT AI/Autoplay-turns are slow for some reason.

4. Very old saves from when I just started and before turnLastExplored:
Loads very slow and AI/Autoplay-turns are slow for some reason.

I didn't have any of these effects before the merge. I could just load a save from whenever and it would seemingly run with no issues.

Edit: I think I understand why it slows down the AI:

int Tile::getLastExplored(UnitFaction faction)
{
   int lastExplored = 0;
   if (faction == FACTION_PLAYER)
      lastExplored = _lastExploredByPlayer;
   else if (faction == FACTION_NEUTRAL)
      lastExplored = _lastExploredByNeutral;
   else
      lastExplored = _lastExploredByHostile;
   if (lastExplored > _save->getTurn())
      lastExplored = 0;
   return lastExplored;
}

When it reads random stuff from the save as lastExplored it will return 0 as lastExplored because it is > than _save->getTurn() but not correct the members it read. So it will come to the wrong conclusion of having to recalculate potential enemy-positions every time.

[Meridian]

1. New saves created with 9.2.3:
Loading is fine. Gameplay is fine.

ok

2. Saves from 9.2.2:
Loads very slow and AI/Autoplay-turns are slow for some reason.

these are "broken" and will never work ok

3. Saves from <9.2.1 but >2.0.0 or whenever I introduced the turnLastExplored:
Loads quickly BUT AI/Autoplay-turns are slow for some reason.

I have a save from "Extended Brutal 7.12.1 8.3.1" from 10.03.2024 and that works for me without problems (as far as I can say), attached.

Can you attach the save that doesn't work for you?

4. Very old saves from when I just started and before turnLastExplored:
Loads very slow and AI/Autoplay-turns are slow for some reason.

Edit: not much I can do in this case #4

[Meridian]

Also, I don't have slow loading in any combination. Edit: I have slow loading when trying to load OXC/OXCE battlescape saves
It is either quick or doesn't work at all.
That is probably caused by different compilers... one more reason to save the variables consistently using the same data type to prevent unpredictable behavior.

Attached is the error message example I get in cases where you report it is loading slowly for you.

[Meridian]

I changed it now as you suggested.

did you push it to github yet?
I can't see the change to Uint8 in 9.2.3

[Xilmi]

No, I didn't push it yet. Still experimenting.

I think I had an issue with my compilation before. I changed something mid-compilation and it didn't compile it but then also thought it was already compiled. Because after some other changes the problem with the pre 9.2.2 saves was suddenly gone without any change warranting it.

So I guess situation is now:

9.2.3 will be compatible with everything except 9.2.2 and very old saves or saves made with regular OXCE.

Before the saves made with OXCE still did work though.

I attached one of these very old saves, that I always used for benchmarking.

[Meridian]

9.2.3 will be compatible with everything except 9.2.2 and very old saves or saves made with regular OXCE.

Before the saves made with OXCE still did work though.

Good point.
I will look later at this too.
I'm sure something can be done for backwards/cross-compatibility there too.

Or maybe redirect this to Delian, he wrote the whole thing and understands it much better than I.

[Meridian]

9.2.3 will be compatible with everything except 9.2.2 and very old saves or saves made with regular OXCE.

Before the saves made with OXCE still did work though.

Seems like you had this issue before already.
And this seems to be the "fix" for it (or one of the fixes): https://github.com/Xilmi/OpenXcom/commit/6aedfcb8cec2cc4e025e3100cf3595e2c4663bd0

The issue has probably returned, because this commit has been partially reverted during the 8.0.0 merge.
(Just guessing here, cannot prove it.)

This commit is also the one causing the slow load (caused by log file spam... logging is very slow).
Although I don't know why it complains that 2 is not a valid option, since 2 is a valid option.
Maybe because in one case it's a "number 2" and in another case it's a "string 2" ?
(again just speculating here, needs to be properly looked at...)

Code: [Select]

[2025-01-27_20-57-31] [WARN] unserializeInt has invalid sizeKey of 2 .. this can mean deserialization data is ill-formed
[2025-01-27_20-57-31] [WARN] unserializeInt has invalid sizeKey of 2 .. this can mean deserialization data is ill-formed
[2025-01-27_20-57-31] [WARN] unserializeInt has invalid sizeKey of 2 .. this can mean deserialization data is ill-formed

To properly merge/remerge this, you'll probably need to contact the original author who gave you the above-mentioned commit: ruderubik <58171205+ruderubik@users.noreply.github.com>

Edit: I asked Delian for help too (even though it is probably only indirectly caused by yaml lib change).

[Yankes]

Message is wrong:

Code: [Select]

Log(LOG_WARNING) << "unserializeInt has invalid sizeKey of " << sizeKey
and `sizeKey` is `char`, this mean it output character not int. '2' is 50. And this is not any of 3 available sizes of tile (1byte, 2byte and 4byte).
Engine try load variable that should have size of 50 that is not supported.
If we mix char value with character itself we can't handle it unless we add new cases like:

Code: [Select]

case 1: ... //current code
case 2: ...
case 4: ...

case '1': ... //hack for wrong version
case '2': ...
case '4': ...


[ps]

Please remove `__builtin_unreachable` from code, code that reach this function is UB and this is whole point of this function.
you can't use this function to "validate" user inputs as compiler can assume that he can ALWAYS load 32 bit here when you send any other value than
1 or 2.

[Delian]

can you check if we can somehow help with this issue?
TL'DR: OXC/OXCE battlescape saves don't load in BOXCE

I cloned Xilmi's repository, built it, ran it. Loaded why_so_unbearable.sav, saved it to why_so_unbearable2.sav. Loaded why_so_unbearable2.sav. No crash.

[Meridian]

I cloned Xilmi's repository, built it, ran it. Loaded why_so_unbearable.sav, saved it to why_so_unbearable2.sav. Loaded why_so_unbearable2.sav. No crash.

Try in Release mode.

(in Debug it worked for me too now, I guess that's what you did)

PS: I don't have a clue what that part of the code does (on the attached screenshot)... but based on Yankes' comment I'd guess "nothing good"

[Delian]

The warnings that I get in release build are:

Code: [Select]

[2025-01-28_00-31-10] [WARN] unserializeInt has invalid sizeKey of NUL .. this can mean deserialization data is ill-formed
[2025-01-28_00-31-10] [WARN] unserializeInt has invalid sizeKey of NUL .. this can mean deserialization data is ill-formed
[2025-01-28_00-31-10] [WARN] unserializeInt has invalid sizeKey of NUL .. this can mean deserialization data is ill-formed
NUL is \x00. So you seeing 2 is probably a result of some sort of UB. The actual value of that variable is 0.

Anyway, the problem has nothing to do with rapidyaml; the binary tile data gets deserialized just fine.

Code: [Select]

_lastExploredByHostile = unserializeInt(&buffer, serKey._lastExploredByHostile);
_lastExploredByNeutral = unserializeInt(&buffer, serKey._lastExploredByNeutral);
_lastExploredByPlayer = unserializeInt(&buffer, serKey._lastExploredByPlayer);

should be

Code: [Select]

if (serKey._lastExploredByHostile != 0)
_lastExploredByHostile = unserializeInt(&buffer, serKey._lastExploredByHostile);
if (serKey._lastExploredByNeutral != 0)
_lastExploredByNeutral = unserializeInt(&buffer, serKey._lastExploredByNeutral);
if (serKey._lastExploredByPlayer != 0)
_lastExploredByPlayer = unserializeInt(&buffer, serKey._lastExploredByPlayer);

Btw, the CrossPlatform::unreachable() that runs in debug mode should throw on windows. Yet it doesn't lol

[Yankes]

No, this code should never have `if (serKey._lastExploredByHostile != 0)`, at lest in near future.
Each of this values should have non-zero values, if you get zero some data get corrupted, and this is whole problem.

[Delian]

Those values are 0 on the first load of an old save file, because they don't exist in the old save file yet. The data is not corrupted and it's correct that those values are 0.

[Yankes]

aaaa, this is new property that is not in OXCE, this mean my previous comment is not accurate. This mean OXCE save should not have it.
But this save was generated by OXCE? Should this save be from BOXCE?