xBimTeam / XbimGeometry

XbimGeometry contains the CLR interop libraries and the c++ engine used to compute the 3D geometry of models.
https://xbimteam.github.io/
Other
260 stars 132 forks source link

Crash in BRepBuilderAPI_MakeFace and potential fix #334

Closed jomijomi closed 3 years ago

jomijomi commented 3 years ago

Using XbimGeometry master as of 2021-04-28 (v5.1.407) I get a crash in BRepBuilderAPI_MakeFace when loading the attached file. As far as I’ve been able to go to the bottom of this, it seems like one of the profiles (70907) has a very tiny area and then BRepBuilderAPI_MakeFace crashes because of this. I’ve used the same file in a previous version (from 2020-04-07) and then this tiny area is detected, i.e. “Face cannot be built with a profile that has no area”. The thing is that this specific part of the code in XbimFace.cpp is identical in both versions, basically:

XbimVector3D n = wire->Normal;
if (n.IsInvalid()) //it is not an area
{
    XbimGeometryCreator::LogWarning(logger, profile, "Face cannot be built with a profile that has no area.");
    return;
}

So, it seems that in the older version we get an invalid normal, and can then skip this face. However, in the newer version (current) the normal isn’t invalid. So, my suggestion is that we also check wire->Area and then when below some threshold we consider the face to have zero area. In my tests I’ve successfully used 1E-9, but it’s possible that we have to be more sophisticated and base that on tolerance, etc. As to why we get a different, non-invalid normal in the newer version (current) I haven’t yet figured that out… So, something like this:

//It might not be sufficient to only check normal. If we get a very tiny area BRepBuilderAPI_MakeFace (below) will crash!!!
//Solution: also check wire->Area. What is a good reference/threshold for the area??
const double areaThreshold = 1e-9;
double area = wire->Area;
if (n.IsInvalid() || (area < areaThreshold)) //it is not an area
{
    XbimGeometryCreator::LogWarning(logger, profile, "Face cannot be built with a profile that has no (or very small) area. A = {0}", area);
    return;
}

I’ve made these (small) changes in XbimFace.cpp (attached). Search for “DALMAN” in the file. Also, this file has the changes made in #285 (and #288). Again, sorry for no real PR, I really have to learn how to do that...

Geometry_with_small_area_face.ifc.stripped.zip

XbimFace_21-04-29.zip

/Mikael

CBenghi commented 3 years ago

Hi @jomijomi, send me an email and I'll show you how to create a PR... it's super simple. We'll get it done in 20 minutes. (teach a man to fish...) ;-) Claudio

CBenghi commented 3 years ago

Hi again, I've had a look at the file with the latest develop branch and this is what I see:

image

Is this fixed?

jomijomi commented 3 years ago

Yes, it's fixed. Great! Then it means that your latest changes basically does the same and make sure that the small area/normal error gets detected. By the way, this is also a SimpleBIM processed file. I see that you get a lot of warnings (that comes from the tolerances issues?). I am looking into this in more detail and will soon have access to files pre/post SimpleBIM, but it seems like the problem is that from Revit the file is originally in millimeters (and then the 0.01 mm tolerance works), but then the geometry in the file is converted to meters, but the tolerance is still the same (0.01, but now in meters...) /Mikael

CBenghi commented 3 years ago

Thanks @jomijomi, that makes sense. I've tried to get in touch with Simplebim this morning to se if I can advise them on fixing this. Feel free to close if this is ok.

jomijomi commented 3 years ago

Yes! I'll close it! Let's keep in touch about the SimpleBIM issue. And also about me learning how to do PRs :)