With the height difference one can still climb impassable hills and edges. The last thing I recall about this ticket is that the hierarchical pathfinder could be used to test actual passability.
This still solves the issue in most cases. Also for ships it sounds like the reachability test can't be used due to the different passability classes, in which case we still need your patch I guess.
Magic numbers should never be hidden in the code, at least it should be moved to the top of the file as a const.

In D1040#40850, @elexis wrote:

With the height difference one can still climb impassable hills and edges. The last thing I recall about this ticket is that the hierarchical pathfinder could be used to test actual passability.

That was my first thought, but regions can be connected even though there's a cliff between them (take the long way around). So instead I thought to try maybe a straight line terrain test, but then there's a problem because the building itself will be included in the impassable area. So instead you'd have to do terrain-only. But then as you note there's problems going between water and land, either with ungarrisoning from ships or spawning from docks (Briton's island settlement and Athens' dock can spawn land units).

This still solves the issue in most cases.

Combined with a smaller spawning area (in D1041), hopefully it should, especially in the most egregious cases.

That was my first thought, but …

It would with the A* path feature. And not getting units stuck in impassable terrain is probably more important than spawning them in far-away accessible regions.

Edit: well it would if we stored what region a building is in, since that makes it impassable. So right now no it can't.

Using the hierarchical pathfinder would have another advantage: when a rally-point has been set (in an accessible region), only tiles connected to the rally-point would be looked at. That would really help with ships surrounded by several islands (or even small isolated rocks) where currently units can be spread in different non-connected regions.

In D1040#40862, @mimo wrote:

Using the hierarchical pathfinder would have another advantage: when a rally-point has been set (in an accessible region), only tiles connected to the rally-point would be looked at. That would really help with ships surrounded by several islands (or even small isolated rocks) where currently units can be spread in different non-connected regions.

But ships don't have rally points? In D1041 I use positions in front of the ship first before moving around to the back (rather than the current way of doing a row at a time), plus I use a smaller area, so those things should help mitigate the problem.

In D1040#40916, @temple wrote:

In D1040#40862, @mimo wrote:

Using the hierarchical pathfinder would have another advantage: when a rally-point has been set (in an accessible region), only tiles connected to the rally-point would be looked at. That would really help with ships surrounded by several islands (or even small isolated rocks) where currently units can be spread in different non-connected regions.

But ships don't have rally points?

We can always add one if needed (only problem would be to add a hotkey or command order to differentiate it from a move).
And i think it would be really useful for other purposes (but i was too lazy to do it): imagine putting a rally point on some foundation, the ship would move as near as possible to the foundation, ungarrison its units which would then automatically go to the foundation and start building it.

In D1040#40918, @mimo wrote:

We can always add one if needed (only problem would be to add a hotkey or command order to differentiate it from a move).
And i think it would be really useful for other purposes (but i was too lazy to do it): imagine putting a rally point on some foundation, the ship would move as near as possible to the foundation, ungarrison its units which would then automatically go to the foundation and start building it.

I can't say that's a real common scenario, because you'd need a civic center on the island in order to build anything in the first place. :) But I agree an "ungarrison here" command would be nice. It could work with siege towers unloading units over walls (#2038), and keeping units in rams until they're close enough to a target.

As mentioned in D1037, this is the only C++ component returning a 3D vector with Y = 0. This patch adds a getter for the Y coordinate and the patch seems to be useful even if we add a hierarchical pathfinder implementation additionally.
So we could fix the return value while at it.
Committing D1039 prior would mean that this diff and the cleanup will be shorter by 50%, so should look into scheduling that first.

The magic number 5 is still fishy. C++ gameplay constants are ugly af. We already have a system where we save arbitrary property values of components, we should use it here too IMO.

In D1040#41227, @elexis wrote:

As mentioned in D1037, this is the only C++ component returning a 3D vector with Y = 0. This patch adds a getter for the Y coordinate and the patch seems to be useful even if we add a hierarchical pathfinder implementation additionally.
So we could fix the return value while at it.

Unless we're going to stack units on top of each other, like airplanes or submarines, it doesn't seem that useful. I'd prefer returning the 2D vector since that's all we use.

Committing D1039 prior would mean that this diff and the cleanup will be shorter by 50%, so should look into scheduling that first.

Yes, I'm skipping PickSpawnPointBothPass here. Note that doing D1041 first would cut the work in half again. :)

The magic number 5 is still fishy. C++ gameplay constants are ugly af. We already have a system where we save arbitrary property values of components, we should use it here too IMO.

Fixed.

In D1040#42129, @wraitii wrote:

I think this would be better as a component of footprint. This precludes having a "stairway" building that lets you go atop mountains on purpose, for example.

