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

Add Int32Matrix4x4 struct. #14940

Merged
merged 1 commit into from Mar 21, 2018

Conversation

Projects
None yet
4 participants
@RoosterDragon
Copy link
Member

RoosterDragon commented Mar 18, 2018

This allows matrices to be represented as a value type, and additionally allows avoiding array allocations when calculating rotations.

Reduces ongoing memory allocations on the RA shellmap by 14%.

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Mar 18, 2018

OpenRA.Game/Int32Matrix4x4.cs:L43: [SA1513] Statements or elements wrapped in curly brackets must be followed by a blank line.

@RoosterDragon RoosterDragon force-pushed the RoosterDragon:matrix-struct branch from 4be9c5a to 3fde346 Mar 18, 2018

@@ -105,6 +105,7 @@
<Compile Include="Actor.cs" />
<Compile Include="CacheStorage.cs" />
<Compile Include="FileSystem\IPackage.cs" />
<Compile Include="Int32Matrix4x4.cs" />

This comment has been minimized.

@pchote

pchote Mar 18, 2018

Member

Can we please put this in Primitives alongside our other composite number types?

This comment has been minimized.

@RoosterDragon

RoosterDragon Mar 18, 2018

Author Member

Done.

@RoosterDragon RoosterDragon force-pushed the RoosterDragon:matrix-struct branch from 3fde346 to 2701f7c Mar 18, 2018

Add Int32Matrix4x4 struct.
This allows matrices to be represented as a value type, and additionally allows avoiding array allocations when calculating rotations.

@RoosterDragon RoosterDragon force-pushed the RoosterDragon:matrix-struct branch from 2701f7c to 81bdda6 Mar 18, 2018


public static bool operator !=(Int32Matrix4x4 me, Int32Matrix4x4 other) { return !(me == other); }

public override int GetHashCode() { return M11 ^ M22 ^ M33 ^ M44; }

This comment has been minimized.

@abcdefg30

abcdefg30 Mar 19, 2018

Member

I guess this won't be an issue for our use here... However, is only using the main diagonal to create the hash safe enough? Doesn't that mean it is possible to get same hashes for matrices with different values?

This comment has been minimized.

@RoosterDragon

RoosterDragon Mar 19, 2018

Author Member

This is allowable for hash codes in general. If hashes collide then the Equals method is called to determine if they're really equal or not. (Consider that many classes are bigger than 32-bits - you couldn't assign a unique hash code to every possible one without at least some collisions!)

return 0; would be a valid hash for example. It would be a terrible choice since everything having the same hash means you have to check equality of everything and thus lose all the benefits - but it would still be valid. The ideal is a hash that minimizes collisions without being too expensive to calculate (or in this case - type out :)).

This comment has been minimized.

@abcdefg30

abcdefg30 Mar 19, 2018

Member

Ah right, thanks for the clarification! I had already started to wonder about xoring the values as well (as you can get equal hashes, like 0, relatively easy... just two times the same value). That explains it. :)

public override int GetHashCode() { return M11 ^ M22 ^ M33 ^ M44; }

public bool Equals(Int32Matrix4x4 other) { return other == this; }
public override bool Equals(object obj) { return obj is Int32Matrix4x4 && Equals((Int32Matrix4x4)obj); }

This comment has been minimized.

@abcdefg30

abcdefg30 Mar 19, 2018

Member

We usually use as and a null check elsewhere, not sure if there are any other guidelines for Equals methods or was using as just not practical in this case?

This comment has been minimized.

@RoosterDragon

RoosterDragon Mar 19, 2018

Author Member

as should be preferred in almost all code for this type of thing - you're quite right.

In the particular case of structs, is and a cast is faster: https://stackoverflow.com/questions/1583050/performance-surprise-with-as-and-nullable-types. So we tend to use this in all the Equals(object obj) implementations.

For a pragmatic point of view - this is boilerplate code that equality on structs requires, that nothing actually calls, and this is a nicer one liner than as.

This comment has been minimized.

@abcdefg30

abcdefg30 Mar 19, 2018

Member

Ok, thanks for the answer. :)

@pchote

pchote approved these changes Mar 21, 2018

Copy link
Member

pchote left a comment

LGTM

@pchote pchote added the PR: Needs +2 label Mar 21, 2018

@abcdefg30 abcdefg30 merged commit e17ede3 into OpenRA:bleed Mar 21, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Mar 21, 2018

@RoosterDragon RoosterDragon deleted the RoosterDragon:matrix-struct branch Mar 21, 2018

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.