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

GLTFLoader v2 #9786

Merged
merged 23 commits into from
Oct 7, 2016
Merged

GLTFLoader v2 #9786

merged 23 commits into from
Oct 7, 2016

Conversation

richtr
Copy link
Contributor

@richtr richtr commented Sep 27, 2016

This is a complete refactor/rewrite of the existing glTF code in https://github.com/mrdoob/three.js/tree/dev/examples/js/loaders/gltf.

The new GLTFLoader.js includes all of the features of the legacy glTF code.

The benefits of this rewrite are as follows:

  • added async asset loading (buffers, textures, images) to @mrdoob's rewritten GLTFLoader.js.
  • better code readability and less code indirection
  • more compact internal glTF object representations (use THREE.js native objects as early and often as possible)
  • feature parity with the legacy glTF code (glTF v1.0 support w/ shaders, animations, skins, lights, extensions, etc) with a >60% reduction in size (1057 lines of code vs 2696 lines of code. 18KB vs 41KB when run through https://jscompress.com/)

@richtr
Copy link
Contributor Author

richtr commented Sep 27, 2016

The objective here is to deprecate https://github.com/mrdoob/three.js/tree/dev/examples/js/loaders/gltf in favor of a single glTF loader in the THREE.js project.

We should eventually move /examples/js/loaders/gltf to /examples/js/loaders/deprecated/gltf, update the docs to point to the new class and update the glTF example to use the new class.

I feel this could be a separate pull request once this new GLTFLoader.js is merged and battle ready.

/ping the original authors @tparisi @mrdoob for review.

@richtr
Copy link
Contributor Author

richtr commented Sep 28, 2016

@mrdoob do you prefer everything squashed in to one commit or is the current commit history acceptable?

@richtr
Copy link
Contributor Author

richtr commented Sep 28, 2016

Also, I'm wondering whether instead of adding /examples/webgl_loader_gltf_new.html whether this PR should instead fix /examples/webgl_loader_gltf.html to use this new loader (+ update the references in the GLTFLoader documentation). Or should that wait for a later PR?

@tparisi
Copy link
Contributor

tparisi commented Sep 28, 2016

@richtr this looks like a lovely refactor; but I haven't actually tried it - is there a fork I can grab to test everything out against it?

@richtr
Copy link
Contributor Author

richtr commented Sep 28, 2016

is there a fork I can grab to test everything out against it?

You can clone and switch to the branch that this is on:

git clone [email protected]:richtr/three.js.git three.js-richtr
cd three.js-richtr
git checkout feature/GLTFLoader

Then run a local web server using a one-liner within that root directory.

I've also mirrored this branch to here: https://richtr.github.io/three.js/examples/webgl_loader_gltf.html if you want to see how it works quickly online too.

I've used this PR successfully with all the models in https://github.com/KhronosGroup/glTF/tree/master/sampleModels but it would be good to get more testing in too.

@fraguada
Copy link
Contributor

fraguada commented Sep 29, 2016

@richtr I've been wanting migrate the json file I'm currently generating to glTF. I've tested a few .dae files converted with https://cesiumjs.org/convertmodel.html with your feature/GLTFLoader branch and they seem to work fine. This pull made me look into gltf and for a file that was 27,9mb with the Three.js format, it becomes 13,2mb in GLTF and 4,7mb zipped.

Geometry loaded fine, but textures as .png did not load for my files. I'm not sure if it is something in particular with my model. I don't get any errors that the .png are not found, but the meshes don't get a texture applied. Maybe a sample file generated from my process would help?

@fraguada
Copy link
Contributor

fraguada commented Sep 30, 2016

@richtr This is a simple box. It has a material with a diffuse channel as a jpg. The box itself loads, but I get errors such as:
ERROR: Missing fragment shader definition: tbox0FS GLTFLoader.js:1117:7 THREE.MeshPhongMaterial: 'uniforms' is not a property of this material. three.js:7248:6 THREE.Material: 'fragmentShader' parameter is undefined. three.js:7239:6 THREE.MeshPhongMaterial: 'vertexShader' is not a property of this material. three.js:7248:6

So I can see the box, but no material.

I've compared with the other glts files and while there are some differences, I am not sure which ones are causing the material to not be understood. The program and shaders seems to be defined in the same manner, and the glsl files have the same relationship in the directory to the .gltf file.

The process to make this: Create box in Rhino 3d, export as .dae, run through collada2gltf with no special options, define the model in your webgl_loader_gltf_new.html and run the scene. Other models show just fine.

@AVGP AVGP mentioned this pull request Sep 30, 2016
@AVGP
Copy link

AVGP commented Sep 30, 2016

Great work, @richtr!

I'm looking forward to see this land 👍

I also tested the loader with:

  • Data URIs ✓
  • Primitive without indices ✓
  • Primitive without mode ✗ [Pull request pending]

I'll see if I can find some more test models :)

Improves spec compliance: primitive.mode is optional
@richtr
Copy link
Contributor Author

richtr commented Sep 30, 2016

Thanks for the pull request @AVGP! This has been merged in to this branch now (so that 'Primitive without mode' is now working).

@fraguada I'm yet to look in to this issue but I will endeavor to do so shortly!

@mrdoob
Copy link
Owner

mrdoob commented Sep 30, 2016

@mrdoob do you prefer everything squashed in to one commit or is the current commit history acceptable?

No need to squash 😊

Also, I'm wondering whether instead of adding /examples/webgl_loader_gltf_new.html whether this PR should instead fix /examples/webgl_loader_gltf.html to use this new loader (+ update the references in the GLTFLoader documentation).

I would just fix /examples/webgl_loader_gltf.html.


this.crossOrigin = value;

},

