Uploaded image for project: 'Request For Comments'
  1. Request For Comments
  2. RFC-324

Change Box constructors to not invert points

    Details

    • Type: RFC
    • Status: Adopted
    • Resolution: Unresolved
    • Component/s: DM
    • Labels:
      None

      Description

      Box's two main constructors look like this:

          /**
           *  @brief Construct a box from its minimum and maximum points.
           *
           *  @param[in] minimum   Minimum (lower left) coordinate (inclusive).
           *  @param[in] maximum   Maximum (upper right) coordinate (inclusive).
           *  @param[in] invert    If true (default), swap the minimum and maximum coordinates if
           *                       minimum > maximum instead of creating an empty box.
           */
          Box2I(Point2I const & minimum, Point2I const & maximum, bool invert=true);
       
          /**
           *  @brief Construct a box from its minimum point and dimensions.
           *
           *  @param[in] minimum    Minimum (lower left) coordinate.
           *  @param[in] dimensions Box dimensions.  If either dimension coordinate is 0, the box will be empty.
           *  @param[in] invert     If true (default), invert any negative dimensions instead of creating
           *                        an empty box.
           */
          Box2I(Point2I const & minimum, Extent2I const & dimensions, bool invert=true);
      

      Because an empty Box2I is defined is one with minimum > maximum (because minimum==maximum is a one-pixel box), one cannot round-trip a Box2I through its minimum and maximum points without explicitly setting invert=False.

      This is a source of confusion and the behavior invert=True was probably too clever to be a good default, and I'd like to make the default False remove the invert option entirely.

      (The same should be done for Box2D.)

        Attachments

          Issue Links

            Activity

            Hide
            Parejkoj John Parejko added a comment -

            A hearty +1, seeing as I just got burned by this when dealing with empty bboxes in BoundedFields.

            Show
            Parejkoj John Parejko added a comment - A hearty +1, seeing as I just got burned by this when dealing with empty bboxes in BoundedFields.
            Hide
            rowen Russell Owen added a comment - - edited

            +1 though I would prefer getting rid of the invert flag entirely; it seems unnecessary in both constructors and also very confusing in the the (minimum, dimension, invert) constructor.

            Show
            rowen Russell Owen added a comment - - edited +1 though I would prefer getting rid of the invert flag entirely; it seems unnecessary in both constructors and also very confusing in the the (minimum, dimension, invert) constructor.
            Hide
            jbosch Jim Bosch added a comment -

            Adopted as-is. Russell Owen may be right that we don't even need this option any more, but I don't think it does any harm if it isn't the default, and leaving it will make the migration easier (since the old behavior will at least still be possible to get).

            Show
            jbosch Jim Bosch added a comment - Adopted as-is. Russell Owen may be right that we don't even need this option any more, but I don't think it does any harm if it isn't the default, and leaving it will make the migration easier (since the old behavior will at least still be possible to get).
            Hide
            jbosch Jim Bosch added a comment -

            Discussions with Krzysztof Findeisen on DM-14960 have convinced me that Russell Owen was right on this - it's better if we just remove invert entirely, as retaining it with a default of false can still do some harm.  I'm reopening this and giving it another week to stew with that as the proposed outcome.

            Show
            jbosch Jim Bosch added a comment - Discussions with Krzysztof Findeisen on DM-14960 have convinced me that Russell Owen was right on this - it's better if we just remove invert entirely, as retaining it with a default of false can still do some harm.  I'm reopening this and giving it another week to stew with that as the proposed outcome.
            Hide
            krzys Krzysztof Findeisen added a comment -

            I guess I have to comment, now. I have no problem with either removing invert or changing the default, as long as it can be implemented quickly.

            Are Box2D's constructors included in the scope of this RFC? They have identical signatures (aside from Extent2I -> Extent2D), and it would be best if any decision preserved that.

            Show
            krzys Krzysztof Findeisen added a comment - I guess I have to comment, now. I have no problem with either removing invert or changing the default, as long as it can be implemented quickly. Are Box2D 's constructors included in the scope of this RFC? They have identical signatures (aside from Extent2I -> Extent2D ), and it would be best if any decision preserved that.
            Hide
            jbosch Jim Bosch added a comment -

            Yes, Box2D too, good catch; now fixed.  I hadn't noticed (at least recently) that this RFC (as previously written) applied to only Box2I.

            Show
            jbosch Jim Bosch added a comment - Yes, Box2D too, good catch; now fixed.  I hadn't noticed (at least recently) that this RFC (as previously written) applied to only Box2I.
            Hide
            tjenness Tim Jenness added a comment -

            Jim Bosch is this RFC now ready for adoption?

            Show
            tjenness Tim Jenness added a comment - Jim Bosch is this RFC now ready for adoption?
            Hide
            jbosch Jim Bosch added a comment -

            Re-adopting with the amendment to remove the invert option instead of changing its default.

            Show
            jbosch Jim Bosch added a comment - Re-adopting with the amendment to remove the invert option instead of changing its default.

              People

              • Assignee:
                jbosch Jim Bosch
                Reporter:
                jbosch Jim Bosch
                Watchers:
                Jim Bosch, John Parejko, John Swinbank, Krzysztof Findeisen, Russell Owen, Tim Jenness
              • Votes:
                0 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Planned End:

                  Summary Panel