#5031: Fix accessibility logic for wagons #5053

dhthwy · 2024-11-29T14:20:28Z

The save for 5031 has down stairs blocking the wagon path. This PR attempts more robust wagon accessibility handling.

Other issues:

There is also an issue with the 'follow mouse' functionality where walkability is being correctly set for top ramps. This causes the UI to erroneously display an 'X' over the ramp tile, incorrectly indicating that the tile (or the z-level below) is inaccessible.

I am unsure about tests for this.

Me, from elsewhere:

"It looks like walkable is computed by df, right? The down ramp itself is not walkable. I suspect we'll have to special case ramps in Maps and have it tie its walkability group to an adjacent tiles group" AND ensure the tile below the ramp also matches the group of its adjacent tile.

Assumed invariant: adjacent tiles belong to the same walkability group.

dhthwy · 2024-11-29T20:55:11Z

Apparently I may have worked on a separate problem that I saw. After re-reading the issue, if dfhack claims wagon is accessible when it is not, then this doesn't fix that. Back to the start on this one -- though I still think these changes may be wanted ~~and possibly necessary for the fix.~~

dhthwy · 2024-11-29T23:26:31Z

Marked as draft temporarily.

…rectly reporting wagon accessibility

dhthwy · 2024-11-30T20:04:04Z

The first four commits deal with what I perceive to be an issue with how down ramps are handled. Without those commits, we get X's on the down ramps, but not on the up ramps. The lack of symmetry is odd. However, it probably doesn't belong in this PR - especially the change in Maps.

The down ramp appearance change in pathable will, iirc, visually make down ramps appear accessible for wagons. I believe the change in maps was to do the same for 'follow mouse' in gui/pathable.

I think I am done with this pending review ~~and pending some tests on that adv mode save from the original complaint.~~

myk002 · 2024-12-07T12:57:16Z

I see some wonkiness with trees and boulders:

but notably, it's not all trees and boulders

test save: https://drive.google.com/file/d/1ID5Tu9XteyEwCpeKuFVjf3ygfWiXe6K0/view?usp=sharing

library/modules/Maps.cpp

myk002

don't forget a changelog entry!

myk002 · 2024-12-07T13:18:36Z

plugins/pathable.cpp


    if (ctx.wgroup == Maps::getWalkableGroup(pos))
        return true;

