xBimTeam / XbimExchange

XbimExchange contains several COBie schemas and serialisation functions as well as the Model Validation library adopted by theNBS digital toolkit.
https://xbimteam.github.io/
Other
46 stars 41 forks source link

COBieLiteUK: Conversion from IFC 2x3 sets Contact Keys to null #17

Open MichaelBuehler opened 7 years ago

MichaelBuehler commented 7 years ago

Hello,

Conversion IFC to COBie LiteUK works if model is based on IFC4, but not on IFC2x3.

The Email is used as Contact key, but always null if defined inside IFC2x3 model (if not set @undefined.email is used)

Involved code lines causeing issue On COBieLiteUKHelper.cs Line 1647 and 1656:

Similar potentialy not working code patterns are used elsewhere, as in MappingIfcActorToContact.

Using version on NuGET: Essentials 4.0.15 and Exchange 4.0.4.

Jero9999 commented 7 years ago

Hey cool I was just about to raise this very same issue. It's a real problem since the email address is like a primary key for the contact info in cobie.

I'm not sure how the code would ever work. email = telecom.ElectronicMailAddresses.FirstOrDefault(e => !string.IsNullOrWhiteSpace(e));

To be clear the issue is that the string.IsNullOrWhitespace() call requires a string parameter. However the e variable is not a string. It is an IfcLabel. IfcLabel implements IEquality<string> but that is not the same as it being implicitly convertible to a string. Therefore the call fails. The only thing I don't understand is why it doesn't cause an exception.

Jero9999 commented 7 years ago

Hi there again. I have been looking at this further and I I was a bit wrong about the issue when I wrote the above. The issue I think will only happen when using IFC 2x3. The problem is in the ProxyItemSetclass. For IfcTelecomAddress.ElectronicMailAddresses it contains a bunch of Ifc2x3 IfcLabels. But the ProxyItemSet.FirstOrDefault() filters on IFC4 IfcLabels. Therefore the result will always be empty.

I think it is because it is using the wrong overload of FirstOrDefault()

Note that if you change the calling code to var emailAddress = telecom.ElectronicMailAddresses.FirstOrDefault<Xbim.Ifc2x3.MeasureResource.IfcLabel>(t => !string.IsNullOrWhiteSpace(t)); then the email address is successfully returned. However that locks it to IFC2x3 which isn't ideal

CBenghi commented 7 years ago

Hi Jero and Michael, It looks like something that can be easily fixed; would you be able to write a short test case? That would require a couple of ifc files (possibly one ifc2x3 and one ifc4) and then testing the conversion.

That would help a lot to write a solution and prevent future regressions. I'd be happy to fix the library then.

Thanks, Claudio

Jero9999 commented 7 years ago

As requested, here is a example for you. hopefully it is what you are after. xbimBugTestCase.zip

Now please fix it yesterday :-)

CBenghi commented 7 years ago

Hello All, I've fixed the specific issue in the test provided by Jero, but you mention other cases such as MappingIfcActorToContact @MichaelBuehler, before I investigate that, would you be able to let me have a list of the other areas of problem that you mention? I'm quite rusty on this library it would take time to familiarise again. Thanks, Claudio

MichaelBuehler commented 7 years ago

Hi Claudio I used only a quick check with "Find all references" on ElectronicMailAddresses: A similar pattern appears beside above COBieLiteUKHelper.cs also in

regards Michael

CBenghi commented 7 years ago

This code is nuts! I have to think hard about a review.

Jero9999 commented 7 years ago

Hahaha

Yeah I thought you might have been a little overconfident about how easy the fix would be. The problem seems to be a fairly fundamental one.

Jero9999 commented 7 years ago

Hi Claudio

Any update on this issue? Seems that it is wasn't the quick fix you were hoping for. But do you have an ETA?

CBenghi commented 7 years ago

Hello @Jero9999 , I remember looking at this and thinking that the other places mentioned by @MichaelBuehler do not seem to be involved in actual code paths. Can you confirm if the code is still faulty using current develop branch? I think it might be fixed, but with no robust test case it's difficult to know. I think that the file you passed on this thread does work now. Best, Claudio

Jero9999 commented 7 years ago

Hi @CBenghi I'm afraid it's not fixed. Looking at the unit test in UnitTest1.cs I can see that email is still a random GUID. Phone, PostalCode, and Country are also n/a which is not right. Finally Street is actually "Xbim.Common.Collections.ProxyValueSet`2[Xbim.Ifc2x3.MeasureResource.IfcLabel,Xbim.Ifc4.MeasureResource.IfcLabel]" so the ToString() isn't really working there.

The problem as far as I could tell is actually in the call to IItemSet<>.OfType<>() (CobieLiteUKHelper.cs:line 1663 of your updated version). It never works as required.

Thanks Jero

CBenghi commented 7 years ago

I'm puzzled at what happens there. the code I've pushed a few moments ago seems to work, but I still have to understand why the old one didn't .. I will keep this open as a reminder. Claudio