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
+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.
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).
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.
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.
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. Yes, Box2D too, good catch; now fixed. I hadn't noticed (at least recently) that this RFC (as previously written) applied to only Box2I.
Jim Bosch is this RFC now ready for adoption?
A hearty +1, seeing as I just got burned by this when dealing with empty bboxes in BoundedFields.