+    // RAMP_TOP is assigned a walkability group if that commit is accepted
+    // so this test, I think, would be useless.
    if (shape == df::tiletype_shape::RAMP_TOP ) {
        df::coord pos_below = pos + df::coord(0, 0, -1);
        if (Maps::getWalkableGroup(pos_below)) {


assuming the Maps module change does not go in quite yet, do we need to add logic here that checks whether the walkability group of pos_below matches ctx.wgroup?

Assumed invariant: adjacent (RAMP_TOP) tiles belong to the same walkability group.

The group returned by Maps::getWalkableGroup(pos) for a RAMP_TOP will only return a non-zero group if an adjacent tile is the same group as a tile below the ramp.

I think this test here

if (ctx.wgroup == Maps::getWalkableGroup(pos)) return true;

is sufficient. But I agree, this change should probably not be done in this PR.

This reverts commit 2b76868.

This reverts commit ec83a70.

…cent tile" This reverts commit 7e6ec6f.

This reverts commit 50fe672.

remove invalidated RAMP_TOP code

dhthwy · 2024-12-07T22:48:11Z

I see some wonkiness with trees and boulders
but notably, it's not all trees and boulders

I noticed this on the linked save for this, and mine as well. I haven't figured out the root cause yet -- but it doesn't appear to impact whether the wagon can reach the depot. When this occurs, instead of marking the tree/boulder blocked, it seems to mark the surrounding tiles blocked instead.

…ulders.

dhthwy · 2024-12-08T03:10:26Z

I think that last commit solves it. I didn't see any incorrectly marked tiles after that commit using the save you linked to. However, there's an incorrectly marked boulder on a surface edge tile in my own save that the latest commit doesn't resolve.

myk002 · 2024-12-09T00:59:40Z

The edge tiles need a separate check (which can be solved in a separate PR). The edge tile map is populated indiscriminately. There should be a wagon pathability check on each tile before it is added to the map

myk002

code overally LGTM, just one substantive question and a few nits

myk002 · 2024-12-09T01:26:50Z

plugins/pathable.cpp

@@ -248,12 +332,13 @@ static void check_wagon_tile(FloodCtx & ctx, const df::coord & pos) {
    ctx.seen.emplace(pos);

    if (ctx.entry_tiles.contains(pos)) {
-        ctx.wagon_path.emplace(pos);
+        ctx.wagon_path.emplace(pos); // Is this needed?


probably not since the rendering of the edge tile will overwrite the rendering of the wagon path

actually, keep this in, since we might want to highlight the overlap (by coloring it green?)

myk002 · 2024-12-09T01:29:10Z

plugins/pathable.cpp

+        // Ensure our wagon flood end points lands on an edge tile.
+        // When there is no path to the depot entry_tiles will not
+        // contain any edge tiles.
+        if ((pos.x == 0 || pos.y == 0) && entry_tiles.contains(pos)) {


why are we restricting the entry edge match to the top or left of the map?

Huge oversight. Thanks for pointing that out.

myk002 · 2024-12-09T01:31:05Z

plugins/pathable.cpp

+        // When there is no path to the depot entry_tiles will not
+        // contain any edge tiles.


this statement is not quite correct. when there is no path to the depot, then the search_edge will never contain any edge tiles. entry_tiles contains edge tiles regardless of the accessibility of the depot.

myk002 · 2024-12-09T01:33:02Z

plugins/pathable.cpp

    auto tt = Maps::getTileType(pos);
    if (!tt)
        return false;

    auto shape = tileShape(*tt);
-    if (shape == df::tiletype_shape::STAIR_UP || shape == df::tiletype_shape::STAIR_UPDOWN)
-        return false;
+    auto& occ = *Maps::getTileOccupancy(pos);


while the return value should never ever be NULL if tt is not NULL, it's still common practice to check it before dereferencing it

myk002 · 2024-12-09T01:35:24Z

plugins/pathable.cpp

+        default:
+            //NOTREACHED
+            break;


can remove the default case so the compiler complains if another case ever appears

myk002 · 2024-12-09T01:36:18Z

plugins/pathable.cpp

+
+// NOTE: When i.e. tracks, stairs have a bridge over them, the tile will have
+// an occupancy of floored.
+static bool is_wagon_tile_traversible(df::tiletype& tt) {


df::tiletype is just an int (an enum), so no need to take a reference

myk002 · 2024-12-09T01:36:43Z

plugins/pathable.cpp

+    auto special = tileSpecial(tt);
+    auto material = tileMaterial(tt);
+
+    // Allow murky pool ramps


maybe explain why you're calling out murky pools in particular

…, plus resolve some other issues

ref: #5053

dhthwy added 5 commits November 29, 2024 09:12

DFHack#5031: Handle top ramps when pathfinding to depot

50fe672

DFHack#5031: Assign RAMP_TOP a walkability group from an adjacent tile

7e6ec6f

DFHack#5031: Remove debug prints

ec83a70

DFHack#5031: Fix MSVC build. Missing headers?

2b76868

Merge branch 'develop' into wagonpathfix

d6083da

dhthwy marked this pull request as draft November 29, 2024 23:26

DFHack#5031: make wagon_flood() check it reaches an edge tile for cor…

3c7a3b2

…rectly reporting wagon accessibility

dhthwy force-pushed the wagonpathfix branch from 7826dca to 3c7a3b2 Compare November 30, 2024 18:48

dhthwy marked this pull request as ready for review November 30, 2024 20:04

dhthwy changed the title ~~#5031: Handle top ramps when pathfinding to depot~~ #5031: Fix wagon accessibility Nov 30, 2024

dhthwy changed the title ~~#5031: Fix wagon accessibility~~ #5031: Fix accessibility logic for wagons Nov 30, 2024

dhthwy added 2 commits December 1, 2024 13:18

DFHack#5031: Make wagon pathability tests more robust

8ea4f6a

Merge branch 'develop' into wagonpathfix

317b87d

dhthwy force-pushed the wagonpathfix branch from ba18c4c to 317b87d Compare December 1, 2024 18:21

dhthwy added 2 commits December 1, 2024 15:51

Correctly handle wagon accessible tiles for passable buildings

a0a37f3

DFHack#5031: handle tracks, murky pool and river tiles

2a1a734

myk002 reviewed Dec 7, 2024

View reviewed changes

library/modules/Maps.cpp Outdated Show resolved Hide resolved

myk002 reviewed Dec 7, 2024

View reviewed changes

dhthwy added 6 commits December 7, 2024 14:21

Merge branch 'develop' into wagonpathfix

d11dd6d

Revert "DFHack#5031: Fix MSVC build. Missing headers?"

7c384c9

This reverts commit 2b76868.

Revert "DFHack#5031: Remove debug prints"

464ef96

This reverts commit ec83a70.

Revert "DFHack#5031: Assign RAMP_TOP a walkability group from an adja…

47eb2ed

…cent tile" This reverts commit 7e6ec6f.

Revert "DFHack#5031: Handle top ramps when pathfinding to depot"

3acd1eb

This reverts commit 50fe672.

remove invalidated RAMP_TOP comment

0063cd7

remove invalidated RAMP_TOP code

dhthwy force-pushed the wagonpathfix branch from dcf91bf to 0063cd7 Compare December 7, 2024 20:14

DFHack#5031: update changelog

67497ed

DFHack#5031: check the center too. maybe fix errant passable trees/bo…

10dd5d0

…ulders.

dhthwy added 2 commits December 7, 2024 22:10

Merge branch 'develop' into wagonpathfix

7e33948

Merge branch 'develop' into wagonpathfix

22c7ebe

Merge branch 'develop' into wagonpathfix

6c5dc47

myk002 reviewed Dec 9, 2024

View reviewed changes

dhthwy added 4 commits December 8, 2024 20:57

Merge branch 'develop' into wagonpathfix

d5ac7c6

DFHack#5031: check if path exists for bottom and right edge tiles too…

e926210

…, plus resolve some other issues

DFHack#5031: resolve some other minor issues

ef0b0e4

DFHack#5031: don't add non-edge tiles to entry_tiles

5d24e07

myk002 approved these changes Dec 9, 2024

View reviewed changes

myk002 merged commit ef3144c into DFHack:develop Dec 9, 2024
14 checks passed

myk002 added a commit that referenced this pull request Dec 9, 2024

fix off-by-one error

e4f3cd9

ref: #5053

dhthwy deleted the wagonpathfix branch December 9, 2024 18:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#5031: Fix accessibility logic for wagons #5053

#5031: Fix accessibility logic for wagons #5053

dhthwy commented Nov 29, 2024 •

edited

Loading

dhthwy commented Nov 29, 2024 •

edited

Loading

dhthwy commented Nov 29, 2024

dhthwy commented Nov 30, 2024 •

edited

Loading

myk002 commented Dec 7, 2024 •

edited

Loading

myk002 left a comment

myk002 Dec 7, 2024

dhthwy Dec 7, 2024 •

edited

Loading

dhthwy commented Dec 7, 2024

dhthwy commented Dec 8, 2024 •

edited

Loading

myk002 commented Dec 9, 2024

myk002 left a comment

myk002 Dec 9, 2024

myk002 Dec 9, 2024

myk002 Dec 9, 2024

dhthwy Dec 9, 2024

myk002 Dec 9, 2024

myk002 Dec 9, 2024

myk002 Dec 9, 2024

myk002 Dec 9, 2024

myk002 Dec 9, 2024

		// When there is no path to the depot entry_tiles will not
		// contain any edge tiles.

#5031: Fix accessibility logic for wagons #5053

#5031: Fix accessibility logic for wagons #5053

Conversation

dhthwy commented Nov 29, 2024 • edited Loading

dhthwy commented Nov 29, 2024 • edited Loading

dhthwy commented Nov 29, 2024

dhthwy commented Nov 30, 2024 • edited Loading

myk002 commented Dec 7, 2024 • edited Loading

myk002 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhthwy Dec 7, 2024 • edited Loading

Choose a reason for hiding this comment

dhthwy commented Dec 7, 2024

dhthwy commented Dec 8, 2024 • edited Loading

myk002 commented Dec 9, 2024

myk002 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhthwy commented Nov 29, 2024 •

edited

Loading

dhthwy commented Nov 29, 2024 •

edited

Loading

dhthwy commented Nov 30, 2024 •

edited

Loading

myk002 commented Dec 7, 2024 •

edited

Loading

dhthwy Dec 7, 2024 •

edited

Loading

dhthwy commented Dec 8, 2024 •

edited

Loading