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
259 stars 131 forks source link

Stack overflow when creating geometry context with certain IFC file #281

Open idars opened 4 years ago

idars commented 4 years ago

I have a certain IFC file that's proving to be a bit problematic. When creating the 3D model context (context.CreateContext()), the program stops by itself with no unmanaged exceptions to catch in Visual Studio, and no seemingly fatal errors in the log:

C:\Test\bin\Debug\net47\Test.exe (process 25644) exited with code -1073741819.

Verbose log

Using xBIM Xplorer from the develop branch of XbimWindowsUI, the program crashes while creating the geometry. Analyzing it with ProcDump, the following exceptions are shown, leading up to a stack overflow:

[16:01:06] Exception: E06D7363.PAVEEFileLoadException@@
[16:01:06] Exception: E06D7363.msc
[16:01:06] Exception: E06D7363.PAVEETypeLoadException@@
[16:01:06] Exception: E06D7363.msc
[16:01:06] Exception: E06D7363.PAVEETypeLoadException@@
[16:01:06] Exception: E06D7363.msc
[16:01:07] Exception: E0434F4E.CON
[16:01:07] Exception: E0434F4E.CON
[16:01:07] Exception: E0434F4E.CON
[16:01:07] Exception: E0434F4E.CON
[16:01:07] Exception: E0434F4E.CON
[16:01:07] Exception: E0434F4E.CON
[16:01:07] Exception: E0434F4E.CON
[16:01:07] Exception: E0434F4E.CON
[16:01:07] Exception: E0434F4E.CON
[16:01:07] Exception: E0434F4E.CON
[16:01:07] Exception: E0434F4E.CON
[16:01:07] Exception: E0434F4E.CON
[16:01:07] Exception: E0434F4E.CON
[16:01:07] Exception: C00000FD.STACK_OVERFLOW

Dump file (expires in 1 week)

Only the newer NuGet-packages of Xbim.Geometry end up crashing/stopping, as the IFC file in question can be opened with Xbim.Geometry 5.1.288 as well as xBIM Xplorer from the master branch.

Assemblies and versions affected:

Xbim.Geometry 5.1.317 Xbim.Geometry 5.1.328

xbimxplorer-version

Steps (or code) to reproduce the issue:

Run https://github.com/xBimTeam/XbimWindowsUI/tree/develop and open the file below.

Minimal file to reproduce the issue:

ProblemFile.zip

andyward commented 4 years ago

Looks to be triggering a StackoverflowException

SOLAR-COMPUTER commented 3 years ago

Any progress on this issue? We are experiencing the same issue with certain IFC files (stack overflow exception after calling CreateContext) with the current NuGet package. Older versions did not cause this issue. This is a major problem for us since our IFC plugin is based on the xBim geometry.

martin1cerny commented 3 years ago

Hi @SOLAR-COMPUTER , we hope to get to this issue soon.

CBenghi commented 3 years ago

@SteveLockley, I've isolated the element (one element) that produces the stack overflow. Minimal.zip

I'm also trying to fix it. Claudio

CBenghi commented 3 years ago

Since I was at it, I've tried updating to occt 7.5.2. https://github.com/xBimTeam/XbimGeometry/tree/feature/occt752

That didn't fix it, but it was worth a try. I'll go into the details tomorrow.

CBenghi commented 3 years ago

Hello @idars, sorry it's been a while, can you help me understand more about the file? It looks like the precision of the model is defined incorrectly.

#18 = IFCGEOMETRICREPRESENTATIONCONTEXT($, 'Model', 3, 1.E-2, #19, #21); should be #18 = IFCGEOMETRICREPRESENTATIONCONTEXT($, 'Model', 3, 1.E-4, #19, #21);

The problem happens because precision is set to 0.01 meters and there are points in the mesh that are closer than 1cm. While they should really be distinct, they get merged if we set the required precision according to the file.

I think this might be a change that SimpleBim made to the original Revit file. Can you confirm this for me?

If you change that line, the file reads the file with no problem...

image

I'll investigate ways to mitigate the issue, but conceptually this is a broken file.

Thanks, Claudio

CBenghi commented 3 years ago

I've pushed a fix that mitigates the problem and removes the stack overflow at https://github.com/xBimTeam/XbimGeometry/tree/feature/occt752

idars commented 3 years ago

Thanks for looking into this!

XbimXplorer does no longer crash after adjusting the precision as you mentioned. It also does not report any warnings or errors with the file (unlike with other versions of XbimXplorer). So that small change does it all, it seems.

I don't have the specific details about the origin of the IFC file. It may have been altered by SimpleBim, or be badly configured altogether, or some other reason I don't know of. I'll see if I can find an answer to that.

jomijomi commented 3 years ago

