search

buildingSMART / IFC4.3.x-development

Repository to collect updates to the IFC4.3 Specification
Other
168 stars 86 forks source link

PredefinedType optionality in case of non-existant type objects #658

Open aothms opened 1 year ago

aothms commented 1 year ago

What are the exact rules for predefined type optionality? I initially only had the following rules:

A. Occurrence: optional PredefinedType B. TypeObject: non-optional PredefinedType

But in conversations with @SergejMuhic and @pjanck they pointed me to another situation:

C. Occurrence without a corresponding TypeObject: non-optional PredefinedType

(which we currently don't follow in this repo)

Hence, example of where 4.4 tunnel repo and bsi 4.3 repo disagree: IfcMarineFacility

(On a side note PredefinedType attribute existance is also not consistent, e.g we have PredefinedType on IfcBridge but not on IfcBuilding. But on e.g IfcKerb we specifically added PredefinedType attribute even if it can only select userdefined and notdefined.)

I personally don't find much value in rule C. One can select NOTDEFINED anyway. So I'd rather choose for the simpler situation of just A. and B., but if there's any action needed we should do so now. Changing from optional to non-optional is a breaking change.

aothms commented 1 year ago

@TLiebich do you have any guidance on this from old MSG documents perhaps?

TLiebich commented 1 year ago

I don't recall case C - actually it is currently rather optional - e.g. new in IFC4.3 for the added PredefinedType for IfcVirtualElement. I would recommend to stick with A and B.

IfcBuilding, IfcBuildingStorey is legacy - maybe to be reconsidered in IFC4.4

SergejMuhic commented 1 year ago

There are plenty of case C's:

and then the others:

  1. IfcStructuralAnalysisModel
  2. IfcStructuralCurveAction
  3. IfcStructuralCurveMember
  4. IfcStructuralCurveReaction
  5. IfcStructuralLoadGroup
  6. IfcStructuralResultGroup
  7. IfcStructuralSurfaceAction
  8. IfcStructuralSurfaceMember
  9. IfcSurfaceFeature

So, with spatial structure elements we followed suite. This is true for all infra subtypes of facility and facility parts. Regarding marine facility, we did not influence that and also did not have the authority to change at the time but agreed on the change later when we synchronized the predefined types of all infra spatial structure elements. https://github.com/bSI-InfraRoom/IFC-Specification/pull/463

Since there is no other type for spatial structure elements than IfcSpaceType, I think the approach with no type fits well here. Selecting NOTDEFINED is the exact purpose of not having it empty. It means the user has checked and cannot define yet. If it is optional it might never get filled. PredefinedType being an attribute makes it very compelling for filtering, so I would keep it mandatory as the whole logic revolves around it by not introducing too many leaf nodes underneath but instead still keeping a way to differentiate beyond entity level which is quite necessary even in the existing mechanisms e. g. property sets.

EDIT: I thought this was resolved, see https://github.com/buildingSMART/IFC4.3.x-development/issues/578

aothms commented 1 year ago

Well, I had a look and as always things aren't as consistent as we hoped for. I have implemented these rules in shacl. There are appear to be no violations of the optional rule, but a lot of ptype attributes that should be mandatory according to rule B and C but aren't.

What I will do is that entities introduced after IFC4 will conform to rules A B and C. I will not make any backwards incompatible changes to entities introduced earlier. So even blatant inconsistencies such as IfcFurnitureType.PredefinedType will stay as is.

TypeObjectPredefinedTypeMandatory (B)

OccurenceWithoutTypePredefinedTypeMandatory (C)

aothms commented 1 year ago

What I will do is that entities introduced after IFC4 will conform to rules A B and C. I will not make any backwards incompatible changes to entities introduced earlier. So even blatant inconsistencies such as IfcFurnitureType.PredefinedType will stay as is.

Edit: in retrospect, this creates too much inconsistencies between sibling classes (e.g Grid and Referent, Opening and EarthWorksCut) that I find not excusable. I will instead apply A B and C only to the IfcSpatialStructureElement subtypes where possible, which was the 'disagreement' between 4.3 and tunnel/infra anyway. At night I will be dreaming of a world with IFC5 where all of this is handled generically.

SergejMuhic commented 1 year ago

You take what you get I guess. I can agree on the old IFC4 entities which are a bit of a mystery. But if we can have the new spatial structure derived entities consistent with a mandatory PredefinedType, I can join you in:

At night I will be dreaming of a world with IFC5 where all of this is handled generically.

Although I am not too happy with the IFC5 proposal that I have seen. Maybe this can be discussed some more...

aothms commented 1 year ago

Tbh I'm still weary of the impact on current implementers. I have been retracing the evolution of all of this and at no point in time it seems there was a situation that all facilities had a mandatory predefined type (the parts did have a mandatory select! but that was also necessary for distinguishing the domain of the part at that time). As said earlier, there also isn't a consistently applied rule in the spec by which we can argue this is a necessary change for consistency.

@evandroAlfieri is there any chance you can consult the implementers on their stance regarding this? Removing optionality is a non-backwards compatible change after all (affecting only bsi standards not iso). Do they agree on the need? Do they agree on the time frame (i.e now). Do they agree on the scope, both facility and facility parts? Thanks.

RC1

Bridge: optional, marine: non-optional. Parts: mandatory select.

ENTITY IfcBridge
 SUBTYPE OF (IfcFacility);
    PredefinedType : OPTIONAL IfcBridgeTypeEnum;
END_ENTITY;

ENTITY IfcFacilityPart
 SUPERTYPE OF (ONEOF
    (IfcBridgePart))
 SUBTYPE OF (IfcSpatialStructureElement);
    PredefinedType : IfcFacilityPartTypeSelect;
    UsageType : IfcFacilityUsageEnum;
END_ENTITY;

ENTITY IfcMarineFacility
 SUBTYPE OF (IfcFacility);
    PredefinedType : IfcMarineFacilityTypeEnum;
 WHERE
    CorrectPredefinedType : NOT(EXISTS(PredefinedType)) OR
(PredefinedType <> IfcMarineFacilityTypeEnum.USERDEFINED) OR
((PredefinedType = IfcMarineFacilityTypeEnum.USERDEFINED) AND EXISTS(SELF\IfcObject.ObjectType));
END_ENTITY;

RC2

ibid

RC3

Marine type becomes optional.

ENTITY IfcMarineFacility
 SUBTYPE OF (IfcFacility);
    PredefinedType : OPTIONAL IfcMarineFacilityTypeEnum;
 WHERE
    CorrectPredefinedType : NOT(EXISTS(PredefinedType)) OR
(PredefinedType <> IfcMarineFacilityTypeEnum.USERDEFINED) OR
((PredefinedType = IfcMarineFacilityTypeEnum.USERDEFINED) AND EXISTS(SELF\IfcObject.ObjectType));
END_ENTITY;

RC4

Types introduced for rail and road, both optional

ENTITY IfcRailway
 SUBTYPE OF (IfcFacility);
    PredefinedType : OPTIONAL IfcRailwayTypeEnum;
 WHERE
    HasObjectType : NOT EXISTS(PredefinedType) OR (PredefinedType <> IfcRailwayTypeEnum.USERDEFINED) OR EXISTS(SELF\IfcObject.ObjectType);
END_ENTITY;

ENTITY IfcRoad
 SUBTYPE OF (IfcFacility);
    PredefinedType : OPTIONAL IfcRoadTypeEnum;
 WHERE
    HasObjectType : NOT EXISTS(PredefinedType) OR (PredefinedType <> IfcRoadTypeEnum.USERDEFINED) OR EXISTS(SELF\IfcObject.ObjectType);
END_ENTITY;

TC1

Facility part types rewritten to explicit entities. Predefined type made optional (without giving it a lot of thought I guess)

ENTITY IfcBridgePart
 SUBTYPE OF (IfcFacilityPart);
    PredefinedType : OPTIONAL IfcBridgePartTypeEnum;
 WHERE
    CorrectPredefinedType : NOT(EXISTS(PredefinedType)) OR
 (PredefinedType <> IfcBridgePartTypeEnum.USERDEFINED) OR
 ((PredefinedType = IfcBridgePartTypeEnum.USERDEFINED) AND EXISTS (SELF\IfcObject.ObjectType));
END_ENTITY;

Infra repo tunnel branch

Both facility and parts made non-optional in commit https://github.com/bSI-InfraRoom/IFC-Specification/commit/09c6c32ab7b4b915974953e90656373b5f616b1d dd August 10 2022 part of PR https://github.com/bSI-InfraRoom/IFC-Specification/pull/463 referenced by @SergejMuhic earlier.

EDIT: I thought this was resolved, see https://github.com/buildingSMART/IFC4.3.x-development/issues/578

We agreed on this in the AECGeeks repo.

https://github.com/AECgeeks/infra-repo-issue-test-1/issues/6

evandroAlfieri commented 1 year ago

Good discussion, with pros and cons on every side. It deserves further consensus though. And unfortunately, IFC4.3 must be re-issued to ISO next week - meaning no time to gather implementers and get a conclusion quickly. Given this situation and i) the consistency principle (all or nothing), ii) the risk of making the attributes mandatory before consulting with implementers, I say that if this change has to happen now can only be on the safe/conservative side: all optional. Further constraints can be imposed later, when implementers agree.

SergejMuhic commented 1 year ago

I can say that all of the implementers in Tunnel (most are in IF and are implementing 4X3) have been dealing with it for the past year.

Any decision is good, but it would be great to have common criteria. There are many changes in this submission that nobody tested and are quite arbitrary. On the other hand, we have a quite simple correction of an error in modelling (since the mechanism is already in place in the schema) that is easy to implement with a default value .NOTEDEFINED.. You see my point?