3
\$\begingroup\$

I have a function that creates a rectangle from two rectangles that overlap, I was wondering if there is a smarter way to do it purely mathematically rather than lots of if statements to check min and max the way I currently have setup.

This is my function

    public static bool Overlaps(this Bounds a, Bounds bounds, out Bounds result)
    {
        result = new Bounds();
        //they don't overlap
        if (!a.Intersects(bounds)) return false;

        float minX;
        float maxX;
        float minZ;
        float maxZ;

        if (a.min.x > bounds.min.x)
            minX = a.min.x;
        else
            minX = bounds.min.x;

        if (a.max.x < bounds.max.x)
            maxX = a.max.x;
        else
            maxX = bounds.max.x;

        if (a.min.z > bounds.min.z)
            minZ = a.min.z;
        else
            minZ = bounds.min.z;

        if (a.max.z < bounds.max.z)
            maxZ = a.max.z;
        else
            maxZ = bounds.max.z;

        // using XZ plane for debug
        Vector3 min = new Vector3(minX,0,minZ);
        Vector3 max = new Vector3(maxX,0,maxZ);
        result.SetMinMax(min,max);
        return true;
    }

Here it is in action

enter image description here

\$\endgroup\$
2
\$\begingroup\$
float minX;
float maxX;
float minZ;
float maxZ;

The declarations can be inlined so that it is clearer when they are being set. However, in geometry and other applications it may be preferable to declare them like this.

if (a.max.x < bounds.max.x)
            maxX = a.max.x;
        else
            maxX = bounds.max.x;

These conditions can be replaced with calls to Math.max and Math.min.

The variable names such as minX and maxX seem to be reversed; minX seems to be referring to maxX and vice-versa. I am not very familiar with geometry so I don't know if my suggestion is correct to swap the names.

After applying these suggestions, the method becomes the following:

  public static bool Overlaps(this Bounds a, Bounds bounds, out Bounds result)
    {
        result = new Bounds();
        //they don't overlap
        if (!a.Intersects(bounds)) return false;

        float minX = Math.Max(a.min.x, bounds.min.x);
        float maxX = Math.Min(a.max.x, bounds.max.x);

        float minZ = Math.Max(a.min.z, bounds.min.z);
        float maxZ = Math.Min(a.max.z, bounds.max.z);

        // using XZ plane for debug
        Vector3 min = new Vector3(minX, 0, minZ);
        Vector3 max = new Vector3(maxX, 0, maxZ);
        result.SetMinMax(min, max);
        return true;
    }

Edit: to respond to the comment (minimizing if statements), it could be expressed as:

  public static bool Overlaps(this Bounds a, Bounds bounds, out Bounds result)
    {
        result = new Bounds();

        float minX = Math.Max(a.min.x, bounds.min.x);
        float maxX = Math.Min(a.max.x, bounds.max.x);

        float minZ = Math.Max(a.min.z, bounds.min.z);
        float maxZ = Math.Min(a.max.z, bounds.max.z);

        // using XZ plane for debug
        Vector3 min = new Vector3(minX, 0, minZ);
        Vector3 max = new Vector3(maxX, 0, maxZ);
        result.SetMinMax(min, max);
        return !a.Intersects(bounds);
    }

This would be slightly less efficient; I don't know if this is valid as if the Vector3 class will accept input which did not pass .Intersects.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ But that function call is doing the same thing surely? Other than cleaning up the code its not reduced the if statements and turning it into a purely mathematical approach. \$\endgroup\$ – WDUK Dec 6 '19 at 0:38
  • \$\begingroup\$ @WDUK I have updated my answer with an alternative approach with no if statements. \$\endgroup\$ – alexyorke Dec 6 '19 at 0:48
  • \$\begingroup\$ Thanks for the help :) \$\endgroup\$ – WDUK Dec 6 '19 at 1:18
  • \$\begingroup\$ It's much cleaner with the early exist code. Plus porbalby more effecient too. \$\endgroup\$ – Anders Dec 6 '19 at 9:56
0
\$\begingroup\$

I don't think it is correct to write to the output parameter result when you return false. Maybe you should return the result or null instead.

Apart from that I would yet make @alexyorke 's solution more compact (assuming Bounds have constructor from min/max) by getting rid of all the intermediate variables:

public static Bounds GetOverlap(this Bounds a, Bounds bounds)
{
    if (!a.Intersects(bounds)) return null;

    return new Bounds(
        new Vector3(
            Math.Max(a.min.x, bounds.min.x),
            0,
            Math.Max(a.min.z, bounds.min.z)
        ),
        new Vector3(
            Math.Min(a.max.x, bounds.max.x),
            0,
            Math.Min(a.max.z, bounds.max.z)
        )
    );
}

You may still define the shorthand, but I would suggest omitting the output parameter in which case it becomes alias of Intersects anyway, so maybe it is useless:

public static bool Overlaps(this Bounds a, Bounds bounds)
{
    return a.Intersects(bounds);
}

And btw if you want to speed it up a little bit, you may look into the Intersects function and you are likely to see that some things that you do in the Overlaps function are done there too. Reimplementing Overlaps without calling Intersects may then be faster, but since you haven't provided the Intersects function, I cannot help here.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Structs can't be null and a bounds of zero returned will still require me to check if the result has a valid bounds to draw. So I opted for the bool / out option. \$\endgroup\$ – WDUK Dec 6 '19 at 11:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.