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 slopes in Map.DistanceAboveTerrain. #17990

Merged
merged 3 commits into from May 28, 2020

Conversation

pchote
Copy link
Member

@pchote pchote commented Apr 28, 2020

This PR implements the slightly delayed followup to #8866. Actors now follow the terrain height as they move over slopes, instead of floating above or clipping below the map as they interpolate between cell centers. This should completely fix infantry movement, but vehicle artwork will obviously still clip until actor orientations have been implemented and the voxel rendering fixed.

Wikipedia explains the maths, and walkmap.c has my original implementation that was adapted for this PR. Knowing the geometry of the cells allows us to do some tricks, such as hardcoding the determinant as a constant and allowing u,v to extend outside their formal range to evaluate the height over the full square for RampSplit.Flat. As usual, calculations are done as integers between 0-1024 instead of floats between 0-1.

Closes #17219 and is one of the prerequisites to properly integrating vehicle tilting. Future PRs will further expand CellRamp to expose the ramp normal (for voxel shadow calculation) and rotation (for actor orientation).

@pchote pchote added this to the Next+1 milestone Apr 28, 2020
Mailaender
Mailaender previously approved these changes Apr 28, 2020
Copy link
Member

@Mailaender Mailaender left a comment

Choose a reason for hiding this comment

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

This looks very good in-game and I can't spot regressions.

@pchote
Copy link
Member Author

pchote commented Apr 29, 2020

Updated.

@penev92
Copy link
Member

penev92 commented Apr 29, 2020

A minor nit that I mentioned on Discord is that the br/bl/tl/tr names when predefining the Corners in CellRamp are inconsistent with their semantic as defined later in the code (the ramp type definitions), but that does not affect the correctness or working of the code.
Edit: Guess I was wrong.
From Discord: The ramp definitions below this left me with the distinct impression that cells are rotated clockwise, while the corner definitions (as they are in the PR) lead me to believe cells are rotated counterclockwise. That's where my confusion started from.

A bigger concern is that Map.CenterOfCell() still returns an erroneous Z component that needs to be adjusted in (currently one) of its use-cases.

Other than that it all looks good and seems to work pretty well. 👍

OpenRA.Game/Map/MapGrid.cs Outdated Show resolved Hide resolved
@pchote
Copy link
Member Author

pchote commented May 2, 2020

TODO:

  • Remove unused split field
  • Confirm that tl,tr,bl,br actually do correspond to the assumed corners in isometric grids
  • Fix CenterOfCell and CenterOfSubCell and check their uses to make sure this won't regress anything.

@pchote
Copy link
Member Author

pchote commented May 3, 2020

Updated. Fixing CenterOfCell and CenterOfSubCell required a much larger rewrite, so this will need to be rereviewed and retested again.

reaperrr
reaperrr previously approved these changes May 17, 2020
Copy link
Contributor

@reaperrr reaperrr left a comment

Choose a reason for hiding this comment

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

Code looks good as far as I can tell, didn't notice any in-game regressions

@pchote
Copy link
Member Author

pchote commented May 27, 2020

Rebased.

@atlimit8 atlimit8 merged commit 626b40f into OpenRA:bleed May 28, 2020
@atlimit8
Copy link
Member

Changelog

@@ -89,7 +90,8 @@ void ConvertBridgeToActor(World w, CPos cell)
var subtile = new CPos(ni + ind % template.Size.X, nj + ind / template.Size.X);

// This isn't the bridge you're looking for
if (!mapTiles.Contains(subtile) || mapTiles[subtile].Type != tile || mapTiles[subtile].Index != ind)
var subti = mapTiles[subtile];
if (!mapTiles.Contains(subtile) || subti.Type != tile || subti.Index != ind)
Copy link
Member

Choose a reason for hiding this comment

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

This moved the lookup before the Contains check!

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.

Cell height is defined incorrectly
6 participants