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

GrantConditionOnTerrain checks Location.Layer==0 to fix apcs swimming #14684

Merged
merged 2 commits into from May 10, 2018

Conversation

Projects
None yet
4 participants
@fruestueck
Copy link
Contributor

fruestueck commented Jan 7, 2018

Fix APCs 'swimming' over bridges

Fixes part of #14592

34468893-00783bba-ef13-11e7-8fb4-b2a683786bea

I messed PR #14621 (with rebase) up so i cloesd it and made the exact same change here.

@MunWolf

This comment has been minimized.

Copy link
Contributor

MunWolf commented Feb 19, 2018

I am not sure if checking for Location.Layer == 0 will be good enough, not exactly sure how it works so I defer checking that to someone else.

@@ -52,7 +52,7 @@ void ITick.Tick(Actor self)
return;

var currentTerrain = self.World.Map.GetTerrainInfo(self.Location).Type;

This comment has been minimized.

@pchote

pchote Feb 21, 2018

Member

Other layers can actually define their own terrain types, so a more comprehensive fix would be something like:

var currentTerrain = self.Location.Layer == 0 ? self.World.Map.GetTerrainInfo(self.Location).Type :
		self.World.Map.Rules.TileSet[self.World.GetCustomMovementLayers()[self.Location.Layer].Index].Type;

(hopefully with some tweaks to cache appropriate references!)

This comment has been minimized.

@fruestueck

fruestueck Feb 22, 2018

Author Contributor

I tried your approach but it returns wrong Tileset indexes
(on bridges DirtyRoad instead of Road and in tunnels Bridge)

What do you mean by cache appropriate references? Storing the last cell (before entering tunnel/bridge) and working with that Custom Terrain instead of the Actors current?

EDIT: I tried around a bit
var currentTerrain = self.Location.Layer == 0 ? self.World.Map.GetTerrainInfo(self.Location).Type : previousTerrain;
would work too, yet i don't know if it's polished

This comment has been minimized.

@pchote

pchote May 10, 2018

Member

Sorry, my "something like" wasn't quite right: the .Index should have been .GetTerrainIndex(self.Location).

@pchote pchote force-pushed the fruestueck:fix-bridge-grandcondition branch from 347047d to 9b598d5 May 10, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented May 10, 2018

I have updated this with my requested changes and rebased it against current bleed. 👍 with these in place.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented May 10, 2018

The following can be used to verify the correct behaviour of the terrain lookup:

diff --git a/OpenRA.Mods.Common/Traits/Render/RenderDebugState.cs b/OpenRA.Mods.Common/Traits/Render/RenderDebugState.cs
index 1c45227b28..cc39fc1f81 100644
--- a/OpenRA.Mods.Common/Traits/Render/RenderDebugState.cs
+++ b/OpenRA.Mods.Common/Traits/Render/RenderDebugState.cs
@@ -72,7 +72,10 @@ IEnumerable<IRenderable> IRenderAboveShroudWhenSelected.RenderAboveShroud(Actor
                        if (debugVis == null || !debugVis.ActorTags)
                                yield break;
 
-                       yield return new TextRenderable(font, self.CenterPosition - offset, 0, color, tagString);
+                       var currentTerrain = self.Location.Layer == 0 ? self.World.Map.GetTerrainInfo(self.Location).Type :
+                                       self.World.Map.Rules.TileSet[self.World.GetCustomMovementLayers()[self.Location.Layer].GetTerrainIndex(self.Location)].Type;
+
+                       yield return new TextRenderable(font, self.CenterPosition - offset, 0, color, currentTerrain ?? tagString);
 
                        // Get the actor's activity.
                        var activity = self.CurrentActivity;

then enable actor tags in the debug menu.

@pchote pchote dismissed their stale review via c655ed8 May 10, 2018

@pchote pchote force-pushed the fruestueck:fix-bridge-grandcondition branch from 9b598d5 to c655ed8 May 10, 2018

@fruestueck

This comment has been minimized.

Copy link
Contributor Author

fruestueck commented May 10, 2018

hello pchote,
i feel a bit sorry that i am radio silent but my finals are up. Will read you in the summer.
Keep up the good work : )

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented May 10, 2018

I added a fix for the water trails as well (by adding inwater to LeavesTrails.RequiresCondition). 👍 with that.

@reaperrr reaperrr merged commit c309709 into OpenRA:bleed May 10, 2018

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
You can’t perform that action at this time.