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

Vector4 docs #3857

Merged
merged 5 commits into from May 29, 2015
Merged

Vector4 docs #3857

merged 5 commits into from May 29, 2015

Conversation

Chaosus
Copy link
Contributor

@Chaosus Chaosus commented May 16, 2015

Fully documented Vector4 struct,
+few fixes to other things
+tests
+added missing Transform overloads

@nkast
Copy link
Contributor

nkast commented May 17, 2015

This PR brought up something strange with how Length() , LengthSquared() and static Normalize(ref Vector4 value, out Vector4 result) are implemented. Since zero is now readonly, you had to replace the DistanceSquared(ref this, ref zero, out result) with the slower DistanceSquared(this, zero).
The use of DistanceSquared(...) was inefficient in the first place. Same problem with Vector3 while Vector2 has those inline.
@tomspilman, @KonajuGames , should we rewrite those in the future?

@Chaosus
Copy link
Contributor Author

Chaosus commented May 17, 2015

@nkast Thanks, I fixed it. please said me if I incorrect.

@nkast
Copy link
Contributor

nkast commented May 18, 2015

Actually I was talking about inlining Length(), LengthSquared() and Normalize(ref Vector4 value, out Vector4 result). My plan was to submit the changes after this PR was merged.
I thing the rule here is to get 'ref' methods optimized and the slower counterparts call the ref methods.
But the inline of Normilize() here is good, this roundtrip of this is too much and one can argue that a member method is not the same as the 'static' method.

@Chaosus
Copy link
Contributor Author

Chaosus commented May 18, 2015

@nkast Ok. I left these changes for you.
@tomspilman @KonajuGames This is good PR or I should change it a bit ?

@Chaosus
Copy link
Contributor Author

Chaosus commented May 23, 2015

@tomspilman @KonajuGames So.. what about merging ?

@Chaosus
Copy link
Contributor Author

Chaosus commented May 28, 2015

@tomspilman Please react

@@ -392,9 +392,9 @@ public static void Distance(ref Vector3 value1, ref Vector3 value2, out float re
/// <returns>The squared distance between two vectors.</returns>
public static float DistanceSquared(Vector3 value1, Vector3 value2)
Copy link
Member

Choose a reason for hiding this comment

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

We need a unit test for this as there isn't one and we need to be sure this didn't break anything.

Copy link
Member

Choose a reason for hiding this comment

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

Or if @nkast plans to re-write this then I can merge as is. He will just need to write the tests for it then.

@Chaosus
Copy link
Contributor Author

Chaosus commented May 29, 2015

So I made these tests.. it is ok ?

@tomspilman
Copy link
Member

Ok... merging!

tomspilman added a commit that referenced this pull request May 29, 2015
@tomspilman tomspilman merged commit 174170e into MonoGame:develop May 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants