Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inline faces in IfcPolygonalFaceSet instead of an external IfcIndexedPolygonalFace #71

Open
Moult opened this issue Sep 9, 2020 · 7 comments
Labels
enhancement New feature or request

Comments

@Moult
Copy link

Moult commented Sep 9, 2020

Description of the proposal:

Consider the following snippet - why can't the IfcIndexedPolygonalFace be in-lined (similar to IfcCartesianPointList3D into IfcPolygonalFaceSet? Wouldn't that be much, much more efficient?

#15=IFCINDEXEDPOLYGONALFACE((2,1,4,5));
#16=IFCINDEXEDPOLYGONALFACE((1,2,8,3));
#17=IFCINDEXEDPOLYGONALFACE((1,3,6,4));
#18=IFCINDEXEDPOLYGONALFACE((4,6,7,5));
#19=IFCINDEXEDPOLYGONALFACE((3,8,7,6));
#20=IFCINDEXEDPOLYGONALFACE((8,2,5,7));
#21=IFCCARTESIANPOINTLIST3D(((265.,4.,0.),(265.,6.,0.),(277.,4.,0.),(265.,4.,2.),(265.,6.,2.),(277.,4.,2.),(277.,6.,2.),(277.,6.,0.)));
#22=IFCPOLYGONALFACESET(#21,$,(#15,#16,#17,#18,#19,#20),$);
#23=IFCSHAPEREPRESENTATION(#10,'Body','Tessellation',(#22));

The proposal:

#21=IFCCARTESIANPOINTLIST3D(((265.,4.,0.),(265.,6.,0.),(277.,4.,0.),(265.,4.,2.),(265.,6.,2.),(277.,4.,2.),(277.,6.,2.),(277.,6.,0.)));
#22=IFCPOLYGONALFACESET(#21,$,((2,1,4,5),(1,2,8,3),(1,3,6,4),(4,6,7,5),(3,8,7,6),(8,2,5,7)),$);
#23=IFCSHAPEREPRESENTATION(#10,'Body','Tessellation',(#22));

The proposal is 58% of the original size in bytes.

Describe how it contributes to the objectives (https://github.com/buildingSMART/NextGen-IFC/wiki/Towards-a-technology-independent-IFC): Simpler structure, same outcome.

Is this a proposal to 'add', 'remove' of 'change' entities in the schema (pick one): CHANGE+REMOVE

What do we win: Files shrink to half their filesize otherwise.

What do we lose: nothing?

Schema impact: Change the data type of one of the attributes

Instance model impact: NA

Backwards compatible: No

Automatic migration possible: Yes

Additional implications: NA

Note that not all points need to be satisfied!
Backwards compatibility and file size are not concerns.

@Moult Moult added the enhancement New feature or request label Sep 9, 2020
@jmirtsch
Copy link

jmirtsch commented Sep 9, 2020

The faces are inline for Triangulate Face Set, but I would suggest the logic for polygonal is that a face can have inner loops via the subclass https://standards.buildingsmart.org/IFC/DEV/IFC4_3/RC1/HTML/link/ifcindexedpolygonalfacewithvoids.htm

It is possible that a list of lists of integer could be used, I do agree there would be a significant efficiency in doing this.

@Moult
Copy link
Author

Moult commented Sep 10, 2020

Considering the relative rarity of mesh programs supporting inner loops (perhaps sketchup is one), I propose: IfcPolygonalFaceSet and IfcPolygonlFaceSetWithVoids. The former is as I have proposed, the latter could be (an off-the-top-of-my-head suggestion):

#22=IFCPOLYGONALFACESETWITHVOIDS(#21,$,((2,1,4,5),(1,2,8,3),(1,3,6,4),(4,6,7,5),(3,8,7,6),(8,2,5,7)),$,(1,3),((A,B,C,D),(E,F,G,H)));

2 added attributes are:

  • VoidedFaceIndex - the indices of faces which have voids
  • InnerLoops - the loops of the voids which correspond to voided face index

The filesize savings are too great to be ignored :)

@Moult
Copy link
Author

Moult commented Sep 10, 2020

My little snippet yesterday doesn't take into account multiple inner loops, but you get the idea :)

@TLiebich
Copy link

Thanks @Moult - its a good and valid proposal. Actually, when developing support for tessellation in IFC4, there was one proposal to only use a single instance to exchange a polygonal face set (even with voids), using a multidimensional list of integers. Leading to:

#22=IFCPOLYGONALFACESET(#21,$,(((2,1,4,5)),**((1,2,8,3),(5,4,3,2))**,((1,3,6,4)),((4,6,7,5)),((3,8,7,6)),((8,2,5,7))),$);

It was then not used fearing that a three-dimensional list would be too complex. The inner loop was mandatory in order to use it to superseed IFCFACETEDBREP which offers this possibility. But splitting it into a simple polygonal face set without inner loops and one with inner loops makes sense.

@hlg
Copy link

hlg commented Sep 16, 2020

... be in-lined similar to IfcCartesianPointList3D into IfcPolygonalFaceSet

Where is IfcCartesianPointList3D "inlined"? I probably missed or misunderstood something.

It was then not used fearing that a three-dimensional list would be too complex.

I think it is confirmed that addition of two-dimensional arrays in IFC4 definitely caused substantial implementation and refactoring efforts in existing software. Just want to mention this, even though forward compatibility is not a concern here.

@Moult
Copy link
Author

Moult commented Sep 16, 2020

@hlg sorry for the phrasing confusion, I meant that IfcCartesianPointList3D has in-lined vertex coordinates.

@aothms
Copy link
Contributor

aothms commented Sep 19, 2020

I agree with @TLiebich that the nested lists is a effective way to keep the pairing between facets and inner loops. Like so:

TYPE IfcIndexedPolygonalFace = LIST[1:?] OF LIST [3:?] OF IfcPositiveInteger;
END_TYPE;

Essentially this in "indexed WKT". https://en.wikipedia.org/wiki/Well-known_text_representation_of_geometry WKT also has a polygon definition as a sequence outer followed by inner bounds.

As said elsewhere we loose the two inherited inverse attributes:

LayerAssignment : SET [0:1] OF IfcPresentationLayerAssignment FOR AssignedItems;
StyledByItem : SET [0:1] OF IfcStyledItem FOR Item;

So it's a question of whether styling needs to be supported on individual facets. We have the IfcIndexedColourMap obviously so it's partly redundant, but style > colour. And now it's unclear how IfcIndexedColourMap will interplay with StyledByItem so removing StyledByItem is a quick way to iron that out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants