AECgeeks / infra-repo-issue-test-2

0 stars 0 forks source link

jwg12 IfcPoint Dim #8

Open aothms opened 2 years ago

aothms commented 2 years ago

Move derived Dim attribute up to IfcPoint

aothms commented 2 years ago
--- tmp/a/IFC.exp   2022-08-26 22:55:57.254516200 +0200
+++ tmp/b/IFC.exp   2022-09-05 20:18:27.263500500 +0200
@@ -4757,22 +4757,20 @@
  SUBTYPE OF (IfcDeepFoundationType);
    PredefinedType : IfcCaissonFoundationTypeEnum;
  WHERE
    CorrectPredefinedType : (PredefinedType <> IfcCaissonFoundationTypeEnum.USERDEFINED) OR
  ((PredefinedType = IfcCaissonFoundationTypeEnum.USERDEFINED) AND EXISTS (SELF\IfcElementType.ElementType));
 END_ENTITY;

 ENTITY IfcCartesianPoint
  SUBTYPE OF (IfcPoint);
    Coordinates : LIST [1:3] OF IfcLengthMeasure;
- DERIVE
-    Dim : IfcDimensionCount := HIINDEX(Coordinates);
  WHERE
    CP2Dor3D : HIINDEX(Coordinates) >= 2;
 END_ENTITY;

 ENTITY IfcCartesianPointList
  ABSTRACT SUPERTYPE OF (ONEOF
    (IfcCartesianPointList2D
    ,IfcCartesianPointList3D))
  SUBTYPE OF (IfcGeometricRepresentationItem);
  DERIVE
@@ -8573,48 +8571,44 @@
  ((PredefinedType = IfcPlateTypeEnum.USERDEFINED) AND EXISTS (SELF\IfcElementType.ElementType));
 END_ENTITY;

 ENTITY IfcPoint
  ABSTRACT SUPERTYPE OF (ONEOF
    (IfcCartesianPoint
    ,IfcPointByDistanceExpression
    ,IfcPointOnCurve
    ,IfcPointOnSurface))
  SUBTYPE OF (IfcGeometricRepresentationItem);
+ DERIVE
+    Dim : IfcDimensionCount := IfcPointDim(SELF);
 END_ENTITY;

 ENTITY IfcPointByDistanceExpression
  SUBTYPE OF (IfcPoint);
    DistanceAlong : IfcCurveMeasureSelect;
    OffsetLateral : OPTIONAL IfcLengthMeasure;
    OffsetVertical : OPTIONAL IfcLengthMeasure;
    OffsetLongitudinal : OPTIONAL IfcLengthMeasure;
    BasisCurve : IfcCurve;
- DERIVE
-    Dim : IfcDimensionCount := BasisCurve.Dim;
 END_ENTITY;

 ENTITY IfcPointOnCurve
  SUBTYPE OF (IfcPoint);
    BasisCurve : IfcCurve;
    PointParameter : IfcParameterValue;
- DERIVE
-    Dim : IfcDimensionCount := BasisCurve.Dim;
 END_ENTITY;

 ENTITY IfcPointOnSurface
  SUBTYPE OF (IfcPoint);
    BasisSurface : IfcSurface;
    PointParameterU : IfcParameterValue;
    PointParameterV : IfcParameterValue;
- DERIVE
-    Dim : IfcDimensionCount := BasisSurface.Dim;
 END_ENTITY;

 ENTITY IfcPolyLoop
  SUBTYPE OF (IfcLoop);
    Polygon : LIST [3:?] OF UNIQUE IfcCartesianPoint;
  WHERE
    AllPointsSameDim : SIZEOF(QUERY(Temp <* Polygon | Temp.Dim <> Polygon[1].Dim)) = 0;
 END_ENTITY;

 ENTITY IfcPolygonalBoundedHalfSpace
@@ -13255,20 +13249,40 @@
    END_LOCAL;
      N := SIZEOF (APath.EdgeList);
    REPEAT i := 2 TO N;
       P := P AND (APath.EdgeList[i-1].EdgeEnd :=:
                   APath.EdgeList[i].EdgeStart);
    END_REPEAT;
    RETURN (P);

 END_FUNCTION;

