-
Notifications
You must be signed in to change notification settings - Fork 250
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
2.0 Roadmap #330
Comments
I'm going to wait on this before we start to ask for more contributors. |
gltf-pipeline should be conformant to the glTF spec. We should make the fast runtime engine conversion an option that defaults to off. |
Yeah I agree on that, we already use an |
I think this could be filed separately as a post-2.0 enhancement. Great if someone wants to take it, but not blocking 2.0 release.
Agreed, but let's make this an
Not sure I understand — is this something that should have an enable/disable flag, or better to just debug it if/when we find issues? |
Not quite following... do you mean there are files missing from gltf-pipeline that are provided by Cesium, so running
|
Sounds good, I moved it to a "Later" section.
Also sounds good.
A lot of glTF 1 models do not have proper byte alignment for their accessors, so in order to create a TypedArray of an unaligned accessor the data must be copied to a new ArrayBuffer first. We try to avoid doing this in Cesium to avoid that overhead. And since some gltf-pipeline files are included in Cesium we try to avoid any copies there too. But after looking at the glTF 1 spec I realized byte alignment was required the whole time. I thought it was only added for glTF 2. This means the models have never been "correct" and maybe we shouldn't be worrying as much about the copy overhead. Instead Cesium should just report a warning if it notices unaligned accessors. So what I wrote above probably doesn't matter as much, we should handle this situation however is best for gltf-pipeline, not Cesium. I don't think any flags are needed.
Basically we wanted to be able to use gltf-pipeline's 1.0-2.0 converter in Cesium. Usually browserify or webpack are they way to go here, (and I forget the details) but we weren't able to get it to work. I think part of the problem was that gltf-pipeline includes the Cesium npm package. We included Cesium in order to get some of its utility functions and normals-generation code. But if Cesium the JS library includes a browserified gltf-pipeline, which includes the Cesium npm package, some circular dependency problem starts to happen. So I guess the basic question is, how can we bring gltf-pipeline to the web to be used by Cesium and other engines? Our somewhat hacky solution was to amd-ify a subset of the files. The major downside here was we could no longer use the Node.js API in those files. So in the code you might notice that we're using ArrayBuffers/TypedArrays in some places and Buffers in others. This was meant to be a short term solution from a start, so I would really like to see this approach changed before the next version of gltf-pipeline is considered ready. By the way, I haven't tried browserify myself so I might be able to experiment a little. |
So, this problem has a few parts:
tl;dr: In order for gltf-pipeline to be bundled cleanly, it either has to stop depending on Cesium, or Cesium has to change how it exports its modules. |
Thanks for the clarifications @lasalvavida - that is a great summary. I know ES6 modules is on the Cesium roadmap, but I wouldn't want that to hold up gltf-pipeline. I think the best solution moving forward is to keep the |
Nobody wants a "me too!" comment, but this is a pretty big deal now that facebook embed .glb. My files are gltf and I don't seem to have a way to convert them from there to .glb using node. |
@webprofusion-chrisc you can try out the 2.0 branch. It's not that thoroughly tested but it supports:
|
@webprofusion-chrisc FWIW i'm using https://sbtron.github.io/makeglb/ for glTF->GLB with some success. The Facebook importer is extremely strict about validation (probably for the best), so if there are any validation errors it will fail. May want to run the model through validation here or here first. |
@lilleyse this is great, it does indeed convert my gltf! The resulting .glb gives my a few validation messages: "messages": [
{
"code": "NODE_MATRIX_DEFAULT",
"message": "Do not specify default transform matrix.",
"severity": 2,
"pointer": "/nodes/0/matrix"
},
{
"code": "NODE_MATRIX_DEFAULT",
"message": "Do not specify default transform matrix.",
"severity": 2,
"pointer": "/nodes/2/matrix"
},
....
... @donmccurdy thanks, yes I found makeglb after some googling and it seemed OK but has no options to tweak. Your viewer is great and my new glb (exported from a threejs scene and run through gltf-pipeline 2.0) is working fine with no issues. I had to tweak processBufferView in the threejs exporter to compensate for 4-byte alignment again, I'll do a PR. : Now if I could just get facebook to allow up to 6MB instead of 3MB. Our models were originally >40MB so to get it down to 5, with textures, was a stretch. |
dev version of GLTFExporter should have the other 4-byte alignment issue ironed out now, I think. Edit: oops, I see the new PR :) |
Thanks for the notes @webprofusion-chrisc, the |
Seeing as compressed textures are not part of the GLTF 2.0 spec (see KhronosGroup/glTF#835), does it still make sense to still have the automated tooling to create them in the |
@TimvanScherpenzeel We plan on removing compressed texture support in the |
Moved "Future" items to their own issues with post 2.0 label |
Would it be possible to (a) put a prominent note in the master branch README about the 2.0 branch, (b) merge 2.0 to master, and/or (c) publish the 2.0 branch to npm? Its discoverability is currently pretty low, given that most visitors to this repo are likely using glTF 2.0 at this point. |
@donmccurdy the 2.0 branch is really close to getting merged into master. Hopefully sometime this week. After we'll publish to npm. |
Sounds great, thanks! |
2.0 is now merged into master and published to npm! |
Background:
The 2.0-cesium branch is used by Cesium to upgrade 1.0 models to 2.0. The upgraded models use the KHR_technique_webgl extension, which is supported by Cesium but hardly any other engines. PBR is not supported in this branch.
The 2.0 branch is a cleanup of glTF-pipeline that supports 1.0-2.0 conversion, conversions between glTF and glb, and other basic tasks. The other pipeline stages will eventually be moved over here once the core is solid.
One confusing aspect of the project right now is a subset of the files are used within Cesium for upgrading 1.0 models. Cesium includes gltf-pipeline which includes Cesium. Our early attempts at using browserify didn't work, so we instead added a gulp task called
build-cesium
which saves those gltf-pipeline files with AMD syntax to be packaged directly with Cesium. Additionally those files do not use the Node.js API or any third party packages (except Cesium). If anyone can think of an alternative to this approach that would be very helpful.Another side effect of including gltf-pipeline in Cesium is we try to make those files as performant as possible. We try to avoid any large buffer copies, and because of that glTF 1.0 models that are upgraded to 2.0 are not always byte aligned correctly. This is best for Cesium but not best for gltf-pipeline, so there needs to be some compromise here.
To do:
processModelMaterialsCommon
andprocessPbrMetallicRoughness
from the2.0-cesium
branch out of gltf-pipeline and into Cesium alone.2.0
branch as it did on the2.0-cesium
branch. Will need to re-add thebuild-cesium
gulp task used by2.0-cesium
but with a different list of files to package.parseGlb
,addPipelineExtras
,updateVersion
,addDefaults
, and any helper files used by them need to be packaged. When loading a gltf model Cesium should calladdPipelineExtras
,updateVersion
,addDefaults
in that order. All the packaged files must be pure JS without any node-specific code.master
and2.0-cesium
lately and see if any are applicable to the2.0
branch.The text was updated successfully, but these errors were encountered: