Issue #101 unit test and fosnola's fix

[NQNStudios]

This unit test for issue #101 compiles but I can't run it locally because of, of all things, rpath problems with the unit tests built through mac scons. So hopefully the github actions show that this test fails as expected.

[CelticMinstrel]

Well, it failed, but not in the way you want… I think?

[NQNStudios]

That's right. The test scenarios need to get copied over to the output by Xcode, so that one failed. And the scons builds need to actually run the tests. and copy over the test scenarios. Which I think should happen for every debug build but not release builds, so I'm gonna change ci.yml to call scons with debug=true as well.

[NQNStudios]

Actually these scenarios should just go in the test files folder. Much easier that way. And then the tests won't fail for scons release builds.

[CelticMinstrel]

It says the macos-scons build passed even though it has the same segfault as the macos-xcode build. That seems bad.

As for win-scons and linux, it looks like they didn't even run the tests. Is that intentional?

[NQNStudios]

I think when I changed the scons build to have all the partial flags, I made test default to false. So, I tried passing test=true to get windows and linux to run the tests before the last force push, and if they're still not running them, that's upsetting.

I don't understand why macos-xcode is giving a segfault -- other unit tests access the files folder relatively just like I did....

[NQNStudios]

Now that I think of it, I've only reproduced the bug in a Mac build. So it's possible that Windows and Linux are running the tests, and passing the tests, because the conversion already worked properly on those targets.

I'm gonna go check.

[CelticMinstrel]

I don't understand why macos-xcode is giving a segfault -- other unit tests access the files folder relatively just like I did....

I saw the segfault on one of the Windows builds too.

[NQNStudios]

I got the segfaults to show as test failures, at least.

[CelticMinstrel]

There was no output from the unit tests on linux? That's a bit weird… but it did clearly terminate scons, at least…

[NQNStudios]

Frankly I don't know why the platforms that are showing the test output, are. Since it's a subprocess. (Is piping subprocess output the Python default? Maybe it is on some platforms, but not others?)

I desperately wish I could spend more time on real problems than on build/test problems. 🤦‍♀️

[CelticMinstrel]

I suspect this check is intended to solve the issue that you added pop_paths for, but I'm not sure… do you know what "data" is? Is that related to the install path of a scons build?

[NQNStudios]

build/Blades of Exile/data is the output folder for all platforms where the assets are stored. So I think that check is determining if the path on top of the stack is a final output path, and if it is, leaving it. That said, I haven't looked at the Resource Manager code at all, so I don't know why it has a push/pop abstraction in it to begin with. So I'm just guessing.

[CelticMinstrel]

The intent is to not pop the core path to the assets distributed with the game. When loading a scenario, if it has graphics or sounds inside it, those paths are pushed. When the scenario is unloaded, the paths specific to that scenario must be popped, but they might not have been pushed in the first place, so what this does is pop the top path, check if it's the core game asset path, and push it back on if it is.

[CelticMinstrel]

…does that make sense? It sounds rather convoluted…

[NQNStudios]

Ok, that does make sense. I forgot that there would be contextual paths for scenarios--so the pop is to remove the context of whatever scenario the player was in before.

So is it okay that I added the non-pop option? Seems cleaner than the other alternative I considered, putting the files in a data folder even though they're not being packaged for output. Or doing something more convoluted to detect whether the top-level path is for the tests.

[NQNStudios]

I guess it could just only pop if there are more than one, maybe? idk. I think pop_path is a fine solution.

[CelticMinstrel]

Hmm… I guess another option would be to put the test path underneath the main data path, though that seems potentially a bit stupid (it might mean the main data path is on the stack twice?).

I'm not sure what's best here. I guess the variable isn't terrible, but I'd definitely prefer if it wasn't necessary.

Perhaps the best way would be for the scenario to record whether or not it pushed paths…

[CelticMinstrel]

Let's at least add a TODO comment or open an issue about this, because I really don't like the use of an extra parameter here. We should find something better.

[NQNStudios]

What about this -- push_path() takes a bool argument, temporary or somesuch, and there's a stack of bools corresponding to the stack of paths, marking which ones are temporary and should pop.

[CelticMinstrel]

Hmm… I'm not too fond of that idea either, but… it might be slightly less bad than the extra parameter in the scenario loading functions.

I already mentioned what I think the best approach probably is.

[NQNStudios]

Loading scenario graphics... ("files/town/townrectMac.exs")
Error
Error loading Blades of Exile Scenario: Could not read town record.
No such file or directory

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
boe_test is a Catch v2.13.10 host application.
Run with -? for options

-------------------------------------------------------------------------------
Converting legacy town data
  Boundaries conversion
-------------------------------------------------------------------------------
/Users/runner/work/cboe/cboe/test/town_legacy.cpp:252
...............................................................................

/Users/runner/work/cboe/cboe/test/town_legacy.cpp:265: FAILED:
  CHECK( scen.towns[0]->in_town_rect.width() == 5 )
with expansion:
  0 == 5

/Users/runner/work/cboe/cboe/test/town_legacy.cpp:266: FAILED:
  CHECK( scen.towns[0]->in_town_rect.height() == 3 )
with expansion:
  0 == 3

