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

Gltf 2.0 #4808

Closed
wants to merge 54 commits into from
Closed

Gltf 2.0 #4808

wants to merge 54 commits into from

Conversation

lasalvavida
Copy link
Contributor

@lasalvavida lasalvavida commented Jan 4, 2017

This PR is for initial review/testing, and for me to track progress.

Tasklist:

  • Add back model caching support
  • Re-enable releasing the glTF
  • Migrate gltf-pipeline changes back to its repo
  • Make sure doc is completely up to date with these changes
  • Fix tests (caching support should mostly fix this)
  • Test all model variants

Testing TODO

  • Verify glTF model cache works as before (load lots of the same model and check memory usage). Test glTF, binary glTF, and when glTF 1.0 is upgraded to 2.0 on the fly.
  • Verify animation cache works as before
  • Verify all glTF 1.1 Tutorial Models
  • Verify all Cesium test/sample models and glTF Sample Models.
  • Update Cesium models
    • Commit separately from code changes so we can squash them into one commit for all updates to not bloat the repo size

Optional:

  • Unified loadGltfUris that works for pipeline and Cesium?

As this stands, glTF 0.8 and 1.0 models will be updated to 2.0 automatically on load and 2.0 models should work as they are.

This includes some restructuring of how resources are loaded by Model. It has been updated to use promises instead of callbacks and statistics. In order to updateVersion (and processModelMaterialsCommon), buffers and shaders need to be loaded, so the loading of external resources needed to be made a bit more structured. Textures are still streamed incrementally if that is enabled. This now uses the the previous method of loading resources.

This is ready for a look.

pjcozzi and others added 30 commits August 30, 2016 10:32
glTF 1.0.1 Adds support for normalized vertex arrays and a test
…cissor-test

glTF 1.0.1 remove scissor test
…extensions-1.0.1

Test glTF for required extensions.
@lasalvavida
Copy link
Contributor Author

@mramato if you have time I could use some advice on something here. With the current way the Cesium build process works, all of the individual gltf-pipeline functions we are using here are placed onto the global Cesium scope. I suspect that isn't what we want in the end; something like Cesium.GltfPipeline.* is probably more appropriate.

I did try creating a wrapper GltfPipeline.js, but they still get attached to the Cesium global along with the wrapper because they get matched by sourceFiles.

Is there a fairly straightforward to do this with the existing build structure, or am I going to have to add this as a special case?

@javagl
Copy link
Contributor

javagl commented Jan 27, 2017

@cx20 It could make sense to "mirror" the directory structure of the official sample models in your repo, or (maybe even better: ) include the official sample models as a Git submodule. (Requires users to check out with -recursive, but this can be stated in the README). Beyond that, this point is not directly related to this pull request, and I'd suggest to discuss this in an issue in gltf-test if necessary.

@mramato
Copy link
Contributor

mramato commented Jan 27, 2017

I did try creating a wrapper GltfPipeline.js, but they still get attached to the Cesium global along with the wrapper because they get matched by sourceFiles.

Is there a fairly straightforward to do this with the existing build structure, or am I going to have to add this as a special case?

We don't normally filter things like because being on the Cesium object isn't really a big deal. Is there a specific motivation for this?

The short version is that createCesiumJs is what generated the Cesium object (which gets output to Source\Cesium.js. If you want to filter some files to not be on the object, you want to avoid pushing them onto the assignments array (but push them onto the other arrays as-is). I would check the generated Cesium documentation too, but I'm pretty sure ThirdParty is automatically excluded there. Let me know if you run into issues, but I don't expect any.

As an aside, I've been meaning to wrap my head around what you guys are trying to do for a while, but haven't had a chance to investigate. Can you verify my recap here is correct:

  • It's my understanding that we want to share some code between gltf-pipeline and Cesium
  • gltf-pipeline wants to use Cesium for some Core modules.
  • Scene layer needs to depend on gltf-pipeline.
  • Since Cesium is somewhat monolithic, gltf-pipeline currently copies some of it's files into it's own project so that it doesn't depend on them directly.
  • A directory structure has been carefully crafted so that gltf pipeline can then be used in source form inside of Cesium.

Is that correct? If so, my one question is, why not just have a simple build that outputs a single JS file that directly incorporates the parts of Cesium it needs. This avoids the need for maintaining copies of anything and the only cost is a few kb of duplicate code (assuming gltf-pipeline use of Cesium is small). The whole thing could be automated at build time.

That being said, I'm okay with trying to win the battle and not the war, I just want to make sure I have a good understanding of what's going on architecturally.

@lasalvavida
Copy link
Contributor Author

We don't normally filter things like because being on the Cesium object isn't really a big deal. Is there a specific motivation for this?

Just that these aren't really Cesium functions and it might be confusing to have them as part of the Cesium global scope.

If so, my one question is, why not just have a simple build that outputs a single JS file that directly incorporates the parts of Cesium it needs. This avoids the need for maintaining copies of anything and the only cost is a few kb of duplicate code (assuming gltf-pipeline use of Cesium is small).

There are no copies, we rely on the Cesium node module in gltf-pipeline.

This is kind of what I had tried to do originally. The tl;dr for that is that CommonJS bundling methods like Browserify and Webpack aren't smart enough to optimize something like:

var Cesium = require('cesium');
var Cartesian3 = Cesium.Cartesian3;

such that only Cartesian3 gets bundled for a particular function.

RollUp and Webpack can do tree-shaking with ES6 import syntax:

import Cartesian3 from Cesium;

Long-term, that might be the most correct answer, but for right now there is too much work involved in going AMD -> CommonJS -> ES6 reliably.

The route I took here is to take the gltf-pipeline functions we need and amdify them, resolving out things like Cesium.Cartesian3 into the Cesium Source tree.

@lasalvavida
Copy link
Contributor Author

Also @pjcozzi , all Scene/Model test should pass now.

@lilleyse lilleyse mentioned this pull request May 10, 2017
23 tasks
@lilleyse lilleyse mentioned this pull request Jun 5, 2017
14 tasks
@moneimne moneimne mentioned this pull request Jul 18, 2017
8 tasks
@lilleyse
Copy link
Contributor

Closing - all this work is incorporated in #5641

@lilleyse lilleyse closed this Jul 18, 2017
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.

None yet

7 participants