Fix `BoundBox.is_inside`

[jdegenstein]

The old comparison was not working correctly per issue #827 so I fixed that and added a test that returns False too. This fixes issue #827.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 96.09%. Comparing base (fd5515d) to head (4c53117). Report is 1 commits behind head on dev.

Additional details and impacted files

@@           Coverage Diff           @@
##              dev     #828   +/-   ##
=======================================
  Coverage   96.09%   96.09%           
=======================================
  Files          24       24           
  Lines        8493     8493           
=======================================
  Hits         8161     8161           
  Misses        332      332           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[snoyer]

is "A is inside B" supposed to mean "B fully encloses A" or "A and B are overlapping"?

edit:

https://github.com/gumyr/build123d/blob/c0728e08036369e34ca479f3139a5ea6f8abb308/src/build123d/geometry.py#L1004-L1005

https://github.com/gumyr/build123d/blob/c0728e08036369e34ca479f3139a5ea6f8abb308/src/build123d/topology.py#L1350-L1351

https://github.com/gumyr/build123d/blob/c0728e08036369e34ca479f3139a5ea6f8abb308/src/build123d/topology.py#L6790-L6793

so basically self.is_inside(other) is supposed to be read as "Other is inside of Self" ? Wouldn't it be more intuitive to call these methods self.contains(other) ?

The bounding box method does look more like an overlap test than a is_inside/contains test. Maybe you need 2 methods:

• bbox1.is_inside(bbox2) returning whether bbox1 is fully inside bbox2
• bbox1.overlaps(bbox2) returning whether one box contains any part of the other

[jdegenstein]

@snoyer Putting aside the other is_inside methods, you are correct that BoundBox.is_inside in this PR is really talking about overlapping bounding boxes. I think that may have been what was intended with the current version in dev as well, but it is hard to tell as it is not working correctly per the referenced issue. I was thinking that this method could be enhanced with some different flags, but I now think you are right that it makes more sense to simply have different methods.

With that in mind I agree that the current implementation in this PR should be renamed to BoundBox.overlaps and that a new implementation of BoundBox.contains is created that represents e.g. bboxA fully contains bboxB -> bboxA.contains(bboxB). The current docstring reads to me like "bboxB is inside bboxA" -> bboxA.is_inside(bboxB) which does not make much sense to me because the ordering is backwards. With this in mind I think BoundBox.is_inside should be fixed and renamed to BoundBox.contains and BoundBox.is_inside should be deleted.

EDIT: New proposed behavior

bboxA.overlaps(bboxB) == bboxB.overlaps(bboxA) #i.e. they are always equivalent
bboxA.contains(bboxB) != bboxB.contains(bboxA) #i.e. they are never equivalent even if bboxA and bboxB are coincident 

I am open to input on the latter behavior as this is the most strict implementation.

[snoyer]

@jdegenstein

bboxA.contains(bboxB) != bboxB.contains(bboxA) #i.e. they are never equivalent even if bboxA and bboxB are coincident 

Assuming you mean bboxA.contains(bboxA) == False, saying "a box does not contain itself" may be tempting from a natural language point-of-view but wouldn't it also implies "a box does not contain its own corners" when implemented? IMO it would make sense to use inclusive comparisons and have:

bbox(A,B).contains(bbox(A,B)) == True  # box contains itself
bbox(A,B).contains(bbox(A)) == True  # box contains [the box of] any of its points
bbox(A,B).contains(bbox(B)) == True

The same could be done for the overlapping to have:

bbox(A,B).overlaps(B,C) == True  # boxes have a common corner

[jdegenstein]

IMO it would make sense to use inclusive comparisons

@snoyer Maybe this should be controllable by a boolean flag? It can be useful to differentiate these assumptions in practice when e.g. writing lambdas for filtering purposes.

bboxA.contains(bboxA, inclusive=True) == True
bboxA.contains(bboxA, inclusive=False) == False

and also:

# when e.g. bboxB shares a corner, edge, or face with bboxA
bboxA.overlaps(bboxB, inclusive=True) == True
bboxA.overlaps(bboxB, inclusive=False) == False

Is there a better name for the flag than inclusive?

[snoyer]