parse: function ( json ) {
setBaseUrl: function( value ) {
Copy link
Owner

@mrdoob mrdoob Sep 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been renaming this method to setPath in other loaders:
https://github.com/mrdoob/three.js/blob/dev/examples/js/loaders/OBJLoader.js#L56-L60

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's good to have consistency here. Fixed in e0e7dbf.

@richtr
Copy link
Contributor Author

richtr commented Oct 3, 2016

Also, I'm wondering whether instead of adding /examples/webgl_loader_gltf_new.html whether >> this PR should instead fix /examples/webgl_loader_gltf.html to use this new loader (+ update >> the references in the GLTFLoader documentation).

I would just fix /examples/webgl_loader_gltf.html.

OK. I have moved /examples/webgl_loader_gltf_new.html to /examples/webgl_loader_gltf.html (thereby overwriting the old glTFLoader demo with the new GLTFLoader demo). Fixed in ec54400.

In addition, I have updated the documentation to reflect the API and point to this new GLTFLoader in commit 5b0bfba.

In db17203 I have moved the old /examples/js/loaders/gltf/* to /examples/js/loaders/deprecated/gltf/* to avoid confusion on which glTF loader to use in the future. I'd be happy to revert this commit if we want to keep that around for a while.

@AVGP
Copy link

AVGP commented Oct 3, 2016

@richtr It didn't seem to work with transparent KHR materials, but maybe I misread the spec when I did our exporter, but I think transparent is inside the values property and I could not find the loader setting opacity on the material - I attached a PR to your branch, please feel free to correct my assumptions if I went wrong somewhere.

@richtr
Copy link
Contributor Author

richtr commented Oct 3, 2016

@fraguada wrote:

@richtr This is a simple box. It has a material with a diffuse channel as a jpg. The box itself loads, but I get errors such as:
ERROR: Missing fragment shader definition: tbox0FS GLTFLoader.js:1117:7 THREE.MeshPhongMaterial: 'uniforms' is not a property of this material. three.js:7248:6 THREE.Material: 'fragmentShader' parameter is undefined. three.js:7239:6 THREE.MeshPhongMaterial: 'vertexShader' is not a property of this material. three.js:7248:6

So I can see the box, but no material.

It seems fine to work with no errors for me with the 'View-Perspective' camera you have set up in your glTF file:

screen shot 2016-10-04 at 1 50 05 am

I see no errors on the console when I load this (both glTF and glTF-MaterialsCommon versions provided work). I couldn't really get the other (orthographic) cameras working. Are they set up correctly in the glTF file?

@richtr richtr force-pushed the feature/GLTFLoader branch from 1db4fc2 to 8d254a4 Compare October 4, 2016 00:27
@richtr
Copy link
Contributor Author

richtr commented Oct 4, 2016

Actually @fraguada I should have asked which browser you were using. I did get the errors on the console you also encountered above in the latest Firefox. This has been fixed with 8d254a4.

@richtr
Copy link
Contributor Author

richtr commented Oct 4, 2016

@mrdoob When this lands I believe we can also close #9734 and #9803. Or they could be merged first, then this will deprecate those old interfaces.

@cx20
Copy link
Contributor

cx20 commented Oct 4, 2016

@richtr GLTFLoader v2 does not support Safari?

I tested old and new glTF loaders.
https://github.com/cx20/gltf-test

@mrdoob's GLTFLoader (basic loader) can display a duck in Safari.
However, GLTFLoader v2 (new loader) could not display it in Safari.

@tparisi
Copy link
Contributor

tparisi commented Oct 4, 2016

Are we back to the Safari bug? There is an issue filed here which was recently closed

#8381

@richtr did you pick up those changes?

@cx20
Copy link
Contributor

cx20 commented Oct 4, 2016

@tparisi I think that a closed Safari issue is #8973.
#8381 is different from the Safari issue.

@richtr
Copy link
Contributor Author

richtr commented Oct 5, 2016

@richtr GLTFLoader v2 does not support Safari?

It does but it looks like there is an issue specifically with glTF-Embedded demos in Safari (glTF and glTF-MaterialsCommon demos are fine). I'm hitting this issue: https://stackoverflow.com/questions/30920059/xhr-data-uri-doesnt-work-in-safari.

We currently run everything (including data URIs) through THREE.XHRLoader to save on parallel code paths (one for data URIs and another for http or https URLs).

I'd prefer that this issue would be fixed in THREE.XHRLoader so it works everywhere else this could be an issue too (i.e. don't run data URIs through XMLHttpRequest if a data URI is detected as input in THREE.XHRLoader).

Update:

The simplest test case I could come up that shows the issue is the following:

var loader = new XMLHttpRequest();
loader.open("GET", "data:text/plain,hello");
loader.send();

// works in Chrome & Firefox

// throws the following error in Safari on OS X: 
//     XMLHttpRequest cannot load data:text/plain,hello. Cross origin requests are only supported for HTTP.

@cx20
Copy link
Contributor

cx20 commented Oct 5, 2016

@richtr v2 is supports Safari. However, glTF-Embedded is not supports in Safari. I understood.
I thought to be a problem of judgment rather than a technical problem.
I thought that I should port mrdoob's stringToArrayBuffer function simply.
I think that is a good idea that an issue is fixed in THREE.XHRLoader.

@richtr
Copy link
Contributor Author

richtr commented Oct 5, 2016

I've created a new pull request to fix the issue documented above in THREE.XHRLoader here: #9823.

Using #9823 and this PR (#9786) together fixes the issue of loading and rendering glTF-Embedded models w/ Data URIs on Safari OS X and iOS.

@richtr
Copy link
Contributor Author

richtr commented Oct 6, 2016

Ready to go @mrdoob @tparisi. Please also take a look at #9823.

@richtr richtr force-pushed the feature/GLTFLoader branch from 2b8275d to 0ac2aa0 Compare October 6, 2016 21:42
… global library.* tracking. Other general refactoring
@mrdoob mrdoob merged commit 6ab1bdd into mrdoob:dev Oct 7, 2016
@mrdoob
Copy link
Owner

mrdoob commented Oct 7, 2016

Thaaanks! 😊

aardgoose pushed a commit to aardgoose/three.js that referenced this pull request 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
@richtr
Copy link
Contributor Author

richtr commented Oct 7, 2016

Thanks @mrdoob! (many thanks to @tparisi, @cx20, @fraguada, @AVGP too!)

aardgoose pushed a commit to aardgoose/three.js that referenced this pull request 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
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

Successfully merging this pull request may close these issues.

6 participants