[4.0] refactored .Math stylecop errors #806

jvbsl · 2018-08-23T17:30:40Z

Purpose of this PR

stylecop.props import readded to .Math and fixed all stylecop errors
fixed some xml comment inconsistencies
fixed missing file documentation at top of file(in .Core/Utility/BlittableValueType{T}.cs)

Testing status

Well it does build and it shouldn't have any major implementation changes.

Comments

fixed some stuff of #802 and now in a feature branch. I hope the simple naming is enough.
I did not reorder static variables, because there still seems to be a discussion about that.

* stylecop.props import readded to .Math and fixed all stylecop errors * fixed some xml comment inconsistencies * fixed missing file documentation at top of file(in .Core/Utility/BlittableValueType{T}.cs)

FreezyLemon · 2018-08-23T17:48:49Z

src/OpenTK.Mathematics/Matrix/Matrix3x2.cs

-            float m21
-        )
+        [SuppressMessage("ReSharper", "SA1117", Justification = "For better readability of Matrix struct.")]
+        public Matrix3x2(


I like the idea of these constructor "shapes", but OpenTK uses the "Allman" brace style, which means you should move the parentheses like this:

public Matrix2x3 ( // arguments // more arguments )

I don't think the shape will lose in expressiveness by moving the parentheses a bit

Nihlus

Had a look through, looks mostly good. There are a few things that repeat throughout the PR that need to be fixed - I've put a review comment on the first instance that I found of each issue.

Nihlus · 2018-08-23T18:27:50Z

src/OpenTK.Mathematics/Data/Quaternion.cs

@@ -191,7 +191,7 @@ public Vector4 ToAxisAngle()
        /// <summary>
        /// Returns a copy of the Quaternion scaled to unit length.
        /// </summary>
-        /// <returns>The normalized copy.</returns>


Possible loss of information. Should probably keep as "copy" instead of "Quaternion". We know it's a quaternion.

well I'm not too sure on that I mean it's a struct, of course it's a copy

It's more informative than Quaternion; copy semantics are an important facet of the mathematics subsystem.

Agreed with @Nihlus - it's possible if it were badly designed that it would normalize the current instance and THEN return a copy of that.

something I would never expect from any implementation ever, except if the Method name would be Normalize.
But if already two people think so. I'm gonna change it back.

Nihlus · 2018-08-23T18:28:15Z

src/OpenTK.Mathematics/Matrix/Matrix2.cs

-                rM12 = right.Row0.Y,
-                rM21 = right.Row1.X,
-                rM22 = right.Row1.Y;
+            float leftM11 = left.Row0.X,


Don't declare multiple variables this way. Each one gets its own separate declaration.

Nihlus · 2018-08-23T18:29:12Z

src/OpenTK.Mathematics/Matrix/Matrix3.cs

@@ -781,7 +777,7 @@ public static void Invert(ref Matrix3 mat, out Matrix3 result)
            {
                { mat.Row0.X, mat.Row0.Y, mat.Row0.Z },
                { mat.Row1.X, mat.Row1.Y, mat.Row1.Z },
-                { mat.Row2.X, mat.Row2.Y, mat.Row2.Z }
+                { mat.Row2.X, mat.Row2.Y, mat.Row2.Z },


Do not add trailing commas.

Nihlus · 2018-08-23T18:29:42Z

src/OpenTK.Mathematics/Matrix/Matrix3x4.cs

+            result.Row2.Z = (leftM31 * rightM13) + (leftM32 * rightM23) + (leftM33 * rightM33);
+            result.Row2.W = (leftM31 * rightM14) + (leftM32 * rightM24) + (leftM33 * rightM34) + leftM34;
+
+            /*result.Row0 = (right.Row0 * leftM11 + right.Row1 * leftM12 + right.Row2 * leftM13);


If this comment is not used, remove it.

Nihlus · 2018-08-23T18:30:08Z

src/OpenTK.Mathematics/Matrix/Matrix3x4d.cs

@@ -312,6 +304,7 @@ public void Invert()
        /// <param name="axis">The axis to rotate about.</param>
        /// <param name="angle">Angle in radians to rotate counter-clockwise (looking in the direction of the given axis).</param>
        /// <param name="result">A matrix instance.</param>
+        [SuppressMessage("Microsoft.StyleCop.CSharp.DocumentationRules", "SA1305", Justification = "Is not hungarian notation but abbreveation for precalculated values.")]


What error is this actually suppressing?

jvbsl · 2018-08-23T20:20:56Z

src/OpenTK.Mathematics/Data/Quaternion.cs

@@ -191,7 +191,7 @@ public Vector4 ToAxisAngle()
        /// <summary>
        /// Returns a copy of the Quaternion scaled to unit length.
        /// </summary>
-        /// <returns>The normalized copy.</returns>


something I would never expect from any implementation ever, except if the Method name would be Normalize.
But if already two people think so. I'm gonna change it back.

jvbsl · 2018-08-24T14:41:21Z

src/OpenTK.Mathematics/Matrix/Matrix3.cs

@@ -354,7 +349,7 @@ public Matrix3 Inverted()
        /// <summary>
        /// Returns a copy of this Matrix3 without scale.
        /// </summary>
-        /// <returns>The matrix.</returns>
+        /// <returns>The matrix without scaling.</returns>


What aboout these ones, because The return value comment here does not specify that it is a copy. Perhaps we should mark such functions as "Pure" as well?

Marking them as pure is a good idea. In general, JetBrains annotations should be used to annotate contracts, purity, and nullability.

I left this be at first. And I think I will do pure functions in a separate commit(perhaps even separate PR?).
Question is do we want to have the annotations in the public API as well? Because Jetbrains recommends that if we to, to include it directly into the SourceCode instead of using the nuget package.

We'll be using Fody to generate external annotations, which is what we're already doing in the other projects.

* Vector/Quaternion functions not modifying the object(Pure) specify that they return a copied value(again). * declare variables spread over multiple lines(and statements) now * removed some unused code which was commented out * unecessary error supression removed * use Allman brace style for matrix parameters * removed trailing spaces(I could find)

jvbsl · 2018-08-24T16:40:35Z

Could you review again please? Don't know if you saw that I did a new commit with hopefully all changes ;)

Nihlus · 2018-08-24T17:23:05Z

src/OpenTK.Mathematics/Data/Quaternion.cs

@@ -210,7 +210,7 @@ public void Invert()
        /// <summary>
        /// Returns a copy of this Quaterniond with its rotation angle reversed.
        /// </summary>
-        /// <returns>The inverted copy.</returns>


Same thing here, loss of information.

Nihlus · 2018-08-24T17:28:31Z

src/OpenTK.Mathematics/Vector/Vector4h.cs

@@ -129,7 +129,7 @@ public Vector4h(float x, float y, float z, float w, bool throwOnError)
        /// <summary>
        /// Initializes a new instance of the <see cref="Vector4h"/> struct.
        /// </summary>
-        /// <param name="v">The <see cref="Vector4"/> to convert.</param>


Why this change? This constructor explicitly converts a normal vector.

good quetion. I can't remember to have done this probably happened while merging...

Nihlus · 2018-08-24T17:29:31Z

Nice job! Just a few nitpicks left.

* fixed a mistake from wrong merge choice * fixed two missed return value descriptions(added information that a copy is returned)

* implemented double precision for MathHelper.InverseSqrtFast

jvbsl · 2018-08-24T18:54:43Z

src/OpenTK.Mathematics/MathHelper.cs

@@ -189,23 +189,20 @@ public static float InverseSqrtFast(float x)
        /// which is found in the Quake III source code. This implementation comes from
        /// http://www.codemaestro.com/reviews/review00000105.html. For the history of this method, see
        /// http://www.beyond3d.com/content/articles/8/.
+        /// double magic number from: https://cs.uwaterloo.ca/~m32rober/rsqrt.pdf
+        /// chapter 4.8.
        /// </remarks>
        public static double InverseSqrtFast(double x)


fixed this one as well, because it was a pretty easy and fast one. And I think it would still fit in here, because everything was in .Math.
Edit: More important is actually the Method body

Nihlus · 2018-08-24T18:56:32Z

Spankin'. Thanks a ton!

[4.0] refactored .Math stylecop errors

1023800

* stylecop.props import readded to .Math and fixed all stylecop errors * fixed some xml comment inconsistencies * fixed missing file documentation at top of file(in .Core/Utility/BlittableValueType{T}.cs)

jvbsl mentioned this pull request Aug 23, 2018

4.0 #802

Closed

FreezyLemon reviewed Aug 23, 2018

View reviewed changes

Nihlus suggested changes Aug 23, 2018

View reviewed changes

jvbsl commented Aug 24, 2018

View reviewed changes

Nihlus suggested changes Aug 24, 2018

View reviewed changes

jvbsl added 2 commits August 24, 2018 20:39

[4.0] refactored .Math some last bits and bobs

2d8c79b

* fixed a mistake from wrong merge choice * fixed two missed return value descriptions(added information that a copy is returned)

[4.0] refactored .Math missing implementation for double precision

3175f05

* implemented double precision for MathHelper.InverseSqrtFast

jvbsl commented Aug 24, 2018

View reviewed changes

Nihlus approved these changes Aug 24, 2018

View reviewed changes

Nihlus merged commit 997c902 into opentk:4.0 Aug 24, 2018

jvbsl deleted the feature/refactor branch August 24, 2018 19:14

varon added this to Done in OpenTK 4.0 Oct 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[4.0] refactored .Math stylecop errors #806

[4.0] refactored .Math stylecop errors #806

jvbsl commented Aug 23, 2018

FreezyLemon Aug 23, 2018 •

edited

Loading

Nihlus left a comment

Nihlus Aug 23, 2018

jvbsl Aug 23, 2018

Nihlus Aug 23, 2018

varon Aug 23, 2018

jvbsl Aug 23, 2018

Nihlus Aug 23, 2018

Nihlus Aug 23, 2018

Nihlus Aug 23, 2018

Nihlus Aug 23, 2018

jvbsl Aug 23, 2018

jvbsl Aug 24, 2018

Nihlus Aug 24, 2018

jvbsl Aug 24, 2018

Nihlus Aug 24, 2018

jvbsl commented Aug 24, 2018

Nihlus Aug 24, 2018

Nihlus Aug 24, 2018

jvbsl Aug 24, 2018

Nihlus commented Aug 24, 2018

jvbsl Aug 24, 2018 •

edited

Loading

Nihlus commented Aug 24, 2018

[4.0] refactored .Math stylecop errors #806

[4.0] refactored .Math stylecop errors #806

Conversation

jvbsl commented Aug 23, 2018

Purpose of this PR

Testing status

Comments

FreezyLemon Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

Nihlus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvbsl commented Aug 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nihlus commented Aug 24, 2018

jvbsl Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

Nihlus commented Aug 24, 2018

FreezyLemon Aug 23, 2018 •

edited

Loading

jvbsl Aug 24, 2018 •

edited

Loading