<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Mar 11, 2019 at 11:33 AM Matt Brown <<a href="mailto:matt.brown@kitware.com">matt.brown@kitware.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div></div>I'm not sure what you mean by "properly encodes empty".  The box does encode empty by having at least one max point less then its min point.  There is a function isEmpty() that checks if a box is empty.  I've used bound box classes in several libraries that all work this way.  It is the user's responsibility to check if a box is empty (call isEmpty()) before doing any operations that require a valid box.  I'm not sure there is a better approach here, except maybe that the area/volume of an empty box should return 0.</div></blockquote><div><br></div><div>Ok, I suppose that makes sense. I'd say we should at least:</div><div><ul><li>Return zero for area/volume of an empty box. <br></li></ul></div></div></div></blockquote><div>Yes, probably, but it would be good to understand why Eigen doesn't do this.  It might just be for efficiency.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><ul><li>Add sufficient documentation to bounding_box to make it clear that inverting upper/left and lower/right represents "empty" because that wasn't particularly clear to me at the start.</li></ul></div></div></div></blockquote><div>If we just use the Eigen class directly we can refer to the existing documentation for that class</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><ul><li>Add an is_empty method to bounding_box because it is more efficient to set this at creation versus having someone explicitly check the different combos of min > max.</li></ul></div></div></div></blockquote><div>Or just use the Eigen class directly so we don't have to keep exposing more underlying functions.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><div>Optionally,<br></div><ul><li>Does it also make sense to raise an error if you try to access upper_left() or lower_right() for an empty box as I can't think of a use case where you would want to know what that values are there other than to check is_empty().</li></ul></div></div></div></blockquote><div>Probably not.  This should be simple, fast accessors.  We probably don't want to overhead of checking is_empty for each access.  It is the user's responsibility to know if the box is empty.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><div>If someone wants to warp bounding boxes to another coordinate system that may have the axes inverted, do we assume it is the implementer's responsibility to know that inverting an axis will make the bounding box represent empty?<br></div></div></div></div></blockquote><div><br></div><div>You should not hit this case.  This is a bounding box, not a polygon.  You should warp the corners of this box (or other data within) and then compute a new bounding box on the warped points.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><div></div><div><br></div><div>Best,</div><div>Matt<br></div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div> </div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Mar 11, 2019 at 10:46 AM Matt Brown <<a href="mailto:matt.brown@kitware.com" target="_blank">matt.brown@kitware.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">intersection() could throw a warning that one of the boxes is empty, but it should still return 0 intersection area, I would think. </div></blockquote><div>The issue is, intersection() returns a bounding box, not an area. So there isn't a clear ideal way to return a bounding box that properly encodes "empty." Right now, it returns a bounding box with all properties populated in an undefined manner.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div></div><div>Is there a reason to utilize bounding_box class to also support boxes that are not axis aligned, which is all Eigen::AlignBox supports?</div></div></blockquote><div>I think non-axis-aligned bounding boxes might have utility in the future, but bounding_box is currently configured to only specify axis-aligned boxes. So, we would have to completely redefine the interface to bounding_box to support non-axis-aligned boxes.</div><div><br></div><div>~Matt</div></div></div>
</blockquote></div>
</blockquote></div><br clear="all"><br>-- <br><div dir="ltr" class="gmail-m_-2016168000472913788gmail_signature"><div dir="ltr"><div><div><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr"><span style="color:rgb(0,0,0)"><span>Matt Brown</span>, Ph.D.<br></span><div style="font-size:small" dir="ltr"><span style="color:rgb(0,0,0)">Staff R&D Engineer</span></div><div dir="ltr"><span style="color:rgb(0,0,0)">Kitware, Inc.<br>101 E Weaver St. Suite G4</span></div><div dir="ltr"><span style="color:rgb(0,0,0)">Carrboro, NC 27510<br>(908) 910-3385</span></div></div></div></div></div></div></div></div></div></div></div>
</blockquote></div></div>