Page MenuHomeWildfire Games

Allow a template-defined clearance around buildings (static obstructions)
AbandonedPublic

Authored by wraitii on Apr 11 2017, 5:26 PM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Owners Package(Owns No Changed Paths)
Restricted Owners Package(Owns No Changed Paths)
Trac Tickets
#3685
Summary

Rebased from sanders's patch for #3685 with his permission.

This patch allows defining a "clearance" around the obstruction of a building against which new placements are checked. In effect, it prevents the player from building stuff too close to each other, and specifically here it ensures at least an infantry-sized unit can always pass.

This would IMO be a really nice feature to get.

Obviously, down the road, we should probably add some GUI indications of what's happening, but I don't think that's necessary for now.

Test Plan

Load up the game and play buildings.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1095
Build 1725: Vulcan BuildJenkins
Build 1724: arc lint + arc unit

Event Timeline

wraitii created this revision.Apr 11 2017, 5:26 PM
Owners added a subscriber: Restricted Owners Package.Apr 11 2017, 5:26 PM
Vulcan added a subscriber: Vulcan.Apr 11 2017, 6:18 PM

Build is green

Updating workspaces.
Build (release)...
../../../source/simulation2/components/CCmpObstruction.cpp:537:78: warning: unused parameter ‘onlyCenterPoint’ [-Wunused-parameter]
  virtual EFoundationCheck CheckFoundation(const std::string& className, bool onlyCenterPoint) const
                                                                              ^
Build (debug)...
../../../source/simulation2/components/CCmpObstruction.cpp:537:78: warning: unused parameter ‘onlyCenterPoint’ [-Wunused-parameter]
  virtual EFoundationCheck CheckFoundation(const std::string& className, bool onlyCenterPoint) const
                                                                              ^
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/746/ for more details.

mimo added a subscriber: mimo.Apr 13 2017, 9:32 AM

not clear to me how it works: it seems that you only test on size+clearance when you position a structure? if yes, that won't give you what you expect as it will depend on the order you build: if you start with a structure with a big clearance and then one without clearance, I guess the second can touch the first one.
IMO the correct solution would be to apply the BLOCK_FOUNDATION flag to the size area and the BLOCK_PATHFINDING flag to the size-clearance area (possibly increasing the size so that the newSize = oldSize + clearance).

Then specific problems in the patch:

  • in the static element, it seems that the new clearance is in meter while width and depth are in cells? it yes, that's really bug prone.
  • the AI needs to be modified to take this clearance into account. Note that it already takes some internal clearance into account (see for example lines 306 to 313 of simulation/ai/petra/queueplanBuilding.js, but there may be other places where it have some).

Not my patch so I'll answer as best I can:

not clear to me how it works: it seems that you only test on size+clearance when you position a structure? if yes, that won't give you what you expect as it will depend on the order you build: if you start with a structure with a big clearance and then one without clearance, I guess the second can touch the first one.

I do believe that's correct and working as designed. Most structures have the same clearance, but a wall has negative clearance, and thus can be built slightly on top of other structures. I'm not really seeing that as a problem, tbh. It's not supposed to be a perfect way to keep buildings apart.

in the static element, it seems that the new clearance is in meter while width and depth are in cells? it yes, that's really bug prone.

Mh, maybe the opposite rather? You might be right there though, will check.

AI issues are indeed true, back in my days clearance was most definitely only in one place so unless it's been changed since it should be easy-ish to fix.

mimo added a comment.Apr 13 2017, 9:49 AM
In D318#12620, @wraitii wrote:

Not my patch so I'll answer as best I can:

not clear to me how it works: it seems that you only test on size+clearance when you position a structure? if yes, that won't give you what you expect as it will depend on the order you build: if you start with a structure with a big clearance and then one without clearance, I guess the second can touch the first one.

I do believe that's correct and working as designed. Most structures have the same clearance, but a wall has negative clearance, and thus can be built slightly on top of other structures. I'm not really seeing that as a problem, tbh. It's not supposed to be a perfect way to keep buildings apart.

For me that's inconsistent: if you first build a wall, then you can't have structures with clearance touching it. But if you first build a structure with clearance, you can have a wall touching it. That's not what i would expect, while what i proposed above should work.

It would, but tbh I'm not sure it is feasible with the current code. I'd need to check but my instincts tell me "no". Though I guess we could always rasterise specifically for this.

Imarok awarded a token.Jul 7 2017, 9:18 PM
Imarok rescinded a token.
Imarok awarded a token.
wraitii updated this revision to Diff 4062.Nov 1 2017, 5:56 PM

Rebased (somehow cleanly)

wraitii added a reviewer: Restricted Owners Package.Nov 1 2017, 5:56 PM
Vulcan added a comment.Nov 1 2017, 7:50 PM

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

Updating workspaces...
Build (release)...
../../../source/simulation2/components/CCmpObstruction.cpp:537:78: warning: unused parameter ‘onlyCenterPoint’ [-Wunused-parameter]
  virtual EFoundationCheck CheckFoundation(const std::string& className, bool onlyCenterPoint) const
                                                                              ^
Build (debug)...
../../../source/simulation2/components/CCmpObstruction.cpp:537:78: warning: unused parameter ‘onlyCenterPoint’ [-Wunused-parameter]
  virtual EFoundationCheck CheckFoundation(const std::string& className, bool onlyCenterPoint) const
                                                                              ^
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Vulcan added a comment.Nov 1 2017, 7:51 PM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/components/BuildRestrictions.js
| 127| »   case·"land":
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'default'.

binaries/data/mods/public/simulation/components/BuildRestrictions.js
| 158| »   var·cmpPlayer·=·QueryOwnerInterface(this.entity,·IID_Player);
|    | [NORMAL] JSHintBear:
|    | 'cmpPlayer' is already defined.

binaries/data/mods/public/simulation/components/BuildRestrictions.js
| 242| »   »   »   ||·(cmpWaterManager.GetWaterLevel(pos.x·-·sz,·pos.y·-·cz)·-·cmpTerrain.GetGroundLevel(pos.x·-·sz,·pos.y·-·cz))·>·2.0)·//·back
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/BuildRestrictions.js
| 257| »   »   var·cmpRangeManager·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_RangeManager);
|    | [NORMAL] JSHintBear:
|    | 'cmpRangeManager' is already defined.

binaries/data/mods/public/simulation/components/BuildRestrictions.js
| 258| »   »   var·cmpPlayer·=·QueryOwnerInterface(this.entity,·IID_Player);
|    | [NORMAL] JSHintBear:
|    | 'cmpPlayer' is already defined.
gameboy added a comment.EditedJan 21 2018, 11:05 AM

@wraitii My friend, only you can help us to solve this problem, please take a moment to look at this question, thank you again, my old friend.

https://wildfiregames.com/forum/index.php?/topic/23893-it-didnt-help-us/

wraitii planned changes to this revision.May 5 2018, 4:03 PM

I believe this doesn't entirely fix the issue anyways. Removing from the review queue.

wraitii abandoned this revision.Jan 1 2019, 10:25 AM

Didn't fix the problem mimo highlighted above, so this is a bit of a dead end.