pissang / clay-viewer

3D model viewer with high quality rendering and glTF2.0/GLB export
https://pissang.github.io/clay-viewer/editor/
BSD 3-Clause "New" or "Revised" License
771 stars 97 forks source link

Suzanne.gltf is displayed dark #7

Closed cx20 closed 6 years ago

cx20 commented 6 years ago

I tried to display the following model. https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/Suzanne

However, it appears to be quite dark.

QMV.js + Suzanne.gltf result: image

I tried setting the light intensity to 10 times, and it got a little brighter.

    viewer.setMainLight({
        intensity: 1*10,
        color: '#fff',
        alpha: 45,
        beta: 45
    });
    viewer.setAmbientLight({
        intensity: 0.8*10
    });

However, things like molds are now displayed.

image

Perhaps the usage of the light may be incorrect. In that case, please tell me how to set it.

pissang commented 6 years ago

I think it works as expected.

The material of Suzanne is gloss metal. Which has no diffuse reflection, only specular reflection. So ambient light, which only contributes to the diffuse part, have no effects at all. No matter what the intensity is. Directional light can also only contribute a little specular spot since the surface is gloss.

For gloss metal material like this. The best way is to use image based lighting. In QMV.js setAmbientCubemapLight({ intensity: 1, texture: 'someHdrTexture.hdr' }) is for adding the IBL.

You can also have a try on the online viewer http://pissang.github.io/qtek-model-viewer/editor

screen shot 2017-11-11 at 1 02 35 am
cx20 commented 6 years ago

I confirmed that Suzanne.gltf will be displayed by using setAmbientCubemapLight instead of setAmbientLight.

/*
    // Set ambient light options
    viewer.setAmbientLight({
        // Ambient light intensity
        intensity: 0.8
    });
*/
    // Set ambient cubemap light options
    viewer.setAmbientCubemapLight({
        // Ambient cubemap light intensity
        intensity: 0.8,
        // Ambient Cubemap Texture
        texture: '../../textures/hdr/pisa.hdr'
    });

image

However, in that case, it seems that Triangle.gltf is black and nothing is displayed. https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/Triangle

I want to display all the samples with the same light setting, is it difficult? In other libraries (Three.js) etc., all samples are displayed with the same light setting. https://github.com/cx20/gltf-test

pissang commented 6 years ago

@cx20 Thanks for the further feedback. The triangle glTF is black because it has no surface normals. In https://github.com/pissang/qtek/commit/78d20e20c4a49da9964f30ad3e26e688035eeae9 I add normal generation if mesh loose it to fix this problem.

About light setting, simple models and more complex models can have one light setting with directional light and ambient light. But PBR models and further PBR models need extra ambientCubeMap for PBR rendering, which I believe also can't be avoided in other libraries.

cx20 commented 6 years ago

@pissang Thank you for support of normal generation. However, there seems to be a problem with this correspondence. When calling generateVertexNormals, Triangle.gltf will not get an error, but it seems that TriangleWithoutIndices.gltf got an error.

QMV.js:17705 
Uncaught TypeError: Cannot read property 'length' of null
    at sub.generateVertexNormals (QMV.js:17705)
    at sub.<anonymous> (QMV.js:18962)
    at Array.forEach (<anonymous>)
    at Object.each (QMV.js:442)
    at sub._parseMeshes (QMV.js:18865)
    at afterLoadBuffer (QMV.js:18381)
    at checkLoad (QMV.js:18326)
    at QMV.js:18342
    at Object.onload (QMV.js:18446)
    at XMLHttpRequest.xhr.onload (QMV.js:11376)
        for (var f = 0; f < indices.length;) {
                                    ^^^^^^

Sorry if it seems to be a known problem.

pissang commented 6 years ago

My fault, didn't handle the case without indices. Actually I just saw it's still in the TODO list. Will fix that then. Thanks again!

pissang commented 6 years ago

@cx20 I think it's ok now:)

cx20 commented 6 years ago

@pissang I confirmed that TriangleWithoutIndices.gltf will be displayed as well. I would like to close this Issue.