Found a bug in Pathfinding::isBlockedDirection

[Xilmi]

When trying to implement a cover-evaluation-check for my AI I stumbled across an inconsistency in Pathfinding::isBlockedDirection.
For the even directions, north, east, south and west there was only a check on whether the north- and west-wall are blocked. But not for the "Bigwall".

It seemed to not have terribly much impact on anything but my AI's method.

I played several missions of regular UFO with it and then switched to TFTD.

And alas: I realized: I can't climb on the Triton anymore!

Not being able to climb on the Triton was OG behaviour! Adding these extra checks in isBlockedDirection results in the same behaviour.

The lines I added to achieve this are the ones about the O_BIGWALLs in the following code:

Code: [Select]

case 0: // north
if (isBlocked(unit, startTile, O_NORTHWALL, bam, missileTarget)) return true;
if (isBlocked(unit, _save->getTile(currentPosition + oneTileNorth), O_BIGWALL, bam, missileTarget, BIGWALLSOUTH)) return true;
break;
@ -882,6 +883,7 @@ bool Pathfinding::isBlockedDirection(const BattleUnit *unit, Tile *startTile, co
break;
case 2: // east
if (isBlocked(unit, _save->getTile(currentPosition + oneTileEast), O_WESTWALL, bam, missileTarget)) return true;
if (isBlocked(unit, _save->getTile(currentPosition + oneTileEast), O_BIGWALL, bam, missileTarget, BIGWALLWEST)) return true;
break;
@ -893,6 +895,7 @@ bool Pathfinding::isBlockedDirection(const BattleUnit *unit, Tile *startTile, co
break;
case 4: // south
if (isBlocked(unit, _save->getTile(currentPosition + oneTileSouth), O_NORTHWALL, bam, missileTarget)) return true;
if (isBlocked(unit, _save->getTile(currentPosition + oneTileSouth), O_BIGWALL, bam, missileTarget, BIGWALLNORTH)) return true;
break;
@ -904,6 +907,7 @@ bool Pathfinding::isBlockedDirection(const BattleUnit *unit, Tile *startTile, co
break;
case 6: // west
if (isBlocked(unit, startTile, O_WESTWALL, bam, missileTarget)) return true;
if (isBlocked(unit, _save->getTile(currentPosition + oneTileWest), O_BIGWALL, bam, missileTarget, BIGWALLEAST)) return true;

[Meridian]

Ability to climb the Triton is done via ruleset (see /xcom2/mcdPatches.rul).
You can easily revert to OG behavior if you wish, by not applying this ruleset (the relevant parts of it).

With your change, the intentional OpenXcom behavior would not be possible anymore... and more than likely would have many other side-effects too.

[Xilmi]

Ability to climb the Triton is done via ruleset (see /xcom2/mcdPatches.rul).
You can easily revert to OG behavior if you wish, by not applying this ruleset (the relevant parts of it).

With your change, the intentional OpenXcom behavior would not be possible anymore... and more than likely would have many other side-effects too.

I've dug much deeper into all of that since making this post. Mostly by talking directly to SupSuper. After understanding how it's actually supposed to work, I could modify mcdPatches.rul to make the Triton climbable again even with my fix. My fix is not breaking things, it's correcting things! The intention of making the Triton fully climbable was not properly realized in the mcdPatches.rul but it worked in orthogonal direction because isBlocked didn't check for BigWalls for these direction!
The issue was that the Tiles with the IDs 3 (right wing) and 27-30 (left wing) were still set to their default, which is BIGWALL = 1 and not overwritten in mcdPatches.rul. This means they shouldn't be walkable at all according to the MCD-map-data.
I changed them from BIGWALL 1 to BIGWALL 6 and 4 respectively depending on their side. The code I pasted above also had the last parameter wrong, due to me not fully understanding what it's used for at the time.

Now they are fully walkable even with my fix in the code from the outside of the Triton but not from the inside. As I assume it's supposed to work. Fully walkable means including diagonal directions, which didn't work with what unmodified OXC and OXCE provides. There they are walkable but only in a orthogonal directions since diagonally the check for blocked still determined there is a blockage because the mcdPatch.rul didn't modify them.

The reason why blocked tiles in caves still weren't walkable despite that flaw was because their elevation-change was big enough to prevent that anyways.

A potential side-effect I see is to fix some weird bugs I've encountered earlier. Like a stunned alien carried in a backpack spawning into a wall-tile when waking up or being able to walk through certain walls on the 40k/rosigma rebel-hideout-map.

[Meridian]

My fix is not breaking things, it's correcting things!

No offense, but correcting things also means breaking things in this case.

The modders have been creating (thousands of) maps under the current conditions for years and I will definitely not change anything there without a very good reason.
Regardless whether current conditions are "correct" or "wrong".

If SupSuper will accept any such change into OXC, I will probably (hesitantly) take it into OXCE too.
A OXCE-only change in this part of the code is completely out of question.

[Xilmi]

SupSuper seems to have a similar stance on this as you have.

Kinda like: "If you want to meddle with this stuff, here's what I know about it... Have fun. But if nobody complained about an inconsistency in the current behavior, the risk of changing anything about it is greater than the potential benefit."

I mean I also had just accepted that walking the Triton-wing diagonally didn't work and didn't really care about it or what the reason for that was... Until I stumbled across the reason in the source-code.

So my fork is likely to remain the only one that checks for BIGWALLs in orthogonal direction too when it comes to determining whether a path is blocked.

I highly doubt that any map-makers placed Bigwalls on their maps for areas that they didn't want to be blocked. I think it's much more likely that someone wanted to block off an area with a Bigwall and was confused that it didn't work. I guess most of them didn't bother to use them at all as there's enough other ways to block movement in any particular direction.

[Meridian]

OK, no problem.
If you want to take responsibility for it, I wish you good luck too.

This mod is forked from OXCE and provides a few new options both for modders as well as for players themselves.
All the while being compatible with every mod that is also compatible with OXCE.

However, can you please update this statement on the mod portal, since from now on, you'll be officially incompatible with OXC/OXCE.

The default-Mod-validation-level has been turned down from 2 to 0. I think it's more reasonable to expect modders turn it up than it is people who don't know about it but who want to try a specific mod, to figure out they have to turn it down. I've even heard of people using older versions of OXCE because they couldn't get the mod to run on a newer one.

https://github.com/Xilmi/OpenXcom/commit/8dba5640942eafca5ffdb777545cc2125395e035

Also, I am extremely disappointed by this change.
You're single-handedly negating and sabotaging years of effort in OXCE to prevent game crashes.
You're actively supporting bad practices and making all our lives more difficult.
I am sad and frustrated.
Don't really know what to say... I'm probably just gonna take a long break again, and contemplate on life choices.

[Xilmi]

The way I'd update this statement would be rewording it to say "supposed to be" rather than "is".

"Is" was always a bit of a questionable and dishonest claim because over time I've been presented with several issues when it comes to mod-compatibility. But when it comes to learning about these issues, it surely helped that people tried and have them not work so I could then reproduce and fix the issues.

The latest one was that "Allow AI to pre-prime grenades"-setting was not very compatible with "primed grenades explode at the end of the turn". Unless you think enemies killing themselves this way is fine, of course.

However, I can only fix what people report to me. And in this case it was a coincidence, that I read someone telling that someone else the chat in a stream-VOD.

Claiming something that is wrong does, in a way help learning the truth because it triggers people to correct you. But yeah, if I am more honest in that regard and add that people should report any issues they find with the mods they try to me, it might also help me to learn about them.


The reason for changing the default of the ModValidationLevel-setting basically turned out to be a false alarm. Marbozir claimed on his stream that he couldn't get FMP to work on OXCE but it would run fine on OXC. When later, I helped him trouble-shoot, it turned out that it was a load-order-issue. He basically had put a sub-mod infront of it in the list and that's the reason why it didn't work, nothing to do with ModValidationLevel.

However, the other issue was X-Piratez. They explicitely distribute an old version of OXCE with it because the newer ones "aren't compatible anymore". Turning down the ModValidation-Setting "made it compatible again" for me.

I recognize that both approaches are horrible compared to the much more sustainable solution of fixing whatever the error is in the mod itself.

Anyways, I'll return the default for that to 2 with the next version of my client. Afterall it's actually a similar situation to what the topic of the thread is. A missing check of BIGWALLS in the client masks an issue with the MCD-Patch of the Triton and fixing the Triton-MCD seems a more sustainable fix than relying on the the missing check.

[Yankes]

There is very rare cases of false alarms with `ModValidationLevel`, if there were Meridian would remove it (as he already did it once).
Every test that was added was for fixing bug that was encounter in real mod, this why you can't load old xPiratez because it have crippling gameplay bugs/crashes.
It could be rare but its will happen.

Another case of validation could be for obsolete rules.
And even if now some thing will work now, it will soon not work as obsolete code will be remove in future version.

[Meridian]

However, the other issue was X-Piratez. They explicitely distribute an old version of OXCE with it because the newer ones "aren't compatible anymore". Turning down the ModValidation-Setting "made it compatible again" for me.

They distribute a "selected" (not old) version of OXCE, but for a different reason.
Piratez is VERY good at fixing any ruleset issues... this whole system was invented together with Dioxine (and others).
And for the last 2 years, I personally check all (newest at the time) Piratez releases against the newest OXCE version... I didn't have any issues yet, not even once.

If you have experienced an issue, I of course believe you.
But please report it, so that it can be fixed... or otherwise explained.

[Xilmi]

It might have been a specific version from a specific date with an issue that got fixed in the meantime. As I said, I'll change it back to 2.

[Xilmi]

There actually was an issue brought up with one of the staircases used in the "Hellsword"-craft WH40k.
Upon testing I found that it was indeed caused by the upper part of it being specified as a type-1 big wall.

So there actually is now a precedent for the very thing I was doubting.

Because of that I made the orthogonal bigwall-leakyness optional to provide a work-around without having to rely on anyone having to tweak their MCD-file while also providing the option of having a more strict checking for them in order to prevent other issues this might cause for maps that aren't having this issue.

Ideally I shall rework my cover-detection-algorithm anyways. Right now a isBlockedDirection counts as cover of 1 and anything that blocks walking on the tile for other reasons as a cover of 0.5. Some knee-high trash-can or flower being considered as valuable as a backside of a stair-case obviously isn't ideal. So I'm thinking about taking the value from that voxel-mesh that I could see in the MCD-viewer.