search

opensourceBIM / BIMserver

The open source BIMserver platform
GNU Affero General Public License v3.0
1.56k stars 611 forks source link

The color Black is skipped in GeometryRunner #1326

Closed muren400 closed 2 months ago

muren400 commented 2 months ago

Hi,

there is this bit of code in GeometryRunner starting with line 341:

if (color.isBlack()) {
    continue;
}

From the git history it seems like this block was added for debugging purposes and shouldn't be there. This causes the model to be rendered incorrectly in BimSurfer, if an object has black material.

I have this example file.

It should look like this: image

In BIMView.ws it looks like this: image

In BimSurfer 3 it looks like this: image

I'm pretty sure, this can be resolved by just commenting out those 3 lines again, but maybe there's a reason they're there after all?

hlg commented 2 months ago

https://github.com/opensourceBIM/BIMserver/blob/9b5b49a016b698fb640b1474e475eb13b457cdcc/BimServer/src/org/bimserver/geometry/GeometryRunner.java#L341-L343

Interesting catch. I had a look at the respective code and agree that for this particular case, the BIMsurfer3 rendering would probably look as expected after removing those three lines. It seems like BIMsurfer V1 used in BIMvie.ws just uses the GeometryData#color attribute which contains the most-used color for one product's geometry and would continue to look like that.

I have a little doubt about triangles with no color assigned. Since the render engine returns byte buffers (see RenderEngineGeometry.java), it cannot easily communicate null values in other ways than with zeros. They should not be transparent either, so they will have alpha 1. If that is the case, then we have no chance of distinguishing explicit black from no color. This might be why the three lines of code are there on purpose, even though originally the isBlack method was just introduced for debugging purposes.

muren400 commented 2 months ago

It could just skip those colors in the material array and set the material index to -1;

It does that anyway if it can't find the material I think?

https://github.com/IfcOpenShell/IfcOpenShell/blob/fb803ac35d038d91823f67566b2a344abd6af8bf/src/ifcgeomserver/IfcGeomServer.cpp#L389-L394

hlg commented 2 months ago

Yes, it seems you are right, there was a change in IfcOpenShell to render geometries with materials that have no information on which color to use for its presentation with material index set to -1, hence no material assigned. Before that change, the IfcOpenShell behaviour was as I suspected: https://github.com/IfcOpenShell/IfcOpenShell/commit/f9b76ce022047d3e68bb5c5dc1c5019f239221ff?diff=unified&w=1. Can you verify with a sample with a no-color material assigned? If that works fine, I think those three lines can indeed be deleted.

muren400 commented 2 months ago

Yes, when running ClientRunnner.java for this file, the colors buffer is empty and MaterialIndices is filled with -1.