BHoM / BHoM_Engine

Internal manipulation of the BHoM
GNU Lesser General Public License v3.0
26 stars 12 forks source link

Analytical_Engine: IsContaining potential bug #3326

Open peterjamesnugent opened 3 months ago

peterjamesnugent commented 3 months ago

Description:

A Point outside of the Region is returning true when it's close to 17m away (tolerance set to 0.25m).

Steps to reproduce:

See test script.

Expected behaviour:

Point will return false when fed in to IsContaining.

Test file(s):

#3326-IsContainingBug

peterjamesnugent commented 3 months ago

I think we (following a call with @pawelbaran) have found the cause of the error here: https://github.com/BHoM/BHoM_Engine/blob/57f9eb731191d3ed7ff07946b27148634601e449/Geometry_Engine/Query/IsContaining.cs#L527

Essentially, the tolerance in the test script is 0.25m (this is to account for thickness of the room) but this tolerance gets used all the way through the script - specifically in the line where it works out intersections. This causes the error because it picks more intersections than it is expecting (which is used further down in the method).

This raises a bigger question in the Geometry_Engine on whether we have different tolerances, e.g. here there would be an intersection tolerance (or general tolerance for geometry methods e.g. 1E-6) and a distance tolerance (which would be 0.25m). In this example the point in question is >16m away so should not appear to be contained within the outline.

@al-fisher, @adecler, @IsakNaslundBh is it worth a wider discussion on tolerances and whether we have several?

In terms of this specific issue, there is a workaround whereby:

  1. Filter the points using IsContaining() and the tolerance chosen (e.g. 0.25m);
  2. Project points (including some that are outside the Perimeter) to the Plane of the outline;
  3. Set the tolerance for IsContaining() as default and rerun the method.
IsakNaslundBh commented 3 months ago

Good catch @peterjamesnugent and above all make sense I think. Agree we probably need a discussion about the above, especially for this type of method where the tolerance has a more distinct meaning rather than just being a help to deal with numerical issues arising from dealing with floating point computations.

Not sure if this is over simplifying the issue, but wonder if it can be distinguished between something like:

Given the pain tolerances has been proving to be so far this will most likely not work in some other specific case, but might be a worthwhile addition to fix this particular case to start with and could potentially be used on other methods as well. Have not had a to deep think about this thought, and most likely need discussion/investigation before implementing, but might be a starting point at least.