buildingSMART / IFC4.3.x-development

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

missing entity for defining texture maps for polygonal face sets #135

Closed TLiebich closed 2 years ago

TLiebich commented 2 years ago

original issue from SCTE Review (Norway)

IfcIndexedTextureMap is missing a subtype for textures for IfcPolygonalFaceSet

https://standards.buildingsmart.org/IFC/DEV/IFC4_3/RC2/HTML/schema/ifcpresentationappearanceresource/lexical/ifcindexedtexturemap.htm

Here it states:

" Subtypes of IfcIndexedTextureMap establish the index attribute corresponding to subtypes of IfcTessellatedFaceSet defining the corresponding index lists of vertices."

So, if I want textures on my IfcTriangulatedFaceSet, I will of course use the subtype IfcIndexedTriangleTextureMap. But if I want to have textures on my IfcPolygonalFaceSet, there seems to be missing a corresponding IfcIndexedPolygonalTextureMap.

The IfcIndexedTextureMap is abstract, so we can’t use that.

TLiebich commented 2 years ago

yes the request is valid. When adding the IfcPolygonalFaceSet in IFC4.0.2 adding, the need to also add a particular support for textures on polygonal faces seems to be missed. It would be a good time to add.

I briefly checked the spec. The addition would be along:

ENTITY IfcIndexedPolygonalTextureMap SUBTYPE OF (IfcIndexedTextureMap); TexCoordinates : L[1:?] OF L[3:?] IfcPositiveInteger; END_ENTITY;

a where rule could be added to check that the entity type the the corresponding face set is actually a IfcPolygonalFaceSet. Along (to be validated)

