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 e50cce819eea1e869375ee9b9a0f1266e1956e8d (Scale reused geometry correctly) should resolve that issue.

The second commit 89d309cd4fd090154b7cc4f1c19f17834b5a795c (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 https://github.com/opensourceBIM/BIMserver/commit/e50cce819eea1e869375ee9b9a0f1266e1956e8d: 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 https://github.com/opensourceBIM/BIMserver/commit/89d309cd4fd090154b7cc4f1c19f17834b5a795c, there is even a TODO in the code:

https://github.com/opensourceBIM/BIMserver/blob/6daa43afdda3d25ebbd41e0d0de89361063e7142/BimServer/src/org/bimserver/geometry/StreamingGeometryGenerator.java#L356-L357

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.

[hlg]

You can arbitrarily modify the PR branch including commit removal (reset and commit again) or amending commit titles and descriptions. You would have to force-push which you would never do otherwise, but is ok for a PR branch. When rebasing commits are rewritten anyway. The links to commits in your first comment will probably break. Two new PRs and closing this one would also be fine.

[muren400]

Opened another pull request for the scaling issue. Still didn't come around to improve the color issue.

By the way I'm working on an alternative to the IfcGeomServer from IfcOpenShell, take a look if you're interested.

here and here

It's using this as a render engine, which seems to perform better on large pre triangulated geometries.