Scaling reused geometry and reused colors

[muren400]

Hi,

we noticed a little bug regarding geometry reuse. When using IfcCartesianTransformationOperator3DnonUniform the parameters Scale, Scale2 und Scale3 where never processed and therefore not represented in ProductDef.mappingMatrix.

Commit e50cce8 (Scale reused geometry correctly) should resolve that issue.

The second commit 89d309c (Dont reuse geometry if colors dont match) addresses the fact, that geomtry and therefore color will be reused, even though the elements have a different colors. I fixed that by appending the elements color to the key used in representationMapToProduct. Not sure, if my solution is the prettiest though. Maybe theres a better solution?

Thanks,
Andreas

[hlg]

Great you figured this out! Yet, I have some comments:

General: Please don't use internal issue URLs, at least not in the title and such prominent place. Either create an issue in BIMserver repo and refer to that from the commit message (as you've done earlier) or put the issue description in the detailed commit message (not first line!). In any case, all relevant info should be reachable from the commit, not just the PR commentary, as there is no link from the commit to the PR and later will not be possible to see the reasoning behind the commit.

Commit e50cce8: How about uniform scale - with only first scale attribute set? If I am not mistaken, the code does not cover this case properly. Is this tested, at least with the Std example? I would prefer to keep code to access scale attributes near the matrix and see the factors directly where the matrix is defined, but you can keep it like this for now.

Regarding commit 89d309c, there is even a TODO in the code:

BIMserver/BimServer/src/org/bimserver/geometry/StreamingGeometryGenerator.java

Lines 356 to 357 in 6daa43a

	// TODO Geometries that are the same, but have different colors, will result in geometry with the wrong color, for example SampleModelErrorExportLight.ifc
	// So what we need to do is also compare the materials...

However the solution in the commit has some flaws: IfcPresentationStyleAssignment is deprecated in IFC4 Add2 TC1, the non-deprecated case with IfcPresentationStyle seems not to be supported with this code. I believe, there are other ways to assign colors to the representations via products and materials which are also not covered. Definitely, colors are populated by the render engine so we need to distinguish and group based on every possible "color source" that the render engine (e.g. IfcOpenShell) supports. But how to do that without doing the work that's actually delegated to the render engine? And, I agree, there could be a more elegant way to construct the key. I think this one is a bit tricky.

[muren400]

You're right I haven't covered uniform scaling. I'll add that and update the PR.

Not sure when I will get around to imporve the issue with the style assignments. Is it possible to skip a commit from a pull request or should I maybe create two separate PRs instead. I could also rename the commits while I'm at it.