+FUNCTION IfcPointDim
+ (Point : IfcPoint)
+ : IfcDimensionCount;
+
+  IF ('IFC4X3_DEV.IFCCARTESIANPOINT' IN TYPEOF(Point))
+    THEN RETURN(HIINDEX(Point\IfcCartesianPoint.Coordinates));
+  END_IF;
+  IF ('IFC4X3_DEV.IFCPOINTBYDISTANCEEXPRESSION' IN TYPEOF(Point))
+    THEN RETURN(Point\IfcPointByDistanceExpression.BasisCurve.Dim);
+  END_IF;
+  IF ('IFC4X3_DEV.IFCPOINTONCURVE' IN TYPEOF(Point))
+    THEN RETURN(Point\IfcPointOnCurve.BasisCurve.Dim);
+  END_IF;
+  IF ('IFC4X3_DEV.IFCPOINTONSURFACE' IN TYPEOF(Point))
+    THEN RETURN(Point\IfcPointOnSurface.BasisSurface.Dim);
+  END_IF;
+  RETURN (?);
+
+END_FUNCTION;
+
 FUNCTION IfcPointListDim
 (PointList : IfcCartesianPointList)
            : IfcDimensionCount;

   IF ('IFC4X3_DEV.IFCCARTESIANPOINTLIST2D' IN TYPEOF(PointList))
     THEN RETURN(2);
   END_IF;     
   IF ('IFC4X3_DEV.IFCCARTESIANPOINTLIST3D' IN TYPEOF(PointList))
     THEN RETURN(3);
   END_IF;     
SergejMuhic commented 2 years ago

May I propose a case statement here? It is sort of on the boundary (4 ifs + default makes 5 checks), but it would look much better.

aothms commented 2 years ago

But then what about all the other Ifc____Dim functions? This seems to be the way we have done things in the past and it was just copied over by Thomas.

SergejMuhic commented 2 years ago

Not really. Dim functions were typically not separately defined. If you look at something like IfcCorrectDimensions, you can see CASE being used. It makes for a much nicer function.

Let me know whether I should look at it.

aothms commented 2 years ago

But IfcCorrectDimensions checks for enum constants, typeof() returns a set of strings, I don't see how it would work in a case statement.

SergejMuhic commented 2 years ago

Indeed it might not work best. I found a definition of TYPEOF. My idea was to

case TYPEOF(Point)[1]

which would potentially work in this particular case (maybe as [2]) where IfcPoint only has leaf nodes since the set is initialized with the type that Point was declared as and that the instance represents.

pjanck commented 2 years ago

Meta: How to give approval (this isn't a PR ...)?

I greet the introduction of the function and it seems to have correct syntax.

aothms commented 2 years ago

@SergejMuhic typeof() returns a set; can they be indexed like that and are they ordered to the extent that the first element is the most specific type? It's an honest question, but also hints at my doubts on doing too much refactoring on these rules.

@pjanck this is just a quick and dirty way for @SergejMuhic and me to communicate. I think what will happen is that @SergejMuhic creates a PR off of this discussion.

pjanck commented 2 years ago

just a quick and dirty way

I see. Then I'll await the PR.

SergejMuhic commented 1 year ago

Finally remembered how I wanted to implement CASE for these kinds of checks! It slipped my mind and I couldn't remember anymore. https://github.com/bSI-InfraRoom/IFC-Specification/pull/533#issuecomment-1457866240

CASE TRUE OF
    VALUE_IN(typepath,'IFCGEOMETRYRESOURCE.IFCSPIRAL') :  do_something;
aothms commented 1 year ago

Hm yes, but this doesn't match the typical switch/case expression in typical languages where the switch has an expression, but the cases are statically evaluated (in C languages only integral types even, so no strings, so that it directly translates to a jmp instruction). At least the if statement approach is more universally covered in languages. It's just really unfortunate that this is how type checking is done in express.

SergejMuhic commented 1 year ago

switch/case expression in typical languages where the switch has an expression

Certainly, completely agree there. Technically, TRUE is an expression though. 😉

It would make for a clearer interpretation way for type checks. That way my point. It also looks nicer than a bunch of IFs.