NextGen-IFC icon indicating copy to clipboard operation
NextGen-IFC copied to clipboard

IfcMaterialLayerSet => IfcMaterialLayerList and other Set/List inconsistencies

Open jakob-beetz opened this issue 4 years ago • 5 comments

Tiny, and maybe discussed before within MSG but I have witnessed people being ~~infuriated~~ slightly irritated about this: Currently, we have

ENTITY IfcMaterialLayerSet
 SUBTYPE OF (IfcMaterialDefinition);
	MaterialLayers : LIST [1:?] OF IfcMaterialLayer;
	LayerSetName : OPTIONAL IfcLabel;
	Description : OPTIONAL IfcText;
 DERIVE
	TotalThickness : IfcLengthMeasure := IfcMlsTotalThickness(SELF);
END_ENTITY;

with LIST as a collection type (proper, because order of layers is very relevant) but Set in the naming

similar for

ENTITY IfcPolygonalFaceSet
	Faces : LIST [1:?] OF IfcIndexedPolygonalFace;

I thought this was an inheritance from ISO 10303-42, but there we only have a (meaningful)

EBTITY connected_face_set 
 SUPERTYPE OF (OBEOF (closed_shell, open_shell)) 
  SUBTYPE OF (topological_representation_item); 
      cfs_faces : SET [1:?) OF face; 
END_ENTITY; 

jakob-beetz avatar Mar 06 '20 07:03 jakob-beetz

+1 seems like an easy fix.

If possible, we should resort to SETs (no order), instead of LISTs. Not possible in this case, so that should be IfcMaterialLayerList and IfcPolygonalFaceList.

pipauwel avatar Mar 08 '20 09:03 pipauwel

while I agree on the issue, I would question the cost / benefit ratio here. Yes, a name "IfcMaterialLayerList" is actually more precise, but the cost to change the class = all instance names is high (this entity exists since end of the 1990's in IFC with probably millions of instances in IFC files).

I would opt for not changing, but clearly stating the anomaly in the documentation.

TLiebich avatar Mar 09 '20 12:03 TLiebich

or alternatively:

  • Remove material layers altogether and use decompositions so that you can also attach psets to the layers (my preference: Explicit is better than implicit., There should be one - and preferably only one - obvious way to do it. to cite one former BDFL)
  • Turn it into an actual set by only using IfcMaterialLayerWithOffsets and make IfcMaterialLayer abstract so that the structure becomes order-independent (but implementers have to sort manually upon import)
  • Consolidate MLSet and MLSetUsage and have an aggregation attribute directly on the usage (LIST OF IfcMaterialLayer).

There are other points to consider in attribute names, such as OffsetFromReferenceLine (a) it's probably a surface at least in case of slabs (b) it doesn't need to be a line, can be an arbitrary curve in case of curved walls. Point is, if we intend to be precise about terminology we need to review a lot.

aothms avatar Mar 10 '20 13:03 aothms

Meets the requirements of https://github.com/buildingSMART/NextGen-IFC/wiki/Ten-principles-for-a-future-IFC

berlotti avatar Mar 13 '20 12:03 berlotti

What was the conclusion on this one? It wasn't clear from reading the comments.

Moult avatar Mar 15 '20 21:03 Moult