Updated LegacyGLTFLoader.js shader parsing #13339

1d2d3d · 2018-02-15T21:10:20Z

This is a recap of #13332 - At my workplace, we need to deliver some assets with shaders in GLTF 1.0 format. Since they need attributes that weren't handled by default in the GLTF 1.0 loader, I modified the LegacyGLTFLoader.js file to handle the extra attributes (on this git branch), and tested it by slightly modifying the three.js editor (on another git branch). I realize they're working on adding shader support to GLTF 2, but in the meantime, if we want to use our own shaders, GLTF 1 is all we've got.

The legacy loader now seems to do a better job of loading the sample GLTF 1.0 files (e.g., BoxTextured.gltf and Duck.gltf) on the Khronos web site. This loader fixes two issues; one, handling of indexed vertex attributes with types such as COLOR_1 and COLOR_2, and two, passing user-defined vertex attributes through to three.js.

The argument order for the parse() function's second and third arguments was reversed between the editor's and the legacy loader's code, so in order to test with the editor, I patched that. A separate branch with the editor changes that allow it to open both GLTF 1 and 2 files is in #13333. Please note that branch assumes the parse() argument-swap implemented in this branch's LegacyGLTFLoader.js.

1d2d3d · 2018-02-15T21:17:54Z

This is an updated version of #13332 that aims to modify the existing LegacyGLTFLoader code as little as possible.

1d2d3d · 2018-02-15T21:21:07Z

@donmccurdy, I like your GLTF viewer - I see it loads BoxTextured.gltf just fine. You must be doing something different than the three.js editor does, because I did a quick check on my (reasonably recent nvidia laptop) hardware, and it seems for both GLTF versions 1.0 and 2.0, though BoxTextured.gltf and Duck.gltf both load, they each have an attractive but incorrect all-black material. The code included in this patch fixes that issue for the editor's loading of BoxTextured.gltf and Duck.gltf versions 1.0. Do you want me to file a bug report for the 2.0 versions?

1d2d3d · 2018-02-15T21:26:08Z