Is there a better name for the flag than inclusive?

could flip the boolean value and call it strict? other than that I don't know but I didn't think about it that hard because I'm not a fan of these boolean flags either way.

If you really need to have both versions of contains (do you? bounding boxes would only be used for pre-checks and actual precise tests would be done on actual geometry) you could have separate contains() and contains_strictly() methods. The implementation would be cleaner (no branching in the code), the documentation would be easier to read (no "branching" in the doc string) and if the names are explicit enough the users won't even have to check it. Also compare the filtering usage:

filter(mybox.contains_strictly, other_boxes)

vs

filter(lambda b: mybox.contains(b, inclusive=False), other_boxes)

or

filter(partial(mybox.contains, inclusive=False), other_boxes)

Obviously this is all subjective API design choices and @gumyr's opinion would be more important than mine :)

[jdegenstein]

@snoyer Yeah I agree we need @gumyr to weigh in -- this PR discussion has grown a lot in scope from when it was simply trying to fix a broken method. I personally do not like having 4 separate methods for this, I prefer 2 methods with the flags with the inclusive name vs. strict although still open to other names.

I am convinced that having the effect of inclusive True/False is a useful distinction and should be available.

Regardless I am not sure anyone has even tried to use the existing method given the bug that I discovered -- so at some point it is better to just move on to more important issues and add these small fixes/enhancements.

[gumyr]

The current assumption in the code is that objects on the edge of another object are "inside" it. Internally these two can be separated:

solid_classifier.State() == ta.TopAbs_IN or solid_classifier.IsOnAFace()

but I didn't see the value in that. However, I'm happy to have them separate if that's helpful.

I like the proposed contains & overlaps change. Maybe contains could include on the edges and there could be another method to identify objects specifically on the edge or surface of another?

[snoyer]

Regardless of which one(s) you decide to implement in build123d you may want to pick the names according to existing standards/conventions if possible, for example: https://en.wikipedia.org/wiki/DE-9IM#Spatial_predicates, or, as per shapely:

• A.contains(B)

  Returns True if geometry B is completely inside geometry A.

  A contains B if no points of B lie in the exterior of A and at least one point of the interior of B lies in the interior of A.

• A.contains_properly(B)

  Returns True if geometry B is completely inside geometry A, with no common boundary points.

  A contains B properly if B intersects the interior of A but not the boundary (or exterior). This means that a geometry A does not "contain properly" itself, which contrasts with the contains function, where common points on the boundary are allowed.

• A.covered_by(B)

  Returns True if no point in geometry A is outside geometry B.

• A.covers(B)

  Returns True if no point in geometry B is outside geometry A.

• A.crosses(B)

  Returns True if A and B spatially cross.

  A crosses B if they have some but not all interior points in common, the intersection is one dimension less than the maximum dimension of A or B, and the intersection is not equal to either A or B.

• A.disjoint(B)

  Returns True if A and B do not share any point in space.

  Disjoint implies that overlaps, touches, within, and intersects are False.

• A.intersects(B)

  Returns True if A and B share any portion of space.

  Intersects implies that overlaps, touches, covers, or within are True.

• A.overlaps(B)

  Returns True if A and B spatially overlap.

  A and B overlap if they have some but not all points in common, have the same dimension, and the intersection of the interiors of the two geometries has the same dimension as the geometries themselves. That is, only polyons can overlap other polygons and only lines can overlap other lines.

  If either A or B are None, the output is always False.

• A.touches(B)

  Returns True if the only points shared between A and B are on the boundary of A and B.

• A.within(B)

  Returns True if geometry A is completely inside geometry B.

  A is within B if no points of A lie in the exterior of B and at least one point of the interior of A lies in the interior of B.

[nikandfor]

I like is_inside signature and it should return True for x.is_inside(x) because, say, you have a structure, you have bounding box for it, and then you iterate over some small pieces and check if it belongs to the structure. It would be really strange to miss those that touch the bounds. The code would look like

for piece in pieces:
    if piece.is_inside(box): # and I like having piece on the left as it reads naturally: "piece is inside box"
        # do something

It shouldn't return True for intersections as it's the other case.

In any case that's not such a hard problem, we should decide and fix it soon.