Improved shader parsing in LegacyGLTFLoader.js #13332
Conversation
Would be great to hear (or even better, file bug reports) if you had particular issues; three.js should be able to render all of the glTF 2.0 samples correctly. Here are recent test cases, and a demo of BoxTextured.gltf in particular.
From a quick glance your changes look fine; I'll do a more careful review if we are interested in merging changes like this. @takahirox @mrdoob any thoughts on that, or whether to consider LegacyGLTFLoader "archived" at this point? @1d2d3d I think the odds of including this patch would be higher if it were possible to handle extra attributes without as much refactoring... is it possible to do the same thing with a |
|
In short, I don't wanna update Legacy stuffs but I won't stop merging this change. Not strong preference but personally I don't wanna update "Legacy" stuffs any more (except for fixing serious bugs). If we keep updating it, we'd better to move it from And, if I'm right, the basic Three.js attitude is to advice converting to glTF 2.0 rather than using glTF legacy loader. From another point of view, as you said glTF 2.0 doesn't support custom shader "yet" and this update is one-time update, not we try to keep updating. So could be acceptable. |
| @@ -3,6 +3,7 @@ | |||
| * @author mrdoob / http://mrdoob.com/ | |||
| * @author Tony Parisi / http://www.tonyparisi.com/ | |||
| * @author Takahiro / https://github.com/takahirox | |||
| * @author 1d2d3d / https://github.com/1d2d3d | |||
There was a problem hiding this comment.
FYI - All contributions are welcome, but we typically do not add authorship for minor changes.
There was a problem hiding this comment.
We should change @author for @maintainer.
I bet people would stop adding themselves though 😂
|
@donmccurdy: I haven't explored the GLTF 2.0 examples much; since we need to include shader code, I was concentrating on GLTF 1. But to your other point, sure, will do a "minimal code change" branch. @takahirox: Yeah, at this time, that last point of view is my perspective. I hope the Khronos folks manage to get the plug-in spec'd out ASAP so we can forget all about GLTF 1, and our path forward will be to adopt GLTF 2 as soon as shaders are viable. But in the meantime, we need to ship. @WestLangley: Whoops, can certainly take that out. Want to be a good citizen, especially if people find issues, but there's git for that. |
|
@1d2d3d thanks for looking into the minimal change version. Yeah, I hear you on the GLSL extension, and hope that will move along quickly. Meanwhile using glTF 1.0 is a fair choice.. There may be a few changes to GLSL support in the glTF 2.0 extension, feel free to weigh in if you have any particular needs or feedback on it, or just want to +1 for its expedient development. 🙂 |
|
#13339 above is the minimal change version. |
|
Thanks — let's close this out and continue in #13339. |
Hi! 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.
Not that I've done a thorough exploration, but the legacy loader now seems to do a better job of loading the sample GLTF 1.0 files (e.g., BoxTextured.gltf) on the Khronos web site.
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. If you decide to accept this branch, I have a separate branch with the editor changes that allow it to open both GLTF 1 and 2 files. Please note that will only work if the parse() argument-swap in this branch is in place.