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

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

Closed
TLiebich opened this issue Jan 12, 2022 · 21 comments
Closed

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

TLiebich opened this issue Jan 12, 2022 · 21 comments
Labels
allocated-core implemented Issue closed: Proposal implemented

Comments

@TLiebich
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator

aothms commented Jan 12, 2022 via email

@TLiebich
Copy link
Collaborator Author

TLiebich commented Jan 13, 2022

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
Copy link
Collaborator

aothms commented Jan 13, 2022

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
Copy link
Collaborator Author

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

  • add a new entity to cover texture coordinate mappings for polygonal planes
  • define u,v texture coordinates also for inner holes
  • allow a single texture to be associated to a surface (not multiple with undefined order)
  • at IfcIndexedTriangleTextureMap make attribute TexCoordinate mandatory

@TLiebich
Copy link
Collaborator Author

IfcIndexedPolygonalTextureMap

proposed resolution for the issue

@TLiebich
Copy link
Collaborator Author

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 Moult added allocated-core proposal Step 2: A well defined proposal has been put forward labels Feb 7, 2022
@Moult
Copy link
Collaborator

Moult commented Feb 7, 2022

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
Copy link
Collaborator

Moult commented Feb 7, 2022

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
Copy link
Collaborator

Moult commented Feb 8, 2022

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
Copy link
Collaborator Author

TLiebich commented Feb 8, 2022

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
Copy link
Collaborator

Moult commented Feb 9, 2022

@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
Copy link
Collaborator

aothms commented Feb 9, 2022

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
Copy link
Collaborator Author

TLiebich commented Feb 9, 2022

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
Copy link
Collaborator Author

TLiebich commented Feb 9, 2022

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
Copy link
Collaborator

aothms commented Feb 9, 2022

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).

aothms added a commit that referenced this issue Feb 9, 2022
@Moult
Copy link
Collaborator

Moult commented Feb 10, 2022

So is the proposal to go back to #135 (comment) ? I do not have a strong opinion either way anymore.

@Moult
Copy link
Collaborator

Moult commented Feb 11, 2022

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
Copy link
Collaborator

Moult commented Feb 19, 2022

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

@Moult Moult added decided Step 3: The proposal has been approved and removed proposal Step 2: A well defined proposal has been put forward labels Feb 19, 2022
@aothms aothms added the affects-uml Issues in decided state that require changes to the UML model to be applied label Feb 20, 2022
aothms added a commit that referenced this issue Feb 20, 2022
aothms added a commit that referenced this issue Feb 20, 2022
aothms added a commit that referenced this issue Feb 20, 2022
@Moult
Copy link
Collaborator

Moult commented Mar 1, 2022

It looks as though this is implemented. Closing :)

@Moult Moult added implemented Issue closed: Proposal implemented and removed decided Step 3: The proposal has been approved labels Mar 1, 2022
@Moult Moult closed this as completed Mar 1, 2022
@Moult Moult removed the affects-uml Issues in decided state that require changes to the UML model to be applied label Mar 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
allocated-core implemented Issue closed: Proposal implemented
Projects
None yet
Development

No branches or pull requests

3 participants