Are there plans for such a thing? It wouldn't be hard to add an IgnoreHeightCheck to footprint later on, if it's needed.
But the examples here are Athens docks and Briton island settlements, where we don't want to check the height difference for ships but we do want to check it for infantry.

It wouldn't be hard to add an IgnoreHeightCheck to footprint later on, if it's needed.

I mean it isn't hard to make this an element of the template either :p . As a general rule, I prefer when we go for easier moddability.

But the examples here are Athens docks and Briton island settlements, where we don't want to check the height difference for ships but we do want to check it for infantry.

I mean… Shouldn't we want to check it for docks too? Doesn't make a ton of sense to have a dock atop a mountain that creates ships (not that this is possible atm).

If other people think a pathfinder argument is the way to go, fine, but I think it's too inflexible.

I meant it as a property of the Footprint component as well.

Allows changing it for every unit and building. (So we could make it dependend on stairs being visible in the actor or whatever). Also means pathfinder.xml is kept minimal. Also modders have no clue about the pathfinder file.

Having the property in the entity template doesn't mean we can pick a different height difference for ships and units in docks, but we don't have that versatility in the pathfinder file either.

In D1040#42223, @elexis wrote:

Having the property in the entity template doesn't mean we can pick a different height difference for ships and units in docks, but we don't have that versatility in the pathfinder file either.

Yes we do, because units and ships have different passability classes. Currently infantry (default, large) use 8m and ships (ship, ship-small) use whatever the max fixed is.

It might be messy adding in water height and float depth and whatever else, but I can try...

In D1040#42226, @temple wrote:

Yes we do, because units and ships have different passability classes. Currently infantry (default, large) use 8m and ships (ship, ship-small) use whatever the max fixed is.

Oh that's how it works.
Then maybe we could request a tiebreaker opinion, maybe @bb, mimo or leper once more as the other simulation experts left.

I don't really see why ships would not use the height difference when land units would.

In D1040#42229, @wraitii wrote:

I don't really see why ships would not use the height difference when land units would.

One reason is that the bad behavior is units climbing up mountains, but ships don't climb up mountains anyway.
The other reason is "It might be messy adding in water height and float depth and whatever else, but I can try...".

In D1040#42238, @temple wrote:

In D1040#42229, @wraitii wrote:

I don't really see why ships would not use the height difference when land units would.

One reason is that the bad behavior is units climbing up mountains, but ships don't climb up mountains anyway.
The other reason is "It might be messy adding in water height and float depth and whatever else, but I can try...".

I didn't try. But docks and ships both float, so it seems pointless to check.
Was just thinking about a reverse extinct volcano map, where a lake drains out? Could be interesting...

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

Honestly I really don't see why this should go into pathfinder settings. The only place where we use it is Footprint, it just makes sense to make it a component of footprint.

In D1040#45710, @wraitii wrote:

Honestly I really don't see why this should go into pathfinder settings. The only place where we use it is Footprint, it just makes sense to make it a component of footprint.

Because I feel it should be a property of the spawned unit not the thing it's spawned from. But since it's working for ships now and since we don't have flying units, I guess we could do it that way.

Now just let me add MaxSpawnHeightDifference to a hundred different templates...

Because I feel it should be a property of the spawned unit not the thing it's spawned from. But since it's working for ships now and since we don't have flying units, I guess we could do it that way.

I feel like "both" would be best, in fact. I agree that it makes some sense to put in in the possibility class for the spawned unit, as it represents some sort of "jumping/hiking" ability that generally affects pathfinding too. But it also depends on what they are spawned from (as given in the earlier "stairway" building).

Anyways I'll test that this works and accept it this WE unless somebody comes first, I don't think it's actually that blocking.

Now just let me add MaxSpawnHeightDifference to a hundred different templates...

That's like 3/4 at most if you use templates correctly.

In D1040#45729, @wraitii wrote:

Now just let me add MaxSpawnHeightDifference to a hundred different templates...

That's like 3/4 at most if you use templates correctly.

I gave up before I started. It might not be too bad except we do <Footprint replace=""> all over the place.

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

In D1040#50228, @wraitii wrote:

Feel like this is either not worth fixing or it needs a number of smaller changes.

What would those changes be? Units climbing up mountains is a bad bug, maybe could even be a release blocker, no?

Maybe we ought to make all units creation first garrison the unit then expel it? Or use some kind of generic "unit expeller" component that handles ungarrisoning, creating with a single, more customisable interface.

Not all spawns are garrisonable, e.g. elephants from forts or units from a couple heroes. But I'm not sure what you mean or how it's relevant to this patch?