(Actually, this code doesn't fix the editor directly, but when I modded the editor to use LegacyGLTFLoader, the models loaded with their textures intact.)

1d2d3d · 2018-02-15T21:37:35Z

I think what this patch fixes is when there are user-defined attributes in a shader (anything that's not position, normal, color, texture coordinate, joint or weight), the loader is forgetting to add them.

donmccurdy · 2018-02-16T19:50:10Z

examples/js/loaders/deprecated/LegacyGLTFLoader.js

@@ -714,11 +714,15 @@ THREE.LegacyGLTFLoader = ( function () {

 			switch ( semantic ) {

+				case "POSITION_0":
+				case "POSITION0":


POSITION0 syntax is not permitted by the glTF spec:

Valid attribute semantic property names include POSITION, NORMAL, TEXCOORD, COLOR, JOINT, and WEIGHT. Attribute semantic property names can be of the form [semantic]_[set_index], e.g., TEXCOORD_0, TEXCOORD_1, etc.

So I don't think that case needs to be added here, similarly below.

I'll submit yet another version that leaves this section alone entirely.

By the way, you can also just make new commits on your branch and git push, they'll appear in this PR automatically. If it's easier to start a new branch and new PR that's fine too, though.

Oh, that's good to know.

donmccurdy · 2018-02-16T19:53:09Z

examples/js/loaders/deprecated/LegacyGLTFLoader.js

@@ -1616,26 +1662,74 @@ THREE.LegacyGLTFLoader = ( function () {
 									geometry.addAttribute( 'uv', bufferAttribute );
 									break;

-								case 'TEXCOORD_1':
-									geometry.addAttribute( 'uv2', bufferAttribute );
-									break;


I would keep this TEXCOORD_1 case here (see further comments below).

donmccurdy · 2018-02-16T19:55:17Z

examples/js/loaders/deprecated/LegacyGLTFLoader.js

-				case 'TEXCOORD_1':
-
-					shaderText = shaderText.replace( regEx, 'uv2' );
-					break;


See comments below, I would keep this TEXCOORD_1 case.

donmccurdy · 2018-02-16T19:56:13Z

examples/js/loaders/deprecated/LegacyGLTFLoader.js

+						}
+
+					}
+


I think this particular default: block can be removed, actually — it is not necessary to rename attributes that three.js does not make direct use of. More explanation below.

donmccurdy · 2018-02-16T19:59:59Z

examples/js/loaders/deprecated/LegacyGLTFLoader.js

+										geometry.addAttribute( attributeName, bufferAttribute );
+
+									}
+


If we decide not to rename attributes three.js does not use itself, I believe this default: block can be written more simply:

geometry.addAttribute( attributeId, bufferAttribute );

I imagine you're renaming attributes for consistency with the existing code, but it isn't strictly necessary for attributes whose names don't have inherent meaning in three.js, and I'd be more comfortable merging this PR if we can avoid adding two new nested switch statements to accomplish it.

attributeId represents constants like 'POSITION' and 'TEXCOORD_0', but the first argument to geometry.addAttribute() needs to be the name of the corresponding variable in the shader text. That's why I need to do a couple handstands to find the name of the variable in the GLTF file through the material's technique. But I can simplify the code to do only that, which will be a maybe five-line change total.

Oh I see, thanks!

donmccurdy · 2018-02-16T20:00:34Z

Thanks for the updated PR! Added some comments inline.

donmccurdy · 2018-02-16T20:04:00Z

You must be doing something different than the three.js editor does, because I did a quick check on my (reasonably recent nvidia laptop) hardware, and it seems for both GLTF versions 1.0 and 2.0, though BoxTextured.gltf and Duck.gltf both load, they each have an attractive but incorrect all-black material. The code included in this patch fixes that issue for the editor's loading of BoxTextured.gltf and Duck.gltf versions 1.0. Do you want me to file a bug report for the 2.0 versions?

I think importing models that require multiple files, like external textures, is a bit experimental in the threejs editor maybe? Using a .ZIP might work... I have some workarounds for my own viewer there. Otherwise see #10800 and #6680, and if either comment there or open a new issue 👍

takahirox · 2018-02-16T21:12:45Z

You must be doing something different than the three.js editor does, because I did a quick check on my (reasonably recent nvidia laptop) hardware, and it seems for both GLTF versions 1.0 and 2.0, though BoxTextured.gltf and Duck.gltf both load, they each have an attractive but incorrect all-black material. The code included in this patch fixes that issue for the editor's loading of BoxTextured.gltf and Duck.gltf versions 1.0. Do you want me to file a bug report for the 2.0 versions?

Any editor's console error message for 2.0 models?
And, just in case, did you add lights on the editor?

I suppose the editor also fails to load external .bin and then renders nothing if loading multiple files is the reason...

1d2d3d · 2018-02-21T19:23:55Z

You're totally right, I just needed to add a light. So, never-mind! But yes, it'll be great if by default the editor gets the support files from the same location as the gltf.

…x attributes

mrdoob · 2018-02-22T00:46:58Z

@donmccurdy looks good?

donmccurdy · 2018-02-22T00:53:09Z

editor/js/Loader.js

+
+				try {
+
+					extensions[ EXTENSIONS.KHR_BINARY_GLTF ] = new GLTFBinaryExtension( contents );


extensions is a copy/paste artifact, I imagine? In fact this line will throw a specific error if the asset has a version <=2. If it parses successfully it is glTF 2.0.

Oops, I see now GLTFBinaryExtension is defined in both LegacyGLTFLoader and GLTFLoader, within closures... that seems like an issue, will this code run correctly?

Okay, will take that section out entirely.

donmccurdy · 2018-02-22T00:59:57Z

I need to double-check the spec before approving, but changes to LegacyGLTFLoader look generally fine.

There are issues with the way LegacyGLTFLoader is added to the editor, I've left some comments there. But more generally, first, @mrdoob what do you think about legacy glTF 1.0 support in the editor?

mrdoob · 2018-02-22T01:02:44Z

But more generally, first, @mrdoob what do you think about legacy glTF 1.0 support in the editor?

I think it's a good thing; it'll help people rescue files. I still have to add support to .gltf + .bin, but that'll come later.

donmccurdy · 2018-02-22T01:03:12Z

I should ask, too, are you just using the editor as a quick way of testing your glTF 1.0 files? If that's the case, I might rather create a special gltf-viewer-legacy.donmccurdy.com page, or something like that. For example I wouldn't want people to test a model in the three.js editor, see that it works, and assume they can upload it to Facebook...

EDIT: Just saw comment above. Sounds good, pending suggested changes on the PR. Although people won't be able to convert files using GLSL to glTF 2.0 this way...

donmccurdy · 2018-02-22T01:14:30Z

examples/js/loaders/deprecated/LegacyGLTFLoader.js

+								default:
+									var material = json.materials[ primitive.material ];
+									var attributeNames = json.techniques[ material.technique ].attributes;
+									var attributeName = Object.keys( attributeNames )[ attributeIndex ];


This depends on primitive.attributes having the same order as technique.attributes, which is not guaranteed. Would this work?

var attributeMap = invertObject( json.techniques[ material.technique ].attributes ); var attributeName = attributeMap[ attributeId ]; function invertObject ( input ){ var output = {}; for( var key in input ){ output[ input[ key ] ] = key; } return output; }

Ugh, I see I've missed the indirection on the parameters array... I guess it is not quite this simple. I haven't got time right now but may need to take another look in a day or two. 😕

Yes, that's right. I haven't looked at the spec to see if that's always the case, just saw it held in all the glTF files I looked at. If that's really not guaranteed, then yeah, the complexity increases.

I will attempt to update that to look through the attributes to find a match with the current attributeId.

donmccurdy · 2018-02-25T02:43:26Z

editor/js/Loader.js

+
+			var magic = THREE.LoaderUtils.decodeText( new Uint8Array( contents, 0, 4 ) );
+
+			if ( magic === 'glTF' ) {


probably want magic !== 'glTF' here.

Does this need to be resolved?

It does yeah; the first four bytes are glTF for both 1.0 and 2.0. The next four bytes are a version, which must be 1 in glTF 1.0. Will add a comment below.

donmccurdy · 2018-02-25T02:59:59Z

examples/js/loaders/deprecated/LegacyGLTFLoader.js

@@ -1634,6 +1634,20 @@ THREE.LegacyGLTFLoader = ( function () {
 									geometry.addAttribute( 'skinIndex', bufferAttribute );
 									break;

+								default:
+									var material = json.materials[ primitive.material ];
+									var parameters = json.techniques[ material.technique ].parameters;


The primitive may not have a material, the material may not have a technique, and the technique may not have parameters. In any of those cases we should break rather than throwing an error, I think.

donmccurdy · 2018-02-25T03:00:34Z

Couple safety checks to add, otherwise this is looking close. Is there a model you can share to verify this works, by any chance?

donmccurdy · 2018-02-25T07:32:04Z

Hm. It turns out that glTF 1.0 does not allow custom attributes, and so this is not technically valid... I'm leaning toward saying (1) adding glTF 1.0 support to the Editor is fine, but (2) adding support for custom semantics should be left to your own separate fork of the loader. If your goal is to have a quick way of testing glTF 1.0 files, I'd suggest forking this viewer and swapping out the loader.

glTF 2.0 does support custom attributes, so this should not be an issue there.

mrdoob · 2018-02-27T02:40:39Z

Hm. It turns out that glTF 1.0 does not allow custom attributes, and so this is not technically valid... I'm leaning toward saying (1) adding glTF 1.0 support to the Editor is fine, but (2) adding support for custom semantics should be left to your own separate fork of the loader.

Agreed. Do you mind leaving the custom semantics part for a different PR?

1d2d3d · 2018-02-27T15:11:39Z

Custom semantics, no. But custom attributes should definitely be allowed. So for instance, a custom attribute with name myColor and semantic "COLOR_2" would be valid, assuming I'm reading the spec correctly. I can update the code to enforce that [semantic]_[index] pattern.

donmccurdy · 2018-02-28T00:47:25Z

examples/js/loaders/deprecated/LegacyGLTFLoader.js

+
+									for( var attributeName in parameters ) {
+
+										if( parameters[attributeName]['semantic'] === attributeId ) {


nit: could you add whitespace to match other code —

if ( parameters [ attributeName ][ 'semantic' ] === attributeId ) {

donmccurdy · 2018-02-28T00:48:05Z

Ok, that sounds fine then. No need to validate names against the [existing_semantic]_[index] pattern I don't think. Other than the other few comments above I think this is close.

And, technically, if you wanted to use custom semantics they should still "just work" here. To be technically valid you should add a vendor extension to your file, but since it's presumably for your company's internal use that's your call. :)

donmccurdy · 2018-02-28T03:54:14Z

Remaining items:

Checking magic === 'glTF' just indicates glTF; the next four bytes are a version 1 or 2, and we can skip JSON parsing after checking that.
Safety checks for primitive without material, material without technique, etc.
Whitespace nit.

1d2d3d · 2018-03-08T17:14:54Z

Sorry, tight deadlines dragged me away from this. I'll make those changes ASAP.

1d2d3d · 2018-03-08T21:29:16Z

Seems three methods are required check the version, depending upon whether the input is a string, a text file, or a binary. If it's a glb, the second four bytes is the version number. If it's a string, it needs parsing as JSON, and then json.asset.version[0] contains the version. If it's a text file, the input is an object, which needs decoding to text and then JSON parsing.

donmccurdy · 2018-03-08T22:08:49Z

I'm not sure what you mean by a text file (is something creating a Blob or File instance?) but the other two cases are right. You can always use json.asset.version, the bytes in a GLB are just to save you the hassle of decoding the GLB, which could vary in future glTF versions.

And then if it's GLB and the first four bytes are not "glTF" then it's no kind of glTF file at all.

…ributes

1d2d3d · 2018-03-08T22:16:59Z

"text file" meaning, for example, "Box.gltf". (as opposed to "Box.glb")

donmccurdy · 2018-03-08T22:21:45Z

editor/js/Loader.js

+				var u32bytes = new Uint8Array( contents, 4, 4 );
+				var versionNumber = new Uint32Array(u32bytes)[0];
+
+				return ( versionNumber < 2 );


Can simplify these three lines:

var version = new DataView( contents ).getUint32( 4, true ); return version < 2;

donmccurdy · 2018-03-08T22:22:09Z

editor/js/Loader.js~RF6842b54.TMP

@@ -0,0 +1,622 @@
+/**


Think this file snuck into Git accidentally 🤔

donmccurdy · 2018-03-08T22:26:24Z

examples/js/loaders/deprecated/LegacyGLTFLoader.js

+
+										}
+
+									}


One more nit (sorry!), rather than nesting these could you use break? Example:

if ( ! primitive.material ) break; var material = ...; if ( ! material.technique ) break; var parameters = json.techniques[ material.technique ].parameters || {}; ...

No problem! And yeah, TMP file, whoops. I'll get rid of that when I make the changes above.

donmccurdy · 2018-03-10T00:05:47Z

Demo: https://cdn.rawgit.com/1d2d3d/three.js/486f7b91bc2ef02ef29dd6a2683446cef6b07ba7/editor/index.html

The changes all look good to me, but I can't open any glTF 1.0 .glb models in the editor that contain textures, seeing this error:

DOMException: Failed to execute 'btoa' on 'Window': The string to be encoded contains characters outside of the Latin1 range.
    at GLTFBinaryExtension.prototype.loadTextureSourceUri

Shouldn't be caused by this PR, but might be something we need to fix before this goes out in the editor. Maybe merge this after r91 and then fix the other issue separately before r92?

1d2d3d · 2018-03-13T21:11:52Z

I'm reading about the issue - looks like this might apply: https://developer.mozilla.org/en-US/docs/Web/API/WindowBase64/Base64_encoding_and_decoding#The_Unicode_Problem

donmccurdy · 2018-03-13T21:29:55Z

In the newer GLTFLoader we don't use btoa at all, and instead do something like:

var blob = new Blob( [ bufferView ], { type: source.mimeType } );
sourceURI = URL.createObjectURL( blob );

... which should be much faster, and avoid this issue. But anyway it's not related to this PR. Might be good to fix before this is live in the Editor, but there are not many sources of .glb files using glTF 1.0 at this point anyway.

mrdoob · 2018-03-14T15:38:02Z

Thanks!

donmccurdy · 2018-03-14T15:57:17Z

Nice work @1d2d3d !

1d2d3d · 2018-03-19T15:34:57Z

Great! Thanks to you both!

Updated LegacyGLTFLoader.js shader parsing

70572a2

1d2d3d mentioned this pull request Feb 16, 2018

Improved shader parsing in LegacyGLTFLoader.js #13332

Closed

donmccurdy requested changes Feb 16, 2018

View reviewed changes

mrdoob added this to the r91 milestone Feb 17, 2018

Removed code changes except for the part that adds user-defined verte…

5bc5b8a

…x attributes

donmccurdy reviewed Feb 22, 2018

View reviewed changes

Fixes for issues brought up by donmccurdy

e58f973

donmccurdy reviewed Feb 25, 2018

View reviewed changes

donmccurdy mentioned this pull request Feb 25, 2018

Does glTF 1.0 allow application-specific semantics? KhronosGroup/glTF#1264

Closed

donmccurdy reviewed Feb 25, 2018

View reviewed changes

mrdoob modified the milestones: r91, r92 Feb 28, 2018

donmccurdy reviewed Feb 28, 2018

View reviewed changes

Fixed bug in isGltf1(); added safety checks when parsing for glTF att…

1fc2c44

…ributes

donmccurdy reviewed Mar 8, 2018

View reviewed changes

Alan Millman added 2 commits March 8, 2018 17:11

A bit of cleaning/refactoring

12e254f

Forgot || {}

486f7b9

donmccurdy approved these changes Mar 13, 2018

View reviewed changes

mrdoob modified the milestones: r92, r91 Mar 14, 2018

mrdoob merged commit f66fc35 into mrdoob:dev Mar 14, 2018

This was referenced Mar 14, 2018

LegacyGLTFLoader: Fix parsing textures in GLB files. #13589

Merged

Editor determines GLTF version and uses appropriate loader #13333

Closed


		try {

		extensions[ EXTENSIONS.KHR_BINARY_GLTF ] = new GLTFBinaryExtension( contents );


		var magic = THREE.LoaderUtils.decodeText( new Uint8Array( contents, 0, 4 ) );

		if ( magic === 'glTF' ) {


		for( var attributeName in parameters ) {

		if( parameters[attributeName]['semantic'] === attributeId ) {

Updated LegacyGLTFLoader.js shader parsing #13339

Updated LegacyGLTFLoader.js shader parsing #13339

Conversation

1d2d3d commented Feb 15, 2018 • edited Loading

1d2d3d commented Feb 15, 2018

1d2d3d commented Feb 15, 2018

1d2d3d commented Feb 15, 2018

1d2d3d commented Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donmccurdy Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donmccurdy commented Feb 16, 2018

donmccurdy commented Feb 16, 2018

takahirox commented Feb 16, 2018 • edited Loading

1d2d3d commented Feb 21, 2018 • edited Loading

mrdoob commented Feb 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donmccurdy commented Feb 22, 2018

mrdoob commented Feb 22, 2018 • edited Loading

donmccurdy commented Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1d2d3d Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donmccurdy commented Feb 25, 2018

donmccurdy commented Feb 25, 2018

mrdoob commented Feb 27, 2018 • edited Loading

1d2d3d commented Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

donmccurdy commented Feb 28, 2018 • edited Loading

donmccurdy commented Feb 28, 2018 • edited Loading

1d2d3d commented Mar 8, 2018

1d2d3d commented Mar 8, 2018

donmccurdy commented Mar 8, 2018

1d2d3d commented Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donmccurdy commented Mar 10, 2018

1d2d3d commented Mar 13, 2018

donmccurdy commented Mar 13, 2018

mrdoob commented Mar 14, 2018

donmccurdy commented Mar 14, 2018

1d2d3d commented Mar 19, 2018

1d2d3d commented Feb 15, 2018 •

edited

Loading

donmccurdy Feb 21, 2018 •

edited

Loading

takahirox commented Feb 16, 2018 •

edited

Loading

1d2d3d commented Feb 21, 2018 •

edited

Loading

mrdoob commented Feb 22, 2018 •

edited

Loading

donmccurdy commented Feb 22, 2018 •

edited

Loading

1d2d3d Feb 22, 2018 •

edited

Loading

mrdoob commented Feb 27, 2018 •

edited

Loading

1d2d3d commented Feb 27, 2018 •

edited

Loading

donmccurdy commented Feb 28, 2018 •

edited

Loading

donmccurdy commented Feb 28, 2018 •

edited

Loading

1d2d3d commented Mar 8, 2018 •

edited

Loading