buildingSMART / IFC4.3.x-development

Repository to collect updates to the IFC4.3 Specification
Other
161 stars 83 forks source link

IfcFacilityPart.PredefinedType #90

Closed NickNisbet closed 2 years ago

NickNisbet commented 3 years ago

IfcFacilityPart.PredefinedType is unlike any other PredefinedType. The exceptional representation of IfcFacilityPart.PredefinedType is so disruptive to implementation that, although 'correctly' specified, it should count as a significant Bug.

The majority of my programming time over the last three months has been devoted to correctly implementing this and then finding that other tools do not handle it properly.

Surely it is not beyond the collaborative working of the different infrastructure sub-projects to consolidate the allowed values of the enum into a single list, bringing this PredefiendType back into line with ALL others.?

aothms commented 3 years ago

I agree it's somewhat unfortunate. 1. Within the group of implementers there appears to be quite a few that want to work towards minimizing the use of "sum types". So it's somewhat odd that we're introducing a whole new use of them (union of enum). 2. Furthermore, it's only temporary as in IFC5 all of the predefined type stuff will be eliminated in favour of purely latebound element typing. 3. Conceptually, a union of enum is in my point of view problematic. An enum is a union of unit types, so we're creating here a union of unions of unit types and depend on the fact that in the express modelling language these enumerations are "namespaced" (i.e enum elements with the same name are not considered unique). In most programming languages and conceptual models though enums are not namespaces and then union of unions of unit types is equal to just a union of unit types.

At this time I doubt something can still be done though?

SergejMuhic commented 2 years ago

it's only temporary as in IFC5 all of the predefined type stuff will be eliminated in favour of purely latebound element typing

Sorry, of topic but I am not sure where else to put this. Why not just use classification and publish an IFC late bound classification system on bSDD? This would be an awesome bSDD advertisement and showcase. Wherever the cut off takes place, just add a direct Classification attribute to IfcClassificationReference. This would also enable to not have any typing if it is not needed. If I just want to see my "331 Tragende Wand" I don't really need to know anything other than that it is an IfcElement. Why type it at all in this case since classification does everything?

FYI, reasoning for the various type enums in IfcFacilityPart was a compromise between domains on how to handle their own types in their own domain. Don't shot the messenger please. I proposed it as sort of a joke because the discussions just did not go anywhere as ditches were dug deep. I had no clue that anyone would go for this. I need to learn how to keep my mouth shut.

Moult commented 2 years ago

This is marked as decided, but it wasn't clear what the conclusion was? Is the decision to leave it as is for the moment as a temporary measure?

aothms commented 2 years ago

Note this also applies to:

Moult commented 2 years ago

To clarify, the decision to solve this is to split out the class into subclasses. Each subclass would have one enumtype. No more select of enums.

TLiebich commented 2 years ago

or to flatten it into a single list - could we look into the three occasions one-by-one? e.g. IfcImpactProtectionDevice might already be so specialized that it would be rather unbalanced to add another level of sub classes. Why not adding the means of impact protection as an own property?

Whereas IfcTransportElement is best split into two - the existing IfcTransportElement being fixed and IfcVehicle (or similar) non fixed.

aothms commented 2 years ago

@TLiebich we did that this morning and for several reasons in all cases we found subtyping the best option e.g closest to semantics from the projects, separation of concerns in terms of responsibilities and subschemas (IIRC IfcImpactProtectionDevice had different enums coming from HVAC vs infra)

I must say I really like the suggestion of IfcVehicle instead of the IfcTransportElementNonFixed we had planned.

TLiebich commented 2 years ago

if there are good semantic reasons, I am fine.

note bene: I found other inconsistencies in handling PredefinedType

to 1st.

to 2nd.

aothms commented 2 years ago

See below for some screenshots of the modifications.

Questions and comments:

Facility part

afbeelding

Impact protection

old

afbeelding

new

afbeelding

Transport

old

afbeelding

new

afbeelding

TLiebich commented 2 years ago

a few objections ;-)

a related question (or better open a new issue) -road, rail and port&waterway part is defined in own sub schemas, bridge part not - isn't it inconsistent?

aothms commented 2 years ago

Thanks @TLiebich for the insightful and quick comments

So

aothms commented 2 years ago

Ok, just to put in something, I now have

IfcAbstractImpactProtectionDevice > { IfcVibrationDamper, IfcImpactProtectionDevice, IfcVibrationIsolator }.

I tried to look up a shared hyperonym of vibration and impact, protection and isolation, but didn't find clues. Didn't want to invent my own thing such as a IfcKineticEnergyHandlingDevice.

My current preference is to actually remove the intermediate supertype and have them directly under IfcElementComponent. When i don't hear objections I will move on with that in the coming days.

IfcElementComponent -> {IfcBuildingElementPart, IfcDiscreteAccessory, IfcFastener, IfcVibrationDamper, IfcImpactProtectionDevice, IfcVibrationIsolator, IfcMechanicalFastener, IfcReinforcingElement, IfcSign}

As for example I don't think the { IfcFastener IfcMechanicalFastener } concepts are any less disjoint than { IfcVibrationDamper, IfcImpactProtectionDevice, IfcVibrationIsolator }

Edit: it looks like that was the case in RC2.

aothms commented 2 years ago

afbeelding

afbeelding

afbeelding

TLiebich commented 2 years ago

would second to skip the intermediate abstract supertype

Moult commented 2 years ago

It looks as though this is now implemented. Can I close this?

evandroAlfieri commented 2 years ago

Notes for railway stakeholders - as requested:

Context Some railway stakeholders are considering to reuse the material produced during the IFC Rail Project for next projects and for internal purposes.

Warning The decision made with this issue results in a change of schema (i.e., the one delivered to ISO is different from the one used for the tests). Please, note that the following material for the topics and tests involving spatial structure is to be considered invalid and cannot be reused as it is:

  1. dataset
  2. unit test cases
  3. storyline tests
  4. test instructions
  5. reference IFC files provided by TS
  6. proof of implementation (IFC files) provided by vendors

Based on project's deliverables, the affected stakeholders are:

A quick estimation shows that this change affects 20%-25% of the IFC 4.3 files delivered by the Project during Unit Test and Storyline phase.

Additional note At the time of writing, it is not clear if tests or reference files for the newly proposed solution are available - or were produced and made available, and reviewed, before issuing the change.

@czapplitec @larswik

aothms commented 2 years ago

@evandroAlfieri note that several other issues have been incorporated that also affect schema. Most of them are building domain related and often compatible additions, but at the very least issues such as https://github.com/buildingSMART/IFC4.3.x-development/issues/397 might directly affect some data?

evandroAlfieri commented 2 years ago

Thanks @aothms. The Rail project haven't run a proper check, against all changes. I'm confident that building-related changes do not affect our work, and that changes like the one you flagged (#397) impacts maybe 3 or 4 files. At a quick look, this spatial structure change appears to be the most critical (for the project's work)