Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

glTF Loader issue - duck is dark #8381

Closed
6 tasks done
cx20 opened this issue Mar 16, 2016 · 19 comments
Closed
6 tasks done

glTF Loader issue - duck is dark #8381

cx20 opened this issue Mar 16, 2016 · 19 comments

Comments

@cx20
Copy link
Contributor

cx20 commented Mar 16, 2016

Description of the problem
  1. Show glTF loader example (http://threejs.org/examples/webgl_loader_gltf.html)
  2. Model: [Duck]
  3. Shaders: [ ] Use built-in (checked is off)
Result

duck.gltf (http://threejs.org/examples/models/gltf/duck/glTF/duck.gltf) is dark.

image

...

Three.js version
  • r75
Browser
  • Chrome 49.0.2623.87 m
  • Firefox 44.0.2
  • Internet Explorer 11.162.10586.0
  • Edge 25.10586.0.0 (EdgeHTML 13.10586)
OS
  • Windows 10
Hardware Requirements (graphics card, VR Device, ...)

ThinkPad X201 + Intel HD Graphics

@RemusMar
Copy link
Contributor

nvidia
No problems on Firefox 45 + Nvidia graphics card

@WestLangley
Copy link
Collaborator

Try the dev branch.

http://threejsworker.com/api/branch/dev/examples/webgl_loader_gltf.html

Although, the models in this link do not render for me using Safari 9.0.3 on OS X 10.11.3.

@cx20
Copy link
Contributor Author

cx20 commented Mar 16, 2016

duck

Windows 10 + Chrome 49 + GeForce GTX 660

@mrdoob
Copy link
Owner

mrdoob commented Mar 16, 2016

It seems to work fine here...

screenshot from 2016-03-16 16-38-42

Fedora 23 (Wayland) + Chrome 49 + Intel Iris Graphics 540

/ping @tparisi

@tparisi
Copy link
Contributor

tparisi commented Mar 16, 2016

yeah I'm not seeing that

@cx20
Copy link
Contributor Author

cx20 commented Mar 17, 2016

I made a simple example.

http://jsdo.it/cx20/GLug

and, I tried to change the variables in the Firefox Shader Editor.

specularIntensity = max(0., pow(max(dot(normal, h), 0.), u_shininess)) * attenuation;
//specularIntensity = max(0., pow(max(dot(normal, h), 0.), 1.0)) * attenuation;

image

//specularIntensity = max(0., pow(max(dot(normal, h), 0.), u_shininess)) * attenuation;
specularIntensity = max(0., pow(max(dot(normal, h), 0.), 1.0)) * attenuation;

image

image

ThinkPad X201 + Intel HD Graphics + Windows 10 + Firefox 45.0

@mrdoob
Copy link
Owner

mrdoob commented Mar 17, 2016

Ah yeah, we fixed these issues in the past in the built-in shaders.

@tparisi welcome to the lovely world of gpu discrepancies support 😉

@emadurandal
Copy link

Hello everyone.

I have the same problem with him example ( http://jsdo.it/cx20/GLug ).

My environment is Windows10, Chrome 49.0.2623.87 m, Quadro K2000D.

But, I'm not seeing this problem using MacBook Pro (Retina, 15-inch, Mid 2015, Radeon R9 M370X).

mysterious... anyway, I guess this problem happens in Windows.
(It's cause by a difference between ANGLE webgl implementation and others?)

@mrdoob
Copy link
Owner

mrdoob commented Mar 17, 2016

@tparisi Check #7252.

@tparisi
Copy link
Contributor

tparisi commented Mar 17, 2016

@pjcozzi looks like we some issues w the GLSL being generated by the converter

@pjcozzi
Copy link

pjcozzi commented Mar 21, 2016

@tparisi can you try putting this workaround in JavaScript for the SHININESS semantic: #7252 (comment)?

@cx20 can you try to drag and drop the duck model (original COLLADA and textures or glTF with embedded resources) into the Cesium viewer so we can isolate if this is the shaders: http://cesiumjs.org/convertmodel.html?

@tparisi
Copy link
Contributor

tparisi commented Mar 21, 2016

@pjcozzi will do

@WestLangley
Copy link
Collaborator

Yes. I expect u_shininess has been set to zero. And that is causing the problem.

mrdoob added a commit that referenced this issue Mar 21, 2016
Dev - trying shininess hack for #8381
@mrdoob
Copy link
Owner

mrdoob commented Mar 21, 2016

#8429 should fix this.

@cx20
Copy link
Contributor Author

cx20 commented Mar 22, 2016

I tried fixed glTFLoader.js. But, I failed.

https://github.com/tparisi/three.js/blob/6663990f65b593c925979f5d291dbed6d5c07166/examples/js/loaders/gltf/glTFLoader.js#L167

However, after changing following undefined to 1.0 with a debugger, it was colored..

image

image

@tparisi
Copy link
Contributor

tparisi commented Mar 22, 2016

@mrdoob how to proceed? I guess I can scare up a Windows box to test this on...

@emadurandal
Copy link

Hi.

I'd like to add additional infomation to @cx20's comment.

@cx20 and I tried latest dev:branch (pullrequest #8429 included), but the same problem still happened.

@cx20 set breakpoint at glTFLoader.js#L167. Then, u_shininess's value was undefined in all browsers.

Yes. the value is undefined. But...

My Windows's firefox: works fine.
My Windows's chrome: blinking.
My Windows's Edge: black out.
@cx20's Windows's firefox: black out.
@cx20's Windows's chrome: blinking.
@cx20's Windows's Edge: black out.

According to this results, When a uniform value is set to undefined, the behavior is very undefined! (depend on WebGL implementation).

@cx20
Copy link
Contributor Author

cx20 commented Aug 21, 2016

I found the cause that shininess became undefined.

After making the following modifications, a value came to be set in shininess variable.

https://github.com/mrdoob/three.js/blob/dev/examples/js/loaders/gltf/glTFLoader.js#L824

  • before
    case WebGLRenderingContext.FLOAT :
        utype = "f";
        uvalue = shaderParam.value;
        if (pname == "transparency") {
            var USE_A_ONE = true; // for now, hack because file format isn't telling us
            var opacity =  USE_A_ONE ? value : (1.0 - value);
            uvalue = opacity;
            params.transparent = true;                                    
        } // Case, uvalue except "transparency" are not set pname.
        break;
    case WebGLRenderingContext.FLOAT_VEC2 :
  • after
    case WebGLRenderingContext.FLOAT :
        utype = "f";
        uvalue = shaderParam.value;
        if (pname == "transparency") {
            var USE_A_ONE = true; // for now, hack because file format isn't telling us
            var opacity =  USE_A_ONE ? value : (1.0 - value);
            uvalue = opacity;
            params.transparent = true;
        } else if (value) {
            uvalue = value;
        }
        break;
    case WebGLRenderingContext.FLOAT_VEC2 :

WebGL Inspector

  • before
    image
  • after
    image

cx20 added a commit to cx20/three.js that referenced this issue Aug 21, 2016
@cx20 cx20 mentioned this issue Oct 1, 2016
richtr added a commit to richtr/three.js that referenced this issue Oct 3, 2016
- Replace .set(...) with .fromArray(...) where possible for THREE.js Math objects (thanks @mrdoob)
- Include fix for mrdoob#8381 (thanks @cx20)
- Move THREE.GLTFShaders to THREE.GLTFLoader.Shaders
- Move THREE.GLTFAnimator to THREE.GLTFLoader.Animations
- Rename .setBaseUrl(...) to .setPath(...)
- Update demo page 'examples/webgl_loader_gltf_new.html' with renamed APIs
richtr added a commit to richtr/three.js that referenced this issue Oct 3, 2016
richtr added a commit to richtr/three.js that referenced this issue Oct 3, 2016
@mrdoob mrdoob closed this as completed in 6ab1bdd Oct 7, 2016
aardgoose pushed a commit to aardgoose/three.js that referenced this issue Oct 7, 2016
* Initial GLTFLoader updates so it will also obtain async resources over the network

* First take at adding shader support to new GLTFLoader

* First working custom shaders version of GLTFLoader.js

* Fully working custom GLTF shaders + general library fix-ups

* Add skins, animations, lights, extensions and continue to refactor the library

* Refactor new GLTFLoader (seperate glTF parsing and internal library building)

* Fix GLSL shader update binding to point at the correct nodes + more general refactoring

* Make _each thenable and refactor dependencies graph + other minor fixes

* Improves spec compliance: primitive.mode is optional

* Update GLTFLoader v2 with feedback on PR: mrdoob#9786

- Replace .set(...) with .fromArray(...) where possible for THREE.js Math objects (thanks @mrdoob)
- Include fix for mrdoob#8381 (thanks @cx20)
- Move THREE.GLTFShaders to THREE.GLTFLoader.Shaders
- Move THREE.GLTFAnimator to THREE.GLTFLoader.Animations
- Rename .setBaseUrl(...) to .setPath(...)
- Update demo page 'examples/webgl_loader_gltf_new.html' with renamed APIs

* Replace 'examples/webgl_loader_gltf.html' with new version of GLTFLoader

* Update documentation at /docs/?q=gltf#Reference/Loaders/GLTFLoader to reference new GLTFLoader

* Move /examples/js/loaders/gltf/* to /examples/js/loaders/deprecated/gltf/*

* Actually add a fix for mrdoob#8381

* Re-fix mrdoob#8381 in a more logical way

* Update material parameter checking and validation.

* Fix issue whereby the spec says doubleSided and transparent should be in the 'values' section but every use of these params I've seen in the wild has been outside this section

* Ambient is not a valid properties for THREE.js materials

* THREE textures have flipY set to true by default. By setting to false we can remove double flip from GLTFLoader

* Fix issue where Firefox Promise then callback can not correctly keep track of its key

* Remove debugging from examples/loaders/deprecated/gltf/glTFLoader.js

* Refactor waitForDependencies in to _withDependencies. Remove need for global library.* tracking. Other general refactoring
aardgoose pushed a commit to aardgoose/three.js that referenced this issue Oct 7, 2016
* Initial GLTFLoader updates so it will also obtain async resources over the network

* First take at adding shader support to new GLTFLoader

* First working custom shaders version of GLTFLoader.js

* Fully working custom GLTF shaders + general library fix-ups

* Add skins, animations, lights, extensions and continue to refactor the library

* Refactor new GLTFLoader (seperate glTF parsing and internal library building)

* Fix GLSL shader update binding to point at the correct nodes + more general refactoring

* Make _each thenable and refactor dependencies graph + other minor fixes

* Improves spec compliance: primitive.mode is optional

* Update GLTFLoader v2 with feedback on PR: mrdoob#9786

- Replace .set(...) with .fromArray(...) where possible for THREE.js Math objects (thanks @mrdoob)
- Include fix for mrdoob#8381 (thanks @cx20)
- Move THREE.GLTFShaders to THREE.GLTFLoader.Shaders
- Move THREE.GLTFAnimator to THREE.GLTFLoader.Animations
- Rename .setBaseUrl(...) to .setPath(...)
- Update demo page 'examples/webgl_loader_gltf_new.html' with renamed APIs

* Replace 'examples/webgl_loader_gltf.html' with new version of GLTFLoader

* Update documentation at /docs/?q=gltf#Reference/Loaders/GLTFLoader to reference new GLTFLoader

* Move /examples/js/loaders/gltf/* to /examples/js/loaders/deprecated/gltf/*

* Actually add a fix for mrdoob#8381

* Re-fix mrdoob#8381 in a more logical way

* Update material parameter checking and validation.

* Fix issue whereby the spec says doubleSided and transparent should be in the 'values' section but every use of these params I've seen in the wild has been outside this section

* Ambient is not a valid properties for THREE.js materials

* THREE textures have flipY set to true by default. By setting to false we can remove double flip from GLTFLoader

* Fix issue where Firefox Promise then callback can not correctly keep track of its key

* Remove debugging from examples/loaders/deprecated/gltf/glTFLoader.js

* Refactor waitForDependencies in to _withDependencies. Remove need for global library.* tracking. Other general refactoring
@cx20
Copy link
Contributor Author

cx20 commented Oct 7, 2016

Duck is now bright. Thank you everyone. 👍

https://cdn.rawgit.com/mrdoob/three.js/dev/examples/webgl_loader_gltf.html

image

ThinkPad X201 + Intel HD Graphics

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants