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

Updated CPos struct to use bit field for all properties. #15464

Merged
merged 1 commit into from Oct 1, 2018

Conversation

Projects
None yet
4 participants
@teinarss
Copy link
Contributor

teinarss commented Aug 4, 2018

Trying to minimize the memory footprint for structs used in the pathfinding, so changing the coordinates to use short instead of int saves us 32 bits for this struct.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Aug 4, 2018

We're going to need to audit the code for any places these are multiplied together (and not automatically upcast to ints) to make sure we're not introducing overflow bugs.

@teinarss teinarss force-pushed the teinarss:CPos_64bits branch 2 times, most recently from b5c5df3 to a825d12 Aug 29, 2018

@teinarss teinarss changed the title Updated CPos struct to use short for x,y properties. Updated CPos struct to use bit field for all properties. Aug 29, 2018

@teinarss

This comment has been minimized.

Copy link
Contributor

teinarss commented Aug 29, 2018

Updated: Made changes according to the discussion on irc.

@pchote
Copy link
Member

pchote left a comment

LGTM. I have a couple of suggestions to improve readability / understanding, and this needs a rebase after the subcell change.

const int LayerMask = 0x000000FF;
const int YMask = 0x00000FFF;
const int LayerBits = 8;
const int YBits = LayerBits + 12;

This comment has been minimized.

@pchote

pchote Sep 30, 2018

Member

IMO defining these constants actually makes this much harder to understand at a glance

How about something like this?

// Coordinates are packed in a 32 bit signed int
// X and Y are 12 bits (signed): -2048...2047
// Layer is an unsigned byte
// Packing is XXXX XXXX XXXX YYYY YYYY YYYY LLLL LLLL
public readonly int Bits;

// X is padded to MSB, so bit shift does the correct sign extension
public int X { get { return Bits >> 20; } }

// Align Y with a short, cast, then shift the rest of the way
// The signed short bit shift does the correct sign extension
public int Y { get { return ((short)(Bits >> 4)) >> 4; } }

public byte Layer { get { return (byte)Bits; } }

public CPos(int bits) { Bits = bits; }
public CPos(int x, int y) : this(x, y, 0) { }
public CPos(int x, int y, byte layer)
{
	Bits = (x & 0xFFF) << 20 | (y & 0xFFF) << 8 | layer;
}

Note that I also dropped unnecessary masking from Layer.

public static CPos operator +(CVec a, CPos b) { return new CPos(a.X + b.X, a.Y + b.Y, b.Layer); }
public static CPos operator +(CPos a, CVec b) { return new CPos(a.X + b.X, a.Y + b.Y, a.Layer); }
public static CPos operator -(CPos a, CVec b) { return new CPos(a.X - b.X, a.Y - b.Y, a.Layer); }
public static CPos operator +(CVec a, CPos b) { return new CPos((short)(a.X + b.X), (short)(a.Y + b.Y), b.Layer); }

This comment has been minimized.

@pchote

pchote Sep 30, 2018

Member

I don't think these casts give us anything? They are immediately promoted back to ints for the CPos ctor.

@@ -1,4 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
<?xml version="1.0" encoding="utf-8"?>

This comment has been minimized.

@pchote

pchote Sep 30, 2018

Member

Please unstage the byte order mark here

@teinarss teinarss force-pushed the teinarss:CPos_64bits branch from a825d12 to fd8212b Oct 1, 2018

@teinarss

This comment has been minimized.

Copy link
Contributor

teinarss commented Oct 1, 2018

Update: Made the changes and rebased!

@pchote

pchote approved these changes Oct 1, 2018

@pchote pchote added the PR: Needs +2 label Oct 1, 2018

@IceReaper
Copy link
Contributor

IceReaper left a comment

Looks good to me 👍

@pchote pchote merged commit cfaf5a6 into OpenRA:bleed Oct 1, 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