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

Reimplement Box.Inflate with System.Drawing matching functionality #1616

Closed
NogginBops opened this issue Jul 17, 2023 · 3 comments · Fixed by #1662
Closed

Reimplement Box.Inflate with System.Drawing matching functionality #1616

NogginBops opened this issue Jul 17, 2023 · 3 comments · Fixed by #1662
Milestone

Comments

@NogginBops
Copy link
Member

NogginBops commented Jul 17, 2023

Description

In #1511 and #1615 Box.Inflate got marked as Obsolete pending a change to the functionality of the function to match System.Drawing.Rectangle.Inflate.

We should add a remark in the documentation of Box.Inflate that it used to do something else so that someone who is upgrading from 4.7.7 -> 4.8.2 has any chance to find the issue when debugging.

@NogginBops NogginBops added this to the 4.8.1 milestone Jul 17, 2023
@NogginBops
Copy link
Member Author

Going to target this for 4.8.2 instead.

@NogginBops NogginBops modified the milestones: 4.8.1, 4.8.2 Sep 25, 2023
@MV10
Copy link
Contributor

MV10 commented Oct 15, 2023

@NogginBops so basically re-implement this from #1455 ?

e61afa3

public void Inflate(Vector2 size)
{
    size = Vector2.ComponentMax(size, -HalfSize);
    _min -= size;
    _max += size;
}

public Box2 Inflated(Vector2 size)
{
    // create a local copy of this box
    Box2 box = this;
    box.Inflate(size);
    return box;
}

Here's the .NET 7 implementation (src):

public void Inflate(int width, int height)
{
    unchecked
    {
        X -= width;
        Y -= height;

        Width += 2 * width;
        Height += 2 * height;
    }
}

@MV10
Copy link
Contributor

MV10 commented Oct 15, 2023

I have this ready to PR, but I thought I'd go the extra mile and implement a real Inflate test, which the older PR did not.

I'm no F# expert (I'm not even an F# amateur), nor am I a fan of unit tests, but I think this right. However, it fails due to what looks to me like a rounding error between C# and F#... How the heck do you deal with that? (And this is why I hate unit tests, anything non-trivial is essentially "rewriting the same code a slightly different way" and I inevitably spend more time debugging tests than real code.)

module Inflate =
    [<Property>]
    let ``Box2.Inflate produces the expected min and max changes`` (b1 : Box2, v1 : Vector2) =
        let size = Vector2.ComponentMax(v1, -b1.HalfSize);
        let b = Box2(b1.Min - size, b1.Max + size)
        Assert.Equal(b, b1.Inflated(v1))

Note "expected" from F# is -0.0739928 but actual from C# is -0.07399279:

 OpenTK.Tests.Box2+Inflate.Box2.Inflate produces the expected min and max changes
   Source: Box2Tests.fs line 161
   Duration: 105 ms

  Message: 
FsCheck.Xunit.PropertyFailedException : 
Falsifiable, after 1 test (0 shrinks) (StdGen (613089187, 297245411)):
Original:
((-0.3379175, -1.2313157) - (0.18993191, 0.104034156), (-0.9891462, 0.027614703))

---- Assert.Equal() Failure
Expected: (-0.0739928, -1.2589304) - (-0.07399279, 0.13164885)
Actual:   (-0.07399279, -1.2589304) - (-0.0739928, 0.13164885)

Maybe this is why the earlier PR skipped it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants