fix for #1106

[rvandoosselaer]

when the image object has a ColorSpace value we pass it on, else we leave it at the default value.

[stephengold]

I had hoped for some review by a more senior monkey, but you seem to know what you're doing.

[rvandoosselaer]

I did a few tests with my own models and some free one’s I found online. I didn’t encounter any errors or strange behaviour, it seems to work.
However if people do find issues with it, I’ll be happy to revert the change or look for a solution.

[riccardobl]

Ok, sorry i didn't see this (it might be a good idea for the future to use an actual title for PRs, because when you start having several "Fix ISSUE NUMBER" it's easy to miss some).

My question is, what will happen if i have a matdef that defines a -LINEAR param, but the image is sRGB? Will this pr break the linearization by forcing the mat param to become sRGB ?

[rvandoosselaer]

Ok, sorry i didn't see this (it might be a good idea for the future to use an actual title for PRs, because when you start having several "Fix ISSUE NUMBER" it's easy to miss some).

My question is, what will happen if i have a matdef that defines a -LINEAR param, but the image is sRGB? Will this pr break the linearization by forcing the mat param to become sRGB ?

Hi @riccardobl, I wasn't aware of the -LINEAR option in a material definition file.
I just tested it, and if you specify -LINEAR in the materialDef, the colorspace will be set to linear.

This is my test case:
Gamma correction is enabled, the texture in the material is an sRGB one, the colorspace in the materialDef is set to '-LINEAR':

I also added a log statement printing out the colorspace of the material that is set on the model:
From the log:
Colorspace: Linear

Same test as above, but without the -LINEAR flag:

log:
Colorspace: sRGB

The first image is clearly gamma corrected (too bright) because of the colorspace, so all should still be working as expected.

Part of the relevant texture used:

[riccardobl]

Can you share the code?

[rvandoosselaer]

test:

TestMaterialLinearColorspace.java

public class TestMaterialLinearColorspace extends SimpleApplication {

    public static void main(String[] args) {
        new TestMaterialLinearColorspace().start();
    }

    @Override
    public void simpleInitApp() {
        AmbientLight ambientLight = new AmbientLight(ColorRGBA.White.mult(0.2f));
        DirectionalLight directionalLight = new DirectionalLight(new Vector3f(-0.2f, -1, -0.2f).normalizeLocal(), ColorRGBA.White);

        Spatial model = assetManager.loadModel("staff-0.j3o");
        Material material = assetManager.loadMaterial("colorspace-test.j3m");
        model.depthFirstTraversal(new SceneGraphVisitorAdapter() {
            @Override
            public void visit(Geometry geom) {
                geom.setMaterial(material);
                System.out.println("Colorspace: " + material.getTextureParam("DiffuseMap").getColorSpace());
            }
        });

        rootNode.addLight(ambientLight);
        rootNode.addLight(directionalLight);
        rootNode.attachChild(model);
    }
}

colorspace-test.j3m:

Material colorsapace-test : Common/MatDefs/Light/Lighting.j3md {
    MaterialParameters {
        Shininess: 0
        UseMaterialColors: true
        DiffuseMap: Flip texture.png
        Ambient: 1.0 1.0 1.0 1.0
        Diffuse: 1.0 1.0 1.0 1.0
        Specular: 0.0 0.0 0.0 1.0
    }
}

When running the testcase, I adapted this line in Lighting.j3md:31:

// Diffuse map
Texture2D DiffuseMap

to:

// Diffuse map
Texture2D DiffuseMap -LINEAR

If you want I can create a proper testcase for it, and put it in the jme3-examples module.

[riccardobl]

Oh ok, i see. I was mislead by the fact that both Materials and MatDefs keep a list of MatParamTexture. But the material will always look at the matdef list to see if it needs to switch the colorspace.

[rvandoosselaer]

Oh ok, i see. I was mislead by the fact that both Materials and MatDefs keep a list of MatParamTexture. But the material will always look at the matdef list to see if it needs to switch the colorspace.

Yep, indeed. You can always overwrite it if needed. This should only set the colorspace to the colorspace of the texture instead of defaulting to null.