Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make service depots passable but not blockable #17141

Merged
merged 4 commits into from Jan 4, 2020
Merged

Conversation

@tovl
Copy link
Contributor

tovl commented Sep 22, 2019

Fixes #16934
Fixes #16465

This makes all service depots passable for the pathfinder, but prevents units from stopping on the service depot when not ordered to repair. It also fixes the repair arm in TS from being passable.

The same logic is also applied to the bibs of weapon factories and refineries in TS to prevent production blocking and to make them more in line with the original (#17105 (comment)).

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Sep 22, 2019

This is a cool idea, but am I correct in assuming that it isn't enough by itself to recreate all the details of TS's exit logic? IIRC placing barracks next to a cliff edge can block exits in our TS, while I expect that the same positioning should work fine in the original.

We should see what the MP community thinks about applying this logic to prevent ref/wf blocking in RA and TD.

@Punsho

This comment has been minimized.

Copy link
Contributor

Punsho commented Sep 23, 2019

This type of footprint was mentioned a while ago when ERCC was being made. It was said it was the only proper substitute for transparency. I cannot speak for TD but in RA applying it to all structures would be just removing a strategic aspect of the game for no gain. Blocking War Factory and Refineries is meta

@teinarss

This comment has been minimized.

Copy link
Contributor

teinarss commented Sep 23, 2019

This type of footprint was mentioned a while ago when ERCC was being made. It was said it was the only proper substitute for transparency. I cannot speak for TD but in RA applying it to all structures would be just removing a strategic aspect of the game for no gain. Blocking War Factory and Refineries is meta

Think this was the same conclusion in TD too. To keep the blocking as a strategic aspect.

@tovl

This comment has been minimized.

Copy link
Contributor Author

tovl commented Sep 23, 2019

am I correct in assuming that it isn't enough by itself to recreate all the details of TS's exit logic?

This covers the exit logic for TS warfactories and refineries, which have one obligatory exit and a bib that makes sure the exit is clear. It does not cover TS barracks/hand-of-nod, which have a different logic. They have one preferred exit, but when that one is blocked the units can exit from any unblocked side—even diagonals. i.e. you can completely surround them except for one tile and they will exit through that tile, spawning near the edge of that tile.

I don't know how it was implemented in the original game, but this behaviour can probably be easily emulated, simply by defining 12 different exits for the barracks (and 14 for hand-of-nod) in yaml and making them evaluate in order of definition instead of randomly. I'd say #16526 would be a slight improvement over that.

We'd still need to teach bots to keep some distance when placing buildings (#17105 (comment)), which would be another separate logic.

We should see what the MP community thinks about applying this logic to prevent ref/wf blocking in RA and TD.

I have deliberately refrained from applying this to existing mods, because such a change might be controversial. Bibs are also visually different in TS. They look more like they are part of the building proper while the RA/TD mods look more like they are part of the terrain. It might be counter-intuitive in RA/TD (although I haven't checked how the original RA/TD handled this.)

@tovl tovl force-pushed the tovl:repairpad branch 2 times, most recently from c6e6636 to db153ad Oct 1, 2019
@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 5, 2019

Testing the original game's behaviour, I found that you can get units to stop on the bibs using the stop key while they are moving over. I prefer our approach here, though.

I have implemented the barracks behaviour in #17183.

Testing this PR I find that units will stop on factory bibs if nudged or if the stop key is spammed on a newly produced vehicle.

@tovl

This comment has been minimized.

Copy link
Contributor Author

tovl commented Oct 5, 2019

Testing the original game's behaviour, I found that you can get units to stop on the bibs using the stop key while they are moving over.

You're right, I didn't notice before. It seems like an oversight in the originals though, because the force-move behaviour seems very deliberate and allowing units to stop there using the stop hotkey is inconsistent with that.

I have implemented the barracks behaviour in #17183.

Nice!

Testing this PR I find that units will stop on factory bibs if nudged

That will be easy to fix, but it will save some rebasing pain if we can get #17131 merged first.

@Punsho

This comment has been minimized.

Copy link
Contributor

Punsho commented Oct 22, 2019

It would be beneficial for RA to make it possible for enemies to block service depot. Then SD would not be an exception in that regard

@tovl tovl force-pushed the tovl:repairpad branch from db153ad to cc83ca6 Oct 30, 2019
@tovl

This comment has been minimized.

Copy link
Contributor Author

tovl commented Oct 30, 2019

Dependencies have been merged and the issues with nudges and stop commands have been fixed.

@ghost

This comment has been minimized.

Copy link

ghost commented Nov 19, 2019

This type of footprint was mentioned a while ago when ERCC was being made. It was said it was the only proper substitute for transparency. I cannot speak for TD but in RA applying it to all structures would be just removing a strategic aspect of the game for no gain. Blocking War Factory and Refineries is meta

Think this was the same conclusion in TD too. To keep the blocking as a strategic aspect.

In the original game, tanks fired on walls when they were blocking their path. You saw some parts of vehicles inside the war factory when they were stuck. Both behaviors are missing in OpenRA and IMO a natural improvement over the original game would be to allow armed vehicles to fire on what's blocking them from inside the factory. This would also make blocking factory exits a bit more exciting. The animation below shows some more details about the different behavior for blocked production facilities:

EXITBLOCKED

@ghost

This comment has been minimized.

Copy link

ghost commented Nov 20, 2019

The idea I proposed above has nothing to do with the PR so can be ignored.

@tovl

This comment has been minimized.

Copy link
Contributor Author

tovl commented Nov 20, 2019

It is a good idea though. At the very least, units shooting at enemy walls that are blocking their move should be added. It is probably best to repost your comment as a new issue.

@tovl

This comment has been minimized.

Copy link
Contributor Author

tovl commented Dec 16, 2019

Rebased.

@@ -125,6 +129,9 @@ public IEnumerable<CPos> UnpathableTiles(CPos location)

foreach (var t in FootprintTiles(location, FootprintCellType.OccupiedUntargetable))
yield return t;

foreach (var t in FootprintTiles(location, FootprintCellType.OccupiedPassableCannotStop))

This comment has been minimized.

Copy link
@pchote

pchote Dec 30, 2019

Member

Is this correct? If it is, can we please include a comment to explain why we want to be including tiles that claim to be pathable in a method that claims to be returning tiles that aren't pathable?

This comment has been minimized.

Copy link
@tovl

tovl Dec 30, 2019

Author Contributor

The output of UnpathableTiles gives the occupied cells, which needs to include the 'hot' cells in order to be able to have any interaction with locomotors. This method seems to have been named under the assumption that unpathable and occupied are equivalent; this is obviously no longer the case now. It is probably better to just rename the method.

This comment has been minimized.

Copy link
@tovl

tovl Dec 30, 2019

Author Contributor

Renamed.

OpenRA.Mods.Common/Traits/Buildings/Building.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Buildings/Building.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Buildings/Building.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Mobile.cs Show resolved Hide resolved
@@ -743,11 +766,14 @@ public CPos NearestMoveableCell(CPos target, int minRange, int maxRange)
if (target.Layer != 0)
target = new CPos(target.X, target.Y);

if (CanEnterCell(target))

This comment has been minimized.

Copy link
@pchote

pchote Dec 30, 2019

Member

Does our current code still contain the gotcha that CanEnterCell(self.Location) returns false, or did one of the previous movement PRs straighten that out?

If not, we'll need to check that all of the callers behave properly when self.Location is returned.

This comment has been minimized.

Copy link
@tovl

tovl Dec 30, 2019

Author Contributor

Yes, this is still the case. The line below this was added to work around that.

This also resolves the long-standing issue that units will always move away when given a move order to their current location.

OpenRA.Mods.Common/Traits/World/Locomotor.cs Outdated Show resolved Hide resolved
@pchote

This comment has been minimized.

Copy link
Member

pchote commented Dec 30, 2019

while testing (rebased on latest bleed, if this is important), I found - without doing anything special - that a unit decided to stop in the middle of the service depot:

Screenshot 2019-12-30 at 19 43 17

The group of three wolverines in a line were together to the bottom-left of the image, and I issued an order to the top-right, where the first one is stopped. I don't know how to reproduce this, and all three units were idle and 100% health.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Dec 30, 2019

Replay, which was recorded on https://github.com/pchote/OpenRA/tree/17141-glitch.

Glitch happens at 1:13.

@tovl tovl force-pushed the tovl:repairpad branch 2 times, most recently from 23acc70 to 2bbcfc5 Dec 30, 2019
@tovl

This comment has been minimized.

Copy link
Contributor Author

tovl commented Dec 30, 2019

The glitch was caused by the nearEnough logic. It should be fixed now.

Copy link
Member

pchote left a comment

LGTM overall, just one last minor nit then lets get this merged. Sorry to keep dragging this out...

I suspect that the NearestMoveableCell fix will have unexpected fallout, but better to fix it now and deal with them when they are discovered than to keep the existing bogus behaviour.

OpenRA.Mods.Common/Traits/Buildings/Building.cs Outdated Show resolved Hide resolved
@tovl tovl force-pushed the tovl:repairpad branch from 2bbcfc5 to fa691ae Jan 4, 2020
@pchote
pchote approved these changes Jan 4, 2020
@pchote pchote merged commit 2094142 into OpenRA:bleed Jan 4, 2020
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.