Hi! The core problem here is that one of the outer loops get a normal with NaN components and then faceMaker crash! I will give you a more detailed answer later today (with better formatting), but for now: Look in XbimCompound.cpp in the function : TopoDS_Shape XbimCompound::InitFaces(IEnumerable<IIfcFace^>^ ifcFaces, IIfcRepresentationItem^ theItem, ILogger^ logger)

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

Add this directly after that row: //DALMAN: Here we have the problem. When outerNormal is invalid (all or any component NaN), faceMaker will crash!!! //Perhaps we should also check to see if then normal is valid in the first place (i.e. outerNormal.length > tolerance) //https://stackoverflow.com/questions/15268864/how-to-compare-two-nan-values-in-c if (outerNormal.X() != outerNormal.X() || outerNormal.Y() != outerNormal.Y() || outerNormal.Z() != outerNormal.Z()) { XbimGeometryCreator::LogWarning(logger, ifcFace, "Outer loop has invalid normal (NaN components), face ignored"); continue; }

In the future we should perhaps add this check already in XbimWire::NormalDir and then zero the components (if we get NaN) and then we only need to check normal.isValid

Now, the core of this problem might very well be in how the normal is computed...

/Mikael

jomijomi commented 3 years ago

But, this may very well come from wrong precision/tolerance, as stated by Claudio

CBenghi commented 3 years ago

In the future we should perhaps add this check already in XbimWire::NormalDir and then zero the components (if we get NaN) and then we only need to check normal.isValid

That's what I've done in one of the latest commits. I'm still working on this, because the fix that I've added seem to be behaving differently in Debug/Release mode. I'll continue investigating this afternoon.

Claudio

CBenghi commented 3 years ago

Hello again, so the issue was harder to track down than expected, because OCC behaviour differs in release/debug mode under some circumstances.

24d2d6a45b4683f3b6b1580779fc68e0ec6651dc addresses the stack overflow problem on the minimal test case that we have produced.

It still has invalid boolean results that i need to investigate, but I'll continue working on it in the coming days because invalid files help test the boundaries of the engine and make it more stable.

jomijomi commented 3 years ago

@CBenghi while you're at it, perhaps you could take a quick look at #334. It's another one-line solution to a similar problem, also connected to the validity of normals. As a matter of fact, #334 might even be fixed by the same normal checks you just implemented so perhaps we don't even need to check the area. Also, I started to look into these problems recently when I got access to a lot of problematic files that had been processed with SimpleBIM. I will see if I can get a couple of examples pre/post SimpleBIM. It's definitely something strange going on with the tolerances...

Best, Mikael

jomijomi commented 3 years ago

I have now tested SimpleBIM and can confirm issues with tolerance/precision when applying the "Set Units" modification: https://simplebim.com/support/simplebim-8-template-guide-model.html#set-units I used the RVT ADV sample and when exported to IFC (from Revit) we get model units as millimeter and tolerance/precision as 0.01 (which then means 0.01 mm). After the fix in SimpleBIM we get model units as meter BUT tolerance/precision is still 0.01 (which then means 0.01 m). Conclusion: tolerance/precision is not updated to reflect the new model units. /Mikael

jomijomi commented 3 years ago

@CBenghi do you still get some booleans wrong in the complete file?? Have you tried with precision set to 1.E-5?? As the original file is from Revit and with model units in mm and precision set to 1.E-2, this should then correspond to 1.E-5 with model units in meters. Also, what Fuzzy factor are you using? I think the default is still 10, but I'm starting to see that we should probably lower that for IFCs from Revit. Tekla is still another beast, though... /Mikael

CBenghi commented 3 years ago

I've spoken to people from SimpleBIM and hopefully they will get this right in the coming version. Regardless, I will continue to progress this so that there are no crashes.

Failure of computing meshes might be reasonable, but we want the code to be tolerant to crashes. I'll keep you posted, I have many plates spinning at the moment, it might take time.

Janitum commented 2 years ago

@CBenghi I seem to stumble upon the exact same issue, not being able to handle the StackOverflowException in a graceful way.

Is there any progress on the subject?

jomijomi commented 2 years ago

@Janitum is this with a SimpleBIM processed file? Just to rule out that your problem isn't just the tolerance/precision..? /Mikael

jomijomi commented 2 years ago

A more generic solution to the "SimpleBIM tolerance/precision-problem" is to make sure that you always process the file with a sufficient precision. Do below check/modification just before you call CreateContext

//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;
}
ShaneBSitu commented 2 years ago

Hi, is there any update on this? I've been able to replicate this on an IFC exported directly from Revit. Attached is the minimal form of the file. The precision fix does not work for this file, the offending face is a genuinely degenerate case. It is however gracefully handled by this commit https://github.com/xBimTeam/XbimGeometry/commit/24d2d6a45b4683f3b6b1580779fc68e0ec6651dc .

The fix seems rather innocuous, so is there a reason this is still hanging in a branch?

Minimal Broken Case.zip

Janitum commented 2 years ago

Hi @jomijomi, The precision fix didn't work for me. @ShaneBSitu your Minimal Broken Case didn't seem broken to me with just the latest version of the GeometryEngine.

However my problem still persisted.. UNTIL.. I merged the suggested commit 24d2d6a into a new build!

@CBenghi @jomijomi Can it still be included in a next release?

Thanks!

Belrius commented 1 year ago

overflow.zip @Janitum Yeah that minimal case wasn't broken for me either. I do have one that I'm getting this issue with though with the latest Nuget packages.

            context = new Xbim3DModelContext(model);
            context.CreateContext();  // boom!

Ran @jomijomi 's check before hand and it doesn't seem to make any difference. Looks like there is already enough precision.

Anyone else still getting this?

Belrius commented 1 year ago

Ok I've been debugging. So my issue is with an IFCPolygonalFaceSet.

I get a Standard_ConstructionError in XBimGeometryCreator, which I tracked back to XBimCompound.Cpp Line 1525.

image

Which in turn came from XBimWire.cpp 1320.

image

Where x, y and z are all zero, so it's crashing getting a direction. in gp_Dir (55)

image

So I have two questions:

1) Why does this file load fine in my version of xbim explorer with Version 4? What has changed and can I change it back? 2) This file has come directly from Revit. Is it definitely bad data? Is this a known issue with Revit's exporter?

andyward commented 1 year ago

@SteveLockley can you take a look while you're in the GE area? I had a quick investigation of #412 as well and determined:

I suspect this is similar to a few other StackOverflows you're looking at

guenter1holzeder commented 1 year ago

I also had several problems which stack overflow errors. Commit 24d2d6a is the cure for me too.

In my opinion the develop branch has some important things to be merged into main/netcore/"what ever you call the next release"!!

By the way: What is your scheduled roadmap for version 6?

Belrius commented 1 year ago

Any news?

andyward commented 1 year ago

@Belrius Let me check up with Steve.

@guenter1holzeder Version 6 is nearly there - final few bit of regression testing to do. It's a major update in that it:

  1. Is a new version of OpenCascade
  2. Has a new approach to handling edge-cases & faults
  3. Has significant changes to the interop layer & dynamic linking
  4. Provides net6.0+ support

Should be available in a matter of weeks, not months, but we don't have a hard date - it has to be ready (and work around other projects)

Belrius commented 1 year ago

Thanks @andyward.

  _Works in netcore v5 port_

I've converted my .net framework app to .net 5, in order to test if the "netcore v5 port" will fix my issue. Is this port available through nuget? or just directly from the feature/netcore branch? Is it considered production ready?

Many thanks

andyward commented 1 year ago

Hi @Belrius

There is an unofficial feed at https://pkgs.dev.azure.com/xBIMTeam/_packaging/xbim/nuget/v3/index.json you can get a Nuget from. But it's not yet production-ready. We've not done a full regression test yet and I know there are a couple of known issues:

  1. A multi-threading issue in the native OCC build we're investigating. (workaround-limit to single thread)
  2. There's a boolean issue with openings (an edge case in the Scene rather than Geometry) - should be fixed this week

There are about 18 tests currently failing but they are largely very minor test tollerance issues rather than outright failures.

I would consider it 'beta' ready if you wanted to evaluate any existing issues you have with 5.1. Once in develop (around mid March?) I'd consider ready for Prod, when we'll have an official v6 stack in official Nuget

Belrius commented 1 year ago

Hi Andy,

Thanks a heap for that.

It looks like that's a private feed though. Guessing I'd need to be added as a guest in azure pipelines to access it?

{"$id":"1","innerException":null,"message":"TF400813: The user ' (redacted) ' is not authorized to access this resource.","typeName":"Microsoft.TeamFoundation.Framework.Server.UnauthorizedRequestException, Microsoft.TeamFoundation.Framework.Server","typeKey":"UnauthorizedRequestException","errorCode":0,"eventId":3000}

andyward commented 1 year ago

Andrew I'll look to get you added on

KezbanB commented 7 months ago

I've pushed a fix that mitigates the problem and removes the stack overflow at https://github.com/xBimTeam/XbimGeometry/tree/feature/occt752

Hello everyone, we were having the same stack overflow issue and this change by @CBenghi seem to be solving it (and thank you very much @CBenghi for that). We have some options like using this branch directly or pushing these changes to main branch on a fork, but those are not the most ideal options.

Is it ever planned to have this change on the main branch? I am not very much familiar with the domain or current implementation but is there anything that I can do to help with that if it's a matter of effort?