GeometryGym / GeometryGymIFC

C# classes to generate and parse OpenBIM IFC files
Other
273 stars 97 forks source link

exception on IfcDuration.Convert( string s) #77

Closed simonedd closed 1 year ago

simonedd commented 1 year ago

Hi,

I have a file with an IfcDuration with a wrong definition ( IFCDURATION('50') ) and during the parsing there is an exception in the IfcDuration.Convert method. Is it possible to edit the method to avoid the exception? Or to catch the exception to avoid import fails? Currently, on IfcPropertySingleValue the is a try-catch surrounding the ParserIfc.parseValue(s), this is missing in other places, e.g. in the IfcPropertyBoundedValue.

jmirtsch commented 1 year ago

Hi Simone,

I try to make the toolkit tolerant of non-compliant IFC parsing as much as possible (trying not to penalize the performance of compliant files). I've just pushed a commit that should work better with these requests. Let me know if not. If you need a nuget package built, let me know, else it will be in the next one.

simonedd commented 1 year ago

I tried the fix and it works, I can open the file now. May I ask you if there is a reason for assuming that the integer value '50' corresponds to Days? Is not better to just return an "empty" duration if the first letter is not a 'P'? Something like this:

IfcDuration duration = new IfcDuration();
if (string.IsNullOrEmpty(s) || s[0] != 'P')
    return duration;
jmirtsch commented 1 year ago

I'd suggest the most common unit of planning work on a typical construction project is days (from my experience which I would admit isn't comprehensive). What unit was implied in the file you have? I would urge you to contact the vendor authoring the file and get them to make the duration string compliant ASAP. Empty durations are also dangerous if the problem is in isolation. A more conservative approach would be to assume it's years.

One thing that I never found a good solution too is the best way to log/flag any non compliance detected, although there are dedicated checking tools better at this (and I hope a lot more emerge).

simonedd commented 1 year ago

Thanks for the explanation. These are the unit I found in the file:

#40= IFCSIUNIT(*,.TIMEUNIT.,$,.SECOND.);
#41= IFCMEASUREWITHUNIT(IFCTIMEMEASURE(31556926.),#40);
#42= IFCDIMENSIONALEXPONENTS(0,0,1,0,0,0,0);
#43= IFCCONVERSIONBASEDUNIT(#42,.TIMEUNIT.,'Year',#41);

Anyway, I'm satisfied with the fix, the important thing is to don't fail the import. If a value is not meaningful on a non-compliant file is not a problem for me. Having a logger would be nice, maybe you can use a static class or a singleton to log the info, and use a lock statement for access on multithread.