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
255 stars 128 forks source link

StackOverflowException when trying to create geometry #445

Open ChernyshevDS opened 1 year ago

ChernyshevDS commented 1 year ago

Hello. I've got a problem loading a specific IFC model (which I can not share, unfortunately): calling Xbim3DModelContext.CreateContext has thrown a StackOverflowException. In attempt to find the cause I've built a debug version of Xbim.Geometry.Engine, but the StackOverflowException disappeared and regular exception appeared instead.

I was able to find a place where it was generated - it was Xbim.Geometry.Engine.XbimCompound.cpp in method TopoDS_Shape XbimCompound::InitFaces(IEnumerable<IIfcFace^>^ ifcFaces, IIfcRepresentationItem^ theItem, ILogger^ logger). Here is the fragment (comment was there, line 1531):

try
{
    gp_Dir outerNormal = XbimWire::NormalDir(outerLoop); //this can throw an exception if the wire is nonsense (line) and should just be dropped

    ...
    <more code>
}
catch (const std::exception&)
{
    XbimGeometryCreator::LogDebug(logger, ifcFace, "Outer loop is not a bounded area,  face ignored");
    continue;
}

Problem is that XbimWire::NormalDir did not throw an std::exception, but Standard_ConstructionError from OpenCascade which is not a descendant of std::exception. The exception was caused by the attempt to construct an instance of gp_Dir class (direction vector) with all arguments set to zero. Clearly there is an issue with geometry in my model, but I'd prefer to open a model with some errors than not be able to open it at all.

The comment in the fragment above indicates that this situation is not unexpected, so I guess it'd be fine to just add another catch(Standard_Failure) clause with the same log-and-continue behaviour.

UPD: It seems that XbimWire::NormalDir behaves differently in Debug and Release mode (see comment in XbimWire.h, line 81 on branch XbimGeometry_develop_5.1.666), so the StackOverflowException was still an issue in Release mode. I had to add a check for NaN values in outerNormal like this:

try
{
    gp_Dir outerNormal = XbimWire::NormalDir(outerLoop); //this can throw an exception if the wire is nonsense (line) and should just be dropped
    if (XbimConvert::IsInvalid(outerNormal, 0))
    {
        XbimGeometryCreator::LogDebug(logger, ifcFace, "Outer loop normal could not be found,  face ignored");
        continue;
    }

    ...
}
catch (Standard_Failure sf)
{
    XbimGeometryCreator::LogDebug(logger, ifcFace, "Outer loop normal could not be found,  face ignored");
    continue;
}
catch (const std::exception&)
{
    XbimGeometryCreator::LogDebug(logger, ifcFace, "Outer loop is not a bounded area,  face ignored");
    continue;
}

XbimConvert::IsInvalid is picked from XbimGeometry_develop_5.1.666 branch. So far, this fix has allowed me to finally open my model.

Assemblies and versions affected:

XbimGeometry 5.1.403, commit 24a3db9442b33578eebf9fbf2eb58074cc3afc56

andyward commented 1 year ago

It would be great if you can supply a test for this. I know you can't share the model, but if you know the element triggering the issue, you can use the Simplify capability in Xplorer (View->Developer Windows->IFC Stripping) to extract the single element to a new model.

image

ChernyshevDS commented 1 year ago

@andyward Thanks for the reply. I didn't know that Xplorer has this feature, that's cool. I finally managed to extract the problematic element, and I'd say that was not as easy as I thought. It seems that the problem is not with the element itself, but with associated opening in it, so i had to include the corresponding IFCRELVOIDSELEMENT. Anyway, the stripped file is in the attachment. 01-23-01_main deck_supports under the deck.ifc.stripped.ifc.txt

ChernyshevDS commented 1 year ago

I'd like to share some of my findings, may be they will help. I tried to find out why XbimWire::NormalDir throws an exception. The method accepts the wire and iterates over its vertices to calculate the normal. I've noticed that iterating with BRepTools_WireExplorer returns only 2 vertices while the wire object is built with IfcPolyLoop with 3 vertices, but my cpp-fu is't strong enough to debug the WireExplorer itself.

ChernyshevDS commented 1 year ago

I guess, I've found the source of my problem: incorrect model 🤦‍♂️ I've bumped into this discussion: https://github.com/xBimTeam/XbimGeometry/issues/281 and figured that the fix I've made was already implemented in a dev branch in 2021, but never made it into master branch, which was a base for my own fork of Xbim.

Another important thing I've got from this discussion is that model precision parameter defined in IfcGeometricRepresentationContext is crucial for correct mesh generation and it may be incorrectly set by some IFC exporters. I was struggling with some mesh artifacts for a long time, and decreasing this parameter has immediately fixed them. My models were generated using ProStructures v10, and I'm not sure if this is a bug or just incorrect setting somewhere.

andyward commented 1 year ago

Definitely worth checking out the v6 GE (netcore branch) if you have problematic models - it has quite a few fixes around precision. As you noticed it's key to a lot of the geometry issues. It's also on a later 7.7 OpenCascade version. It's also a lot easier to work with as OCC imported via nuget rather than compiled in.

ChernyshevDS commented 1 year ago

Thanks, I should try it. I've chosen the 5.1.430 version a long time ago (don't remember exactly why, I can recall some issues with dependencies), and I decided to make my own fork and patch it as needed. I seems a bit strange to me that fixes are received into dev branch, but master stays abandoned for 2 years. As for netcore branch I'm a bit hesitant to switch on using a dev branch as I'm not sure if it's stable enough.

jomijomi commented 1 year ago

Yeah, regarding precision I think it is good practice to add a check before calling CreateContext. Just in case...

//Make sure we get at least 1/100 mm
//(mf is model.ModelFactors)
double sufficientPrecision = mf.OneMilliMetre / 100.0;
if(mf.Precision > sufficientPrecision)
{
           mf.Precision = sufficientPrecision;
}
ChernyshevDS commented 1 year ago

@jomijomi I'm not sure that this is a correct approach. I guess, it may introduce some errors when loading georeferenced models which typically have large coordinates. @andyward maybe you have some insight, why can't I set the precision to 1 nanometer and be done with it? What are the consequences?

andyward commented 1 year ago

@martin1cerny or @SteveLockley is probably a better authority on this than me - especially on WCS & geo-referencing.

But having a precision of nanometers when dealing with large coordinates is going to create all kinds of issues with Float/Double precision?

jomijomi commented 1 year ago

Yes, the ideal situation is of course to respect the precision/accuracy from the BIM-modelling or processing software. On that I fully agree!! However, I’ve noticed that I tend to encounter far more IFCs with obvious wrong precision/tolerance than I should, so this has almost been the standard setting in my app… Should state that this is mostly for housing/building projects, and not infrastructure.