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

Account for terrain height and slope when calculating order location. #7748

Merged
merged 6 commits into from Apr 1, 2015

Conversation

pchote
Copy link
Member

@pchote pchote commented Mar 28, 2015

Units will now move where you tell them to on TS maps that include slopes (i.e. all except our one debug map). The terrain geometry overlay will now also highlight the mouseover cell to aid in debugging any future problems.

@reaperrr
Copy link
Contributor

This fixes #6247.

About #6840, it looks better now but still isn't the way it should be (although that might be a problem with how inaccuracy is calculated in general).
See below (red circles are the cells I fired at):
flames-001
Smudges are never created below the targeted cell.

@Mailaender
Copy link
Member

This is a huge step forward, but I got a crash when building vehicles on "A river runs nearby":

Exception of type `System.IndexOutOfRangeException`: Array index is out of range.
  at OpenRA.Map.OffsetOfSubCell (SubCell subCell) [0x00001] in /home/matthias/Entwicklung/OpenRA/OpenRA.Game/Map/Map.cs:192 
  at OpenRA.Mods.Common.Activities.Move.Tick (OpenRA.Actor self) [0x0027c] in /home/matthias/Entwicklung/OpenRA/OpenRA.Mods.Common/Activities/Move/Move.cs:187 
  at OpenRA.Traits.Util.RunActivity (OpenRA.Actor self, OpenRA.Activities.Activity act) [0x00017] in /home/matthias/Entwicklung/OpenRA/OpenRA.Game/Traits/Util.cs:83 
  at OpenRA.Mods.Common.Activities.AttackMoveActivity.Tick (OpenRA.Actor self) [0x00057] in /home/matthias/Entwicklung/OpenRA/OpenRA.Mods.Common/Activities/Move/AttackMoveActivity.cs:43 
  at OpenRA.Traits.Util.RunActivity (OpenRA.Actor self, OpenRA.Activities.Activity act) [0x00017] in /home/matthias/Entwicklung/OpenRA/OpenRA.Game/Traits/Util.cs:83 
  at OpenRA.Actor.Tick () [0x00010] in /home/matthias/Entwicklung/OpenRA/OpenRA.Game/Actor.cs:136 
  at OpenRA.World.Tick () [0x000de] in /home/matthias/Entwicklung/OpenRA/OpenRA.Game/World.cs:304 
  at OpenRA.Game.InnerLogicTick (OpenRA.Network.OrderManager orderManager) [0x001bf] in /home/matthias/Entwicklung/OpenRA/OpenRA.Game/Game.cs:458 
  at OpenRA.Game.LogicTick () [0x0004e] in /home/matthias/Entwicklung/OpenRA/OpenRA.Game/Game.cs:482 
  at OpenRA.Game.Loop () [0x000d4] in /home/matthias/Entwicklung/OpenRA/OpenRA.Game/Game.cs:598 
  at OpenRA.Game.Run () [0x00042] in /home/matthias/Entwicklung/OpenRA/OpenRA.Game/Game.cs:638 
  at OpenRA.Program.Run (System.String[] args) [0x00011] in /home/matthias/Entwicklung/OpenRA/OpenRA.Game/Support/Program.cs:111 
  at OpenRA.Program.Main (System.String[] args) [0x00050] in /home/matthias/Entwicklung/OpenRA/OpenRA.Game/Support/Program.cs:39

Not reproducible and does not seem to be caused by any of the code touched here so 👍

@pchote
Copy link
Member Author

pchote commented Mar 28, 2015

Removed references to #6840, since this doesn't completely fix this.

You can verify the self-consistency of the CellContaining changes by putting the following code somewhere that will run once:

var m = self.World.Map;
foreach (var c in m.Cells)
{
    var cc = m.CellContaining(m.CenterOfCell(c));
    if (c != cc)
        Console.WriteLine("Mismatch: {0} -> {1} -> {2}", c, m.CenterOfCell(c), cc);
}

@reaperrr
Copy link
Contributor

:+0.5: for ingame, can't really judge the code.

@obrakmann
Copy link
Contributor

CandidateMouseoverCells seems to return something invalid.

Exception of type `System.IndexOutOfRangeException`: Array index is out of range.
  at OpenRA.CellLayer`1[OpenRA.TerrainTile].get_Item (MPos uv) [0x00009] in /home/oliver/devel/openra/git/OpenRA.Game/Map/CellLayer.cs:97 
  at OpenRA.Graphics.Viewport.ViewToWorld (int2 view) [0x00119] in /home/oliver/devel/openra/git/OpenRA.Game/Graphics/Viewport.cs:121 
  at OpenRA.Mods.Common.Widgets.ViewportControllerWidget.UpdateMouseover () [0x0001f] in /home/oliver/devel/openra/git/OpenRA.Mods.Common/Widgets/ViewportControllerWidget.cs:98 

@pchote
Copy link
Member Author

pchote commented Mar 28, 2015

@obrakmann added a commit to fix that.

var p = map.CenterOfCell(uv.ToCPos(map.TileShape));
var s = worldRenderer.ScreenPxPosition(p);
var dx = Math.Abs(s.X - world.X);
var dy = Math.Abs(s.Y - world.Y);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the worst case scenario you're doing all this for all candidates twice. I know it's just integer maths, but it gets called quite often. Is it worth it to remove that redundancy?

Edit: Thinking about it some more, it's probably not worth it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a tradeoff between making the common case efficient and eating the perf in the worst case, or making the common case slower to save on the worst case. I decided that the worst case (clicking on the side of a cliff) is rare enough that this way is better.

@obrakmann
Copy link
Contributor

In-game this looks good, and I didn't spot any regressions in the older mods, either. The code is mostly above my head and I wouldn't spot a glaring error if it stared me right in the eye. I'll cautiously add my own :+0.5:s to reaperrrs :)

var dy = Math.Abs(s.Y - world.Y);

return dx * dx + dy * dy;
}).First().ToCPos(map);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why .First() instead of [0]?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enumerables can't be indexed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, I wasn't paying attention to the type.

@phrohdoh
Copy link
Member

This works fantastic for me so if there are no objections this can be merged (after lunch). 👍

phrohdoh added a commit that referenced this pull request Apr 1, 2015
Account for terrain height and slope when calculating order location.
@phrohdoh phrohdoh merged commit 28ea598 into OpenRA:bleed Apr 1, 2015
@phrohdoh
Copy link
Member

phrohdoh commented Apr 1, 2015

Changelog entry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants