10
\$\begingroup\$

Users like exploring maps, seeing all that is around. However they tend to go too far. To prevent the map from getting lost to the user it bounces back into the set bounds, as defined by a rectangle.

To do this I keep track of two rectangles, the outer rectangle that the user moves around (the map), and an inner rectangle that serves as the viewport. When the outer rectangle gets swiped all the way to the right, the left side of the inner and outer rectangles overlap. The user can still drag it to see the overhang of the map, but once they release their touch the map moves back to inside the bounds.

My implementation for this: (unity 2019.1.8f1 with C# 4.x runtime IL2CPP):

MapMover.cs

using System;
using System.Collections;
using UnityEngine;
using UnityEngine.EventSystems;

public class MapMover : MonoBehaviour, IDragHandler, IBeginDragHandler, IEndDragHandler
{
    [Flags]
    private enum OutOfBoundsDirection { Inbounds = 1, Left = 2, Right = 4, Top = 8, Bottom = 16 };
    private OutOfBoundsDirection outOfBounds;

    [SerializeField] private GameObject mainCanvas;
    [SerializeField] private BoxCollider2D maskCol;

    private readonly float snapBackSpeed = 100;

    private Vector3 startTouchPos;
    private BoxCollider2D col;
    private WaitForEndOfFrame waitFrame;

    private void Start()
    {
        waitFrame = new WaitForEndOfFrame();
        col = GetComponent<BoxCollider2D>();
    }

    public void OnBeginDrag(PointerEventData eventData)
    {
        startTouchPos = Input.mousePosition;
    }

    /// <summary>
    /// Move the map in a 1:1 scale to the movement of the finger. To do this We need to divide the difference between the start position of the touch and the current position
    /// by the scale of the canvas (and any other parent, whose scale isn't exactly 1. Right now this isn't the case).
    /// </summary>
    /// <param name="eventData"></param>
    public void OnDrag(PointerEventData eventData)
    {
        var posDiff = startTouchPos - Input.mousePosition;
        transform.localPosition -= posDiff / mainCanvas.transform.localScale.x;
        startTouchPos -= posDiff;
    }

    public void OnEndDrag(PointerEventData eventData)
    {
        CheckBounds();
    }

    /// <summary>
    /// Check if the map is still within the set bounds of the inner rect.
    /// </summary>
    private void CheckBounds()
    {
        Rect innerRect = new Rect(maskCol.GetCornerVertices()[3].x, maskCol.GetCornerVertices()[3].y, maskCol.size.x, maskCol.size.y);
        Rect outerRect = new Rect(col.GetCornerVertices()[3].x + transform.localPosition.x, col.GetCornerVertices()[3].y + transform.localPosition.y, col.size.x, col.size.y);

        if (outerRect.xMin < innerRect.xMin && outerRect.xMax > innerRect.xMax &&
            outerRect.yMin < innerRect.yMin && outerRect.yMax > innerRect.yMax)
        {
            outOfBounds = OutOfBoundsDirection.Inbounds;
        }
        else
        {
            if ((outOfBounds &= OutOfBoundsDirection.Inbounds) == OutOfBoundsDirection.Inbounds)
            {
                outOfBounds ^= OutOfBoundsDirection.Inbounds;
            }

            if (outerRect.xMin > innerRect.xMin)
            {
                outOfBounds |= OutOfBoundsDirection.Left;
            }

            if (outerRect.xMax < innerRect.xMax)
            {
                outOfBounds |= OutOfBoundsDirection.Right;
            }

            if (outerRect.yMin > innerRect.yMin)
            {
                outOfBounds |= OutOfBoundsDirection.Bottom;
            }

            if (outerRect.yMax < innerRect.yMax)
            {
                outOfBounds |= OutOfBoundsDirection.Top;
            }
            StartCoroutine(MoveInbounds());
        }
    }

    /// <summary>
    /// Translate the map to inside the bounds
    /// </summary>
    /// <returns></returns>
    private IEnumerator MoveInbounds()
    {
        if ((outOfBounds & OutOfBoundsDirection.Left) != 0)
        {
            transform.Translate(Vector3.left * snapBackSpeed);
        }
        if ((outOfBounds & OutOfBoundsDirection.Right) != 0)
        {
            transform.Translate(Vector3.right * snapBackSpeed);
        }

        if ((outOfBounds & OutOfBoundsDirection.Top) != 0)
        {
            transform.Translate(Vector3.up * snapBackSpeed);
        }

        if ((outOfBounds & OutOfBoundsDirection.Bottom) != 0)
        {
            transform.Translate(Vector3.down * snapBackSpeed);
        }
        yield return waitFrame;
        CheckBounds();
    }
}

BoxColliderExtensions.cs

using UnityEngine;

public static class BoxColliderExtensions
{
    /// <summary>
    /// Returns the local position of the four corner vertices of the collider
    /// </summary>
    /// <param name="col">Array containing the corner points, starting at the top right and going clockwise</param>
    /// <returns></returns>
    public static Vector2[] GetCornerVertices(this BoxCollider2D col)
    {
        Vector2[] verts = new Vector2[4];

        verts[0] = col.offset + new Vector2(-col.size.x, col.size.y) * 0.5f;
        verts[1] = col.offset + new Vector2(col.size.x, col.size.y) * 0.5f;
        verts[2] = col.offset + new Vector2(col.size.x, -col.size.y) * 0.5f;
        verts[3] = col.offset + new Vector2(-col.size.x, -col.size.y) * 0.5f;

        return verts;
    }
}

The code works smoothly. I've never used bitmasks before but thought this would be a nice application of it, since the map can be moved out of bounds in multiple directions at the same time (e.g. up & right). But this takes a lot of if checks to do, which I feel can be halved, as i'm pretty much checking the same thing twice in CheckBounds and MoveInBounds.

I'm not experiencing any performance issues with it, so don't need to micro optimize it. Mainly looking for readability improvements/getting rid of those double ifs.

\$\endgroup\$
  • 1
    \$\begingroup\$ For some reason, wherever you call GetCornerVertices(), you are only using its element with the index [3]; and then both its x and y fields. Without diving deep inside the logic, this asymmetry looks suspicious: it looks like you rely on the minimap being in one specific corner of the screen, or something like this. However, if there's a valid reason to only check a single vertex, there's no need to detect the rest. \$\endgroup\$ – IMil Aug 1 at 5:49
  • \$\begingroup\$ @IMil it is because of the way a rect is constructed. new Rect(float x, float y, float width, floath height. the vertex at index [3] is the bottom left vertex. Starting from this vertex means I can pass in its full width/height to get the total rectangle. If I used index [0] (top left) i'd have to pass in width/-height. As you say only that single index gets used here and could indeed make a function that returns just that specific vertex, hand't thought of that. Thanks! (If you add that as an answer I can upvote it) \$\endgroup\$ – remy_rm Aug 1 at 6:25
  • \$\begingroup\$ Not a code review, but I find the re-positioning really bad UX (what if I want to find distance to a known landmark that is just outside of the bounds?). Better to provide a sort of "home" button to go back to the initial position. \$\endgroup\$ – Hosch250 Aug 1 at 13:29
10
\$\begingroup\$

Readability

Mainly looking for readability improvements/getting rid of those double ifs.

Since you are considered about readability, I will focus on that part.


Declare a flags enum on multiple lines and prefer to use the bit shift to indicate its location. Use an unsigned type as underlying type. Have a value corresponding to 0, because that is the default of any enum.

[Flags]
private enum OutOfBoundsDirection { Inbounds = 1, Left = 2, Right = 4 // ..
[Flags]
private enum OutOfBoundsDirection : uint
{ 
    None     = 0
    Inbounds = 1 << 0, 
    Left     = 1 << 1, 
    Right    = 1 << 2, 
    Top      = 1 << 3, 
    Bottom   = 1 << 4
}

Don't inline annotations.

 [SerializeField] private GameObject mainCanvas;
[SerializeField] 
private GameObject mainCanvas;

Use early exit when you want to avoid nested if statements.

if (outerRect.xMin < innerRect.xMin && outerRect.xMax > innerRect.xMax &&
    outerRect.yMin < innerRect.yMin && outerRect.yMax > innerRect.yMax)
{
    outOfBounds = OutOfBoundsDirection.Inbounds;
}
else
{
    if ((outOfBounds &= OutOfBoundsDirection.Inbounds) == OutOfBoundsDirection.Inbounds)
    {
        outOfBounds ^= OutOfBoundsDirection.Inbounds;
    }
    // .. CR: code omitted for brevity
}
if (outerRect.xMin < innerRect.xMin && outerRect.xMax > innerRect.xMax &&
    outerRect.yMin < innerRect.yMin && outerRect.yMax > innerRect.yMax)
{
    outOfBounds = OutOfBoundsDirection.Inbounds;
    return;
}
if ((outOfBounds &= OutOfBoundsDirection.Inbounds) == OutOfBoundsDirection.Inbounds)
{
     outOfBounds ^= OutOfBoundsDirection.Inbounds;
}
// .. CR: code omitted for brevity

Use HasFlag to perform the bit check for you.

if ((outOfBounds & OutOfBoundsDirection.Left) != 0)
if ((outOfBounds.HasFlag(OutOfBoundsDirection.Left))

Create constants where you can.

verts[0] = col.offset + new Vector2(-col.size.x, col.size.y) * 0.5f;
verts[1] = col.offset + new Vector2(col.size.x, col.size.y) * 0.5f;
verts[2] = col.offset + new Vector2(col.size.x, -col.size.y) * 0.5f;
verts[3] = col.offset + new Vector2(-col.size.x, -col.size.y) * 0.5f;
const float factor = 0.5f;

verts[0] = col.offset + new Vector2(-col.size.x, col.size.y) * factor;
verts[1] = col.offset + new Vector2(col.size.x, col.size.y) * factor;
verts[2] = col.offset + new Vector2(col.size.x, -col.size.y) * factor;
verts[3] = col.offset + new Vector2(-col.size.x, -col.size.y) * factor;
\$\endgroup\$
  • \$\begingroup\$ Thank you for the suggestions! Is there a technical reason why a bit shift is better when declaring the enum? To me it looks likes that would be less readable on bigger enums. \$\endgroup\$ – remy_rm Jul 31 at 11:36
  • 4
    \$\begingroup\$ No, only for readability :) But you might not agree with this. Many developers show the bit location in the declaration. \$\endgroup\$ – dfhwze Jul 31 at 11:38
  • \$\begingroup\$ Is it considered bad style to inline annotations? I found it useful especially in Unity projects where [SerializeField] private is used frequently \$\endgroup\$ – sonny Jul 31 at 15:21
  • 1
    \$\begingroup\$ @sonny To some point, style preferences are subjective. If you like this style, use it :) Just be consistent in what you pick. \$\endgroup\$ – dfhwze Jul 31 at 17:09
  • 2
    \$\begingroup\$ While using constants is a reasonable suggestion, in this case replacing 0.5 with "factor" is hardly an improvement because there's no indication what this "factor" means. It would make a lot more sense to introduce two variables: "double halfWidth = col.size.x * 0.5" and its counterpart "halfHeight". Then there'll be no need to replace 0.5 with a constant, because its meaning will be obvious from the variable name. \$\endgroup\$ – IMil Aug 1 at 5:51
9
\$\begingroup\$

This

    if (outerRect.xMin < innerRect.xMin && outerRect.xMax > innerRect.xMax &&
        outerRect.yMin < innerRect.yMin && outerRect.yMax > innerRect.yMax)

and this:

        if (outerRect.xMin > innerRect.xMin)
        {
            outOfBounds |= OutOfBoundsDirection.Left;
        }

        if (outerRect.xMax < innerRect.xMax)
        {
            outOfBounds |= OutOfBoundsDirection.Right;
        }

        if (outerRect.yMin > innerRect.yMin)
        {
            outOfBounds |= OutOfBoundsDirection.Bottom;
        }

        if (outerRect.yMax < innerRect.yMax)
        {
            outOfBounds |= OutOfBoundsDirection.Top;
        }

are candidates as extension methods:

static class Extensions
{
  public static bool Contains(this Rect outerRect, Rect innerRect)
  {
    return outerRect.xMin < innerRect.xMin && outerRect.xMax > innerRect.xMax &&
             outerRect.yMin < innerRect.yMin && outerRect.yMax > innerRect.yMax;
  }

  public static OutOfBoundsDirection Overlaps(this Rect outerRect, Rect innerRect)
  {
    OutOfBoundsDirection outOfBounds = OutOfBoundsDirection.None;

    if (outerRect.xMin > innerRect.xMin)
    {
       outOfBounds |= OutOfBoundsDirection.Left;
    }

    if (outerRect.xMax < innerRect.xMax)
    {
      outOfBounds |= OutOfBoundsDirection.Right;
    }

    if (outerRect.yMin > innerRect.yMin)
    {
      outOfBounds |= OutOfBoundsDirection.Bottom;
    }

    if (outerRect.yMax < innerRect.yMax)
    {
      outOfBounds |= OutOfBoundsDirection.Top;
    }

    return outOfBounds;
  }
}

I'm not a fan of the OutOfBoundsDirection.Inbounds flag, because it can lead to invalid states as:

OutOfBoundsDirection bounds = OutOfBoundsDirection.Inbounds | OutOfBoundsDirection.Left;

    if (outerRect.xMin > innerRect.xMin)
    {
       outOfBounds |= OutOfBoundsDirection.Left;
    }

What if outOfBounds has the flag Left set before this check, then it's still set even if outerRect.xMin <= innerRect.xMin? Or maybe you reset outOfBounds elsewhere?

Maybe the first check:

        if ((outOfBounds &= OutOfBoundsDirection.Inbounds) == OutOfBoundsDirection.Inbounds)
        {
            outOfBounds ^= OutOfBoundsDirection.Inbounds;
        }

should just be:

outOfBounds = OutOfBoundsDirection.None;
\$\endgroup\$
  • 1
    \$\begingroup\$ I was just thinking exactly the same about Inbounds. \$\endgroup\$ – t3chb0t Jul 31 at 12:53
  • 2
    \$\begingroup\$ I had not thought of putting them into extension methods. It would look a lot cleaner indeed. Initially I didn't have a .None flag and after adding it didn't realise I can just use that now to set outOfBounds = OutOfBoundsDirection.None instead of the if check. Appreciate your feedback! \$\endgroup\$ – remy_rm Jul 31 at 12:56

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.