# Change Box constructors to not invert points

XMLWordPrintable

## Details

• Type: RFC
• Resolution: Unresolved
• Component/s:
• 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.)

## Activity

Hide
John Parejko added a comment -

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

Show
John Parejko added a comment - A hearty +1, seeing as I just got burned by this when dealing with empty bboxes in BoundedFields.
Hide
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
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
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
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
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
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
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
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
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
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
Tim Jenness added a comment -

Show
Hide
Jim Bosch added a comment -

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

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

## People

• Assignee:
Jim Bosch
Reporter:
Jim Bosch
Watchers:
Jim Bosch, John Parejko, John Swinbank, Krzysztof Findeisen, Russell Owen, Tim Jenness