WHERE CorrectFaceSetType : 'IFC4x4.IFCPOLYGONALFACESET' IN TYPEOF(SELF\IfcIndexedTextureMap.MappedTo;

Note: similar should be added to IfcIndexedTriangleTextureMap

WHERE CorrectFaceSetType : 'IFC4x4.IFCTRIANGULATEDFACESET' IN TYPEOF(SELF\IfcIndexedTextureMap.MappedTo;

TLiebich commented 2 years ago

Also at IfcIndexedTriangleTextureMap

consider to make to attribute TexCoordinate mandatory.

ENTITY IfcIndexedTriangleTextureMap SUBTYPE OF (IfcIndexedTextureMap); TexCoordIndex : LIST [1:?] OF LIST [3:3] OF IfcPositiveInteger; END_ENTITY;

aothms commented 2 years ago

One problem I see is that polygonal faces can have holes. So a one dimensional list of indices per face might not be enough. I was thinking to reuse the same polygonal face definition but pair it with a 2d coord list.

Sent from a mobile device. Excuse my brevity. Kind regards, Thomas

Op wo 12 jan. 2022 19:27 schreef Thomas Liebich @.***>:

Also at IfcIndexedTriangleTextureMap

consider to make to attribute TexCoordinate mandatory.

ENTITY IfcIndexedTriangleTextureMap SUBTYPE OF (IfcIndexedTextureMap); TexCoordIndex : LIST [1:?] OF LIST [3:3] OF IfcPositiveInteger; END_ENTITY;

— Reply to this email directly, view it on GitHub https://github.com/buildingSMART/IFC4.3.x-development/issues/135#issuecomment-1011333109, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAILWV63SZPQEJM27JQOSDDUVXB2HANCNFSM5LZVFS6Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you are subscribed to this thread.Message ID: @.***>

TLiebich commented 2 years ago

quesion to all with a good understanding how CG and shaders work in detail. Is it sufficient to have the outer contour texture coordinate mapping (to position the texture coordinates onto the 3D plane) and leave it to the geometric engine to cut out the inner holes from the texture, or do we need the holes already in explicit 2D texture coordinates upfront?

aothms commented 2 years ago

I'd say for the shader it's too late, their you need a consistent vertex definition. And at that time you're also already talking about triangulated data.

I think you could argue that the indeed the inner bounds uvs should be linearly interpolated from the outer bounds. But if we do that we're going to have to define how, to prevent ambiguities. The outer bound uv coords do not necessarily define a consistent embedding of uv space in 3d. Typically of course they should and will otherwise you have weird distorted appearance.

I'd still prefer to have uv definitions also for the inner bounds just for the sake of less work for (a) bSI spending less time on spelling out how to interpolate and (b) viewer implementations having to write code for the interpolation.

TLiebich commented 2 years ago

based on a group discussion, the following proposed solution was defined:

TLiebich commented 2 years ago

IfcIndexedPolygonalTextureMap

proposed resolution for the issue

TLiebich commented 2 years ago

and the EXPRESS code

ENTITY IfcTessellatedItem
 ABSTRACT SUPERTYPE OF (ONEOF
    (IfcIndexedPolygonalFace
    ,IfcTessellatedFaceSet))
 SUBTYPE OF (IfcGeometricRepresentationItem);
END_ENTITY;

ENTITY IfcIndexedPolygonalFace
 SUPERTYPE OF (ONEOF
    (IfcIndexedPolygonalFaceWithVoids))
 SUBTYPE OF (IfcTessellatedItem);
    CoordIndex : LIST [3:?] OF UNIQUE IfcPositiveInteger;  -- add UNIQUE
 INVERSE
    ToFaceSet : SET [1:?] OF IfcPolygonalFaceSet FOR Faces;
END_ENTITY;

ENTITY IfcIndexedPolygonalFaceWithVoids
 SUBTYPE OF (IfcIndexedPolygonalFace);
    InnerCoordIndices : LIST [1:?] OF LIST [3:?] OF UNIQUE IfcPositiveInteger;
END_ENTITY;

ENTITY IfcTessellatedFaceSet
 ABSTRACT SUPERTYPE OF (ONEOF
    (IfcPolygonalFaceSet
    ,IfcTriangulatedFaceSet))
 SUBTYPE OF (IfcTessellatedItem);
    Coordinates : IfcCartesianPointList3D;
 DERIVE
    Dim : IfcDimensionCount := 3;
 INVERSE
    HasColours : SET [0:1] OF IfcIndexedColourMap FOR MappedTo;
    HasTextures : SET [0:1] OF IfcIndexedTextureMap FOR MappedTo;  -- changed to [0:1]
END_ENTITY;

ENTITY IfcPolygonalFaceSet
 SUBTYPE OF (IfcTessellatedFaceSet);
    Closed : OPTIONAL IfcBoolean;
    Faces : LIST [1:?] OF IfcIndexedPolygonalFace;
    PnIndex : OPTIONAL LIST [1:?] OF IfcPositiveInteger;
END_ENTITY;

ENTITY IfcTriangulatedFaceSet
 SUPERTYPE OF (ONEOF
    (IfcTriangulatedIrregularNetwork))
 SUBTYPE OF (IfcTessellatedFaceSet);
    Normals : OPTIONAL LIST [1:?] OF LIST [3:3] OF IfcParameterValue;
    Closed : OPTIONAL IfcBoolean;
    CoordIndex : LIST [1:?] OF LIST [3:3] OF IfcPositiveInteger;
    PnIndex : OPTIONAL LIST [1:?] OF IfcPositiveInteger;
 DERIVE
    NumberOfTriangles : IfcInteger := SIZEOF(CoordIndex);
END_ENTITY;

ENTITY IfcTextureCoordinate
 ABSTRACT SUPERTYPE OF (ONEOF
    (IfcIndexedTextureMap
    ,IfcTextureCoordinateGenerator
    ,IfcTextureMap))
 SUBTYPE OF (IfcPresentationItem);
    Maps : LIST [1:?] OF IfcSurfaceTexture;
END_ENTITY;

ENTITY IfcIndexedTextureMap
 ABSTRACT SUPERTYPE OF (ONEOF
    (IfcIndexedTriangleTextureMap
    ,IfcIndexedPolygonalTextureMap))   -- added
 SUBTYPE OF (IfcTextureCoordinate);
    MappedTo : IfcTessellatedFaceSet;
    TexCoords : IfcTextureVertexList;
END_ENTITY;

ENTITY IfcIndexedTriangleTextureMap
 SUBTYPE OF (IfcIndexedTextureMap);
    TexCoordIndex : LIST [1:?] OF LIST [3:3] OF IfcPositiveInteger;  -- remove OPTIONAL
 WHERE  -- added where rule
    CorrectFaceSetType : 'IFC4x3.IFCTRIANGULATEDFACESET' IN TYPEOF(SELF\IfcIndexedTextureMap.MappedTo;
END_ENTITY;

ENTITY IfcIndexedPolygonalTextureMap   -- new entity
 SUBTYPE OF (IfcIndexedTextureMap);
    TexCoordinates : L[1:?] OF IfcTextureCoordinateIndices;
 WHERE
    CorrectFaceSetType : 'IFC4x3.IFCPOLYGONALFACESET' IN TYPEOF(SELF\IfcIndexedTextureMap.MappedTo;
END_ENTITY;

ENTITY IfcTextureCoordinateIndices  -- new entity
 SUBTYPE OF (IfcPresentationItem)
 SUPERTYPE OF (IfcTextureCoordinateIndicesWithVoids);
    TextCoordIndex : LIST [3:?] OF IfcPositiveInteger;
END_ENTITY;

ENTITY IfcTextureCoordinateIndicesWithVoids  -- new entity
 SUBTYPE OF (IfcTextureCoordinateIndices);
    InnerTextCoordIndex : LIST [1:?] OF LIST [3:?] OF IfcPositiveInteger;
END_ENTITY;
Moult commented 2 years ago

For the record, the diagram makes sense to me. I recently implemented the triangular counterpart and as far as I can tell this follows the same concept.

I see minor discrepancies between the names in the diagram "InnerCoordIndices" and "Text" vs "Tex" (Tex is what currently exists) but I assume these will be resolved.

I thought I was able to, and I really did try to think of a simpler diagram but I failed :) In particular its the voids in faces which make it very difficult to simplify. This proposal gets the +1 from me.

Moult commented 2 years ago

Just for the record, there was a proposal by @aothms to create an attribute relating IfcIndexedPolygonalFace and IfcTextureCoordinateIndices. This means:

  1. You don't need to define UVs for every single face
  2. You don't need to make sure your UV polygons are specified in exactly the same order as the polygonal face set polygons

This creates value because it means you don't need to split geometry up if you only want to texture a portion of it. This means you retain closed shells which makes quantification easier. The specification of exactly the same order also has advantages sense it makes it less rigid for implementers - they don't need to loop poly faces and UV faces at the same time.

Moult commented 2 years ago

A good note by Michalis is that a benefit of having the attribute that links polygonal faces with texture faces is that this decouples textures from representations - it allows UV coordinates to be reused by different representation items.

TLiebich commented 2 years ago

when cross checking with coloring, there is still the need to have an ordered list of faces for the polygonal face set. So we can't give up on this. So we should raise the question again, if we then should also go back to corresponding lists for textures?

IfcIndexedColourMap_PolygonalFaceSet

Moult commented 2 years ago

@TLiebich so you are worried about an inconsistency, where TriangleTextureMap relies on an ordered list of faces, IndexedColourMap relies on an ordered list, but IndexedPolygonalTextureMap does not?

aothms commented 2 years ago

I agree, we need the order, because the per-face colour indices are provided as a list.

I have another proposal though for IndexColourMap. Currently we support only face colours, not vertex colours. The latter is quite appropriate for dense meshes obtained from scans or other data. The vertex colours are then interpolated over the faces.

I think what we can do is make ColourIndex optional. In which case the ColourList should match the dimensions of the cartesian point list?

Thoughts?

TLiebich commented 2 years ago

we discussed this in the IFC4 time frame and decided at that time to not include color by vertex. However with infrastructure, meshes coming from surveys, etc. it could be time to revisit it.

however we need different solution - we can't rely on corresponding Cartesian point and RGB lists. Those could also be shared by several tesselations and using pxindex also allows to have a different sequence. Im my view, it would require to have a dedicated entity for colour by vertices.

TLiebich commented 2 years ago

regarding last question ny @Moult - yes, this is what I thought - having inconsistency in observing order and also direct links. Not sure whether it is crutial.

aothms commented 2 years ago

we can't rely on corresponding Cartesian point and RGB lists. Those could also be shared by several tesselations and using pxindex also allows to have a different sequence. Im my view, it would require to have a dedicated entity for colour by vertices.

I don't fully understand. I don't think it's realistic to expect [reuse with] the same positions with different colours, or vice versa, in the case of detailed survey meshes. Also I think defining them in the same order is a realistic requirement.

If you think it deserves a separate entity I can live with that, but I don't think the mechanism of optional indices is illegible, where if absent it maps to the vertices directly.

In the opengl world vertex colours is also much more common than face colours (which barely exists because you only have the vertices with position/normal/colour/uv and than faces only as indices without any possibility for attributes).

Moult commented 2 years ago

So is the proposal to go back to https://github.com/buildingSMART/IFC4.3.x-development/issues/135#issuecomment-1024100752 ? I do not have a strong opinion either way anymore.

Moult commented 2 years ago

If that's so, I am happy to give a +1 to it. It's consistent with the way the rest of the schema works, and it solves the problem. Could it be better? Perhaps, but also sounds like it'd be a lot of work to make everything consistent, which takes time which we don't have. Maybe better revisited after IFC4.3, but the current proposal is fine by me.

If nobody has objections, I'll mark this as decided on Monday.

Also fun to note this issue was brought up almost 2 years ago.

Moult commented 2 years ago

Happy days, marking this as decided so it can be implemented :)

Moult commented 2 years ago

It looks as though this is implemented. Closing :)