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

Box tests #926

Merged
merged 26 commits into from
Jun 13, 2019
Merged

Box tests #926

merged 26 commits into from
Jun 13, 2019

Conversation

frederikja163
Copy link
Member

Purpose of this PR

Comments

  • The Box2i support #918 PR caused a block of the master branch as it wasn't style cop compliant (woops)
  • This caused Box2n and Box3n cleanup #920 to be merged pre maturely (before tests were made)
  • This PR should fix all this box horror that i created

@frederikja163 frederikja163 changed the title Generators [WIP] Generators Jun 6, 2019
Copy link
Contributor

@Perksey Perksey left a comment

Choose a reason for hiding this comment

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

errr clicked approve accidentally. as you were

@Perksey Perksey self-requested a review June 6, 2019 17:43
_max.Y = value.Y;
}

_max = value;
Copy link
Contributor

Choose a reason for hiding this comment

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

why? that changes the meaning?

If I have a Box e.g. Box2(0,0,5,5)
and set min to (0,10)
Box2 will be (0,10,5,10), therefore it having Height of zero.
I would expect it to be similar to the rubberband rectangle in e.g. paint. If you drag the minimal value over to the other side, it will automatically set the max value automatically...

Copy link
Member Author

Choose a reason for hiding this comment

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

@frederikja163 frederikja163 changed the title [WIP] Generators [WIP] Tests Jun 9, 2019
@frederikja163 frederikja163 changed the title [WIP] Tests [WIP] Box tests Jun 9, 2019
@Perksey Perksey removed their request for review June 11, 2019 15:51
@frederikja163 frederikja163 changed the title [WIP] Box tests Box tests Jun 12, 2019
Copy link
Member

@varon varon left a comment

Choose a reason for hiding this comment

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

Excellent work @frederikja163 . Nicely written tests and great coverage.


[<Property>]
let ``The distance should never be negative`` (b1 : Box3, v1 : Vector3) =
Assert.True(b1.DistanceToNearestEdge(v1) >= (float32)0)
Copy link
Member

Choose a reason for hiding this comment

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

Minor, but it's preferable to use a float literal here. 0.0f

[<Properties(Arbitrary = [|typeof<OpenTKGen>|])>]
module Equality =
[<Property>]
let ``Any box should be equal to itself`` (b1 : Box3) =
Copy link
Member

Choose a reason for hiding this comment

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

Whenever we test equality, always a good idea to test hashcode too.


b.Size <- v1

Assert.Equal(b1.Size/(float32)2, b1.HalfSize)
Copy link
Member

Choose a reason for hiding this comment

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

Using literals is preferable to converting. 2 -> 2.0f

@@ -370,7 +288,7 @@ public override int GetHashCode()
/// <inheritdoc/>
public override string ToString()
{
return $"({Min.X}{ListSeparator} {Min.Y}) - ({Max.X}{ListSeparator} {Max.Y})";
return $"({Min.X}{ListSeparator} {Min.Y}{ListSeparator} {Min.Z}) - ({Max.X}{ListSeparator} {Max.Y}{ListSeparator} {Max.Z})";
Copy link
Member

Choose a reason for hiding this comment

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

Probably not necessary to use an explicit ListSeparator.

@varon varon merged commit 30cb843 into opentk:master Jun 13, 2019
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