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

2.0 Roadmap #330

Closed
5 of 6 tasks
lilleyse opened this issue Oct 10, 2017 · 21 comments
Closed
5 of 6 tasks

2.0 Roadmap #330

lilleyse opened this issue Oct 10, 2017 · 21 comments
Labels

Comments

@lilleyse
Copy link
Contributor

lilleyse commented Oct 10, 2017

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:

  • Add support for KHR_techniques_webgl and EXT_Blend when updating 1.0 models to 2.0
  • Cesium integration
    • Move processModelMaterialsCommon and processPbrMetallicRoughness from the 2.0-cesium branch out of gltf-pipeline and into Cesium alone.
    • Test that packaging for Cesium still works on the 2.0 branch as it did on the 2.0-cesium branch. Will need to re-add the build-cesium gulp task used by 2.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 call addPipelineExtras, updateVersion, addDefaults in that order. All the packaged files must be pure JS without any node-specific code.
    • Fix Remove pipeline extras for glTF cesium#5776
  • Look at all pull requests that have gone into master and 2.0-cesium lately and see if any are applicable to the 2.0 branch.
@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 10, 2017

Bring the new 2.0 branch up to good enough quality so others can start working on it. It is almost there.

I'm going to wait on this before we start to ask for more contributors.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 10, 2017

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.

gltf-pipeline should be conformant to the glTF spec. We should make the fast runtime engine conversion an option that defaults to off.

@lilleyse
Copy link
Contributor Author

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 optimizeForCesium flag elsewhere that we can use here too.

@donmccurdy
Copy link

donmccurdy commented Oct 11, 2017

  • Support exporting with specular-glossiness. This will require a metallic-roughness to specular-glossiness converter.

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.

  • Continue to support KHR_technique_webgl throughout the pipeline since having custom shaders is very important for some of Cesium's users.

Agreed, but let's make this an --experimental_khr_technique_webgl flag, that is off by default, to avoid having these models spread too far before the extension is finalized.

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.

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?

@donmccurdy
Copy link

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...

Not quite following... do you mean there are files missing from gltf-pipeline that are provided by Cesium, so running browserify index.js -o bundle.js doesn't work? What I see when trying that now is something like:

Error: Cannot find module './lib/AccessorReader' from '/Users/donmccurdy/Documents/Projects/gltf-pipeline'
@lilleyse
Copy link
Contributor Author

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.

Sounds good, I moved it to a "Later" section.

Agreed, but let's make this an --experimental_khr_technique_webgl flag, that is off by default, to avoid having these models spread too far before the extension is finalized.

Also sounds good.

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?

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.

Not quite following... do you mean there are files missing from gltf-pipeline that are provided by Cesium, so running browserify index.js -o bundle.js doesn't work? What I see when trying that now is something like:

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.

@lasalvavida
Copy link
Contributor

lasalvavida commented Oct 11, 2017

So, this problem has a few parts:

  1. Cesium uses requirejs in order to provide an npm package
    Because Cesium uses AMD for its modules, the endpoint that is exposed is actually an instance of requirejs pointed at the Cesium source tree. This barrier made it impossible by any means that I explored to resolve Cesium dependencies in gltf-pipeline with a bundler.
    If you look at my node-cesium-experimental repository, that was my proposed solution to this problem, which to my knowledge does work. It reformats and conditions the Cesium source code from AMD -> CommonJS and exposes Cesium sources directly from index.js so that Browserify and Webpack can read it.
    However, this would have been a big change to Cesium so we decided that in order to get glTF 2.0 support in Cesium moving faster that we would have to work around this. So instead of bringing Cesium to gltf-pipeline, we brought gltf-pipeline to Cesium.

  2. ES5 CommonJS require statements can't be optimized by any existing bundler
    Simply put: var Cesium = require('cesium'); var Cartesian3 = Cesium.Cartesian3; can't be optimized to only bundle Cartesian3. This is usually called "tree-shaking" by bundlers. Webpack and Roll-up do support this for ES6 imports: import Cartesian3 from 'cesium'. But since we weren't even willing to move Cesium from AMD -> ES5, I didn't explore going from ES5 -> ES6 too deeply. I think there are some babel based projects that attempt this though. The alternative that I settled on for the work I did do in my experimental repo was splitting Cesium into smaller packages, Core, Renderer, etc, and then having gltf-pipeline depend on a subset of Cesium.

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.

@lilleyse
Copy link
Contributor Author

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 2.0-cesium branch in a working state for inclusion in Cesium, but treat the 2.0 branch as its own thing and not worry about how it interacts with Cesium as a third-party. This will make things a lot simpler in the long run.

@webprofusion-chrisc
Copy link

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.

@lilleyse
Copy link
Contributor Author

@webprofusion-chrisc you can try out the 2.0 branch.

It's not that thoroughly tested but it supports:

  • Converting glTF to glb (and reverse)
  • Saving buffers/textures as embedded or separate files
  • Converting glTF 1.0 models to glTF 2.0 (using the KHR_technique_webgl extension)
@donmccurdy
Copy link

donmccurdy commented Feb 23, 2018

@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.

@webprofusion-chrisc
Copy link

@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. :
glb

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.

@donmccurdy
Copy link

donmccurdy commented Feb 24, 2018

dev version of GLTFExporter should have the other 4-byte alignment issue ironed out now, I think.

Edit: oops, I see the new PR :)

@lilleyse
Copy link
Contributor Author

lilleyse commented Mar 7, 2018

Thanks for the notes @webprofusion-chrisc, the NODE_MATRIX_DEFAULT problem is fixed.

@TimvanScherpenzeel
Copy link

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 2.0 branch? Or is this something specific to Cesium?

@lilleyse
Copy link
Contributor Author

@TimvanScherpenzeel We plan on removing compressed texture support in the 2.0 branch. It wasn't widely used and like you pointed out it would be better to wait for a glTF 2 extension.

@ggetz
Copy link
Contributor

ggetz commented Jun 25, 2018

Moved "Future" items to their own issues with post 2.0 label

@donmccurdy
Copy link

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.

@lilleyse
Copy link
Contributor Author

lilleyse commented Aug 5, 2018

@donmccurdy the 2.0 branch is really close to getting merged into master. Hopefully sometime this week. After we'll publish to npm.

@donmccurdy
Copy link

Sounds great, thanks!

@lilleyse
Copy link
Contributor Author

lilleyse commented Aug 15, 2018

2.0 is now merged into master and published to npm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
7 participants