[PATCH] Building Placement Restrictions

[historic_bruno]

Buildings should have restrictions on where they can be placed:

• Docks can only be built in a shallow coastal area (where villagers can reach it to build and created boats will be water units).
  • Possibly have docks autorotate to be perpendicular to the shore, so the user is not burdened with doing this.
• Other buildings must be built on land.
• There may be restrictions by territory (once #41 is implemented), e.g. requiring a building to be on user's own and/or ally territory.

(See simulation\components\BuildRestrictions.js for more details)

[Erik Johansson]

Maybe it would be worth creating an "Advanced Building Placement Restrictions" for the last two? They are both dependent on territories/settlements to be implemented so maybe they will not be possible to be finished at the same time as the rest. On the other hand that may be implemented before this ticket is closed, so the best is probably to wait until then and create a new ticket in that case :)

[historic_bruno]

This is one of the most common complaints I see, and it's highly annoying, so I'm bumping up priority.

[Kieran P]

Sure :-) Want to take a crack at it?

[historic_bruno]

I'll see if any good solutions come to mind :)

[Kieran P]

As a reminder, this also needs to work when units are garrisoned to a ship (they should be able to reach it), and when they are ungarrisoned from a ship (put on land within reach of the ship). At no point should the ship be allowed to ungarrison over deep water.

[Erik Johansson]

Kieran, your comment is all good and true, but is it just me or should it perhaps be posted on another ticket? I fail to see any connection between boat garrisoning and building placement restrictions :-/ Perhaps I'm missing something though :)

[Kieran P]

I suppose, but my thinking was, if you redesigning the system for building placement, you could also put some thought to make it possible to extend for unit placements/spawning. Maybe it doesn't get done in this ticket, but don't design the system in a way that'll make it harder later.

[historic_bruno]

Attaching first version of patch for passability check when placing buildings and spawning units (so it also addresses part of #893). Feedback needed :)

[Philip Taylor]

Haven't tried actually running the code, but some comments from reading it:

We really need to be able to tell people why they can't build a building ("blocked by terrain", "blocked by building", "outside the world", "outside territory", "in SoD", "in FoW", "insufficient resources", "reached max number", "reached max number per territory", "too close to civ center" - any others?). So things like CheckPlacement/CheckBuildingPlacement/IsValidFoundation ought to return some kind of enum indicating the detailed type of problem, instead of just a bool. (Could probably be handled in a separate ticket from this if that'd be easier, since it's not a new problem or necessary for this patch.)

BuildRestrictions.prototype.CheckPlacement -- should check cmpPosition.IsInWorld() before calling GetPosition() on it. (Probably impossible for that to fail in this particular case, but it's good to be explicit about guaranteeing API preconditions). Also, should probably use GetPosition2D() instead (which totally-not-confusingly returns X and Z coordinates in .x and .y) to save a neglible amount of processing time computing the Y coordinate.

This looks wrong:

cmpTerrain.GetGroundLevel(pos.x + s*halfX, pos.z + c*halfZ)

The front/back of the building is the vector (0, +/-halfZ) (assuming buildings face north/south at angle 0 - I may be off by pi/2), and the standard rotation matrix is (c -s / s c) so multiplying gives (-/+ s*halfZ, +/- c*halfZ), or maybe some other combination of + and - depending on directions. In any case it shouldn't use both halfX and halfZ.

Maybe we should check the depth of water, rather than just checking the front is underwater by at least 0? Otherwise it looks like you could build a dock in shallows where ships are unable to exist. I don't have a good idea of how to check that kind of thing properly, though - maybe wait and see if it's a problem in practice.

GuiInterface.prototype.GetFoundationSnapData -- snapping based on terrain normal seems kind of dodgy, since shores won't always be sloping downwards smoothly. Possible slope-independent alternative (not thought through very much): Sample a number of points (say 16) in a circle around the building, identify which points are underwater, then pick the 'middle' of the underwater points (not sure how to calculate that - maybe find the longest sequence of adjacent underwater points, then choose the middle of that sequence), and point in that direction.

ICmpUnitMotion::GetPassabilityClass() (and callers etc) -- should use ICmpPathfinder::pass_class_t, not u16, since we might want to change it later. CCmpUnitMotion.cpp ought to use them for its m_{Pass,Cost}Class members too (currently it uses u8 for pass which is too small - presumably my bug but this is a good opportunity to fix it).

CCmpFootprint::PickSpawnPoint -- circular shapes should probably do the expand-out-by-4-tiles thing too.

CCmpPathfinder::UpdateGrid:

Floats are evil so fixed::FromFloat(1.f) should be fixed::FromInt(1).

(allowing for precision error): what precision error? Seems more like the definition of shore, not anything to do with an error.

If there's e.g. a lake with depth 0.9, it looks like the entire lake will be considered 'shore', which doesn't sound intentional. It might be better to do a first pass over the grid marking each tile as underwater/overwater, then a second pass where a tile is marked as 'shore' if it is overwater and an adjacent tile is underwater, so you're guaranteed to get a single-tile-wide strip of shore around the water, regardless of how deep the water or steep the terrain is.

Should document why the max shore distance is set to 32767, not e.g. 65535, since it doesn't seem particularly obvious. (I assume it's to allow conversion to fixed without overflow, and/or to prevent the u16 min from overflowing.)

CCmpPathfinder::CheckUnitPlacement -- why in CCmpPathfinder_Vertex.cpp? Doesn't seem to have anything to do with the vertex-based pathfinder; probably CCmpPathfinder.cpp would be a better fit.

CCmpPathfinder::CheckUnitPlacement -- should iterate over j in the outer loop and i in inner, for more cache-efficient grid access.

CCmpPathfinder::CheckBuildingPlacement -- re a better way to do this: maybe more like CCmpTerritoryManager::RasteriseInfluences and/or CCmpObstructionManager::Rasterise which use Geometry::PointIsInSquare so they only test tiles that are under the obstruction shape, and which handle rotations correctly. (But expand the obstruction shape by 1/sqrt(2) tiles first, so tiles that even partially overlap the obstruction will have their center point overlap the expanded shape.)

Formatting nitpicking:

data/pathfinder.xml, templates/template_structure_military_dock.xml -- bad indentation (should use spaces, not tabs).

cmpWaterMgr -- maybe should be cmpWaterManager to fit the vague undocumented convention of mirroring the interface name. (Sometimes I forget that myself, but that's usually unintentional.)

switch(this.template.PlacementType) -- should have space before (.

Also there's some inconsistent trailing whitespace everywhere which is really ugly in my diff tool so I get disproportionately irritated :-)

[ben]

(In [9970]) Implements building restrictions (by terrain, territory, category, and distance). See #41. Fixes #804, #287. Implements build limits. See #687. Implements autorotation for dock placement. Fixes unit spawning to consider terrain passability. See #893. Adds new passability criteria based on distance from shore. Updates build restrictions on some templates. Changes unit spawning search to 4 tiles away from foundation. Changes garrison/training spawn failure to nicer UI notification.