Loading scenario graphics... ("files/town/townrectWindows.exs")
Error
Error loading Blades of Exile Scenario: Could not read town record.
No such file or directory
/Users/runner/work/cboe/cboe/test/town_legacy.cpp:265: FAILED:
  CHECK( scen.towns[0]->in_town_rect.width() == 5 )
with expansion:
  0 == 5

/Users/runner/work/cboe/cboe/test/town_legacy.cpp:266: FAILED:
  CHECK( scen.towns[0]->in_town_rect.height() == 3 )
with expansion:
  0 == 3

So, the version of the check that uses the newer scenario format is passing, which we expected it to because it's not converting rectangles at all. But the legacy Windows and Mac scenarios, we want to fail because 3 != 5 and 5 != 3, not because 0 != 5 and 0 != 3

This must be because the "town record" failed to read, which is something I have to figure out what it means, unless you have an idea @CelticMinstrel

[NQNStudios]

Oh the universal one isn't passing the test, I have it commented out....

[NQNStudios]

Ok, interesting--the width and height are reported as 6 and 4 despite the rectangle only containing 5x3 squares the party can inhabit. But that's a weird quirk of rectangle that we probably don't want to mess with.

That's from getting the universal case to work. I still don't know how to fix the legacy ones.

[CelticMinstrel]

Ok, interesting--the width and height are reported as 6 and 4 despite the rectangle only containing 5x3 squares the party can inhabit. But that's a weird quirk of rectangle that we probably don't want to mess with.

It's kinda like the way ranges work in the STL – the rectangle covers all spaces in x range [left, right) and all spaces in y range [top, bottom). This is probably like that because Quickdraw / WinAPI already did things that way, but I don't remember. (It probably makes more sense when you realize that the structure was originally intended to address pixels, not tiles.)

[NQNStudios]

I started looking again at the 2nd commit that fosnola said is part of this fix. I know that this first commit fixes the bug when going into a scenario with a brand-new save file, but going over the 2nd commit it does appear to me now to be related, like we might want to cherry-pick it, but I don't know enough about file loading to know if it's correct:

fosnola@d63b089

[CelticMinstrel]

I'm equally not sure. I suspect changing the mac_is_intel use to true/false is wrong, but at least part of that diff looks like it might be reasonable.

There are ultimately two concerns when loading a legacy file:

• The endianness
• The rectangle ordering

A legacy Mac scenario (or saved game) is big-endian with TLBR rectangles, and a legacy Windows scenario is little-endian with LTRB rectangles. The current version of the game only uses TLBR rectangles. The host could either be little-endian or big-endian. (In practice, it will usually be little-endian, but there is absolutely no guarantee of that, and the game needs to run correctly on a big-endian machine.)

So I think that gives four possible cases that all need to work correctly when loading a legacy scenario (or saved game), as shown by this table:

Host	Scenario	Endian-swap?	Rectangle reorder?
Little-endian	Mac	✓	✗
Little-endian	Windows	✗	✓
Big-endian	Mac	✗	✗
Big-endian	Windows	✓	✓

I have a suspicion that the commit in question makes the little-endian cases work correctly but might break the big-endian cases. It's pretty hard to actually test the big-endian cases (unless you have access to an old PPC computer that you can install Linux on, or maybe something like a jailbroken Wii or Xbox360 that can run arbitrary code), but we should at least try to make sure it will theoretically work.

[NQNStudios]

If I'm being honest I haven't fully reasoned through understanding the first commit here--all I know for sure is it makes the unit test pass and the boundaries work as expected when testing manually ingame on Mac.

This legacy conversion and endianness stuff is really pushing the boundaries of what I understand, can properly test, etc.

I wonder if there's a Github Actions runner that is big-endian?

I also wonder, how irresponsible would it be to leave the legacy conversion case of this bug un-fixed, just to see if no one ever comes forward with a legacy save that happens to be in a rectangular town and is affected by this bug. Perhaps so long after the game's heyday, no one still possesses such a legacy save who would be in the audience of a BoE 2.0 release. This is lazy thinking, but we do need to triage a lot more aggressively in general.

[CelticMinstrel]

I wonder if there's a Github Actions runner that is big-endian?

I think it's fairly unlikely. I'm not sure how widely-used the PPC or M68k series are these days. Some processors (like ARM, which is admittedly quite commonly used) are bi-endian and can thus be run either in little-endian or big-endian mode, but I don't know how common it is to run them in big-endian mode. More to the point, most consumer CPUs are little-endian nowadays, so the GitHub runners would probably be skewed towards that because it's what most people need to test.

Though I think I remember something about being able to host your own runners, but that's probably more than we can manage.

I also wonder, how irresponsible would it be to leave the legacy conversion case of this bug un-fixed, just to see if no one ever comes forward with a legacy save that happens to be in a rectangular town and is affected by this bug. Perhaps so long after the game's heyday, no one still possesses such a legacy save who would be in the audience of a BoE 2.0 release. This is lazy thinking, but we do need to triage a lot more aggressively in general.

It's definitely higher priority to ensure that legacy scenarios work, rather than legacy saves. I think it's probably worth merging this even without reasoning through whether the other commit helps or what happens in the big-endian cases. Anyway, looking at the code for the current commit, it does look like it potentially handles all 4 cases correctly, but I haven't analyzed it in detail to make sure. We can leave the other commit for saved games until another time when one of us (or someone else) has time to analyze the code more thoroughly.