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 1.0 Changes #3039

Merged
merged 60 commits into from
Oct 29, 2015
Merged

glTF 1.0 Changes #3039

merged 60 commits into from
Oct 29, 2015

Conversation

tfili
Copy link
Contributor

@tfili tfili commented Sep 18, 2015

Don't merge yet

Fixes

  • Handle KHR_binary_glTF extension
  • 1.0 Spec changes (glTF 0.8 to 1.0 Guide KhronosGroup/glTF#406)
    • mesh.primitive.primitive -> mesh.primitive.mode
    • mesh.primitive.indices are now optional
    • technique.parameter.source -> technique.parameter.node
    • Removed passes from technique
    • Make technique optional. Render gray material if no technique or extension
    • Support extensions for COLLADA common profile techniques
    • Don't load models with extensions we don't support
  • Update all sample and test bgltf files to glb except one for testing
  • Update MIME type (Register MIME Types KhronosGroup/glTF#412)
  • Update CHANGES.md
  • Add deprecation warning for CESIUM_binary_glTF
  • Add KHR_materials_common model to Sandcastle
  • Add tests for KHR_materials_common with coverage. Should include all lighting models.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 18, 2015

For #3034

@@ -322,8 +322,16 @@ define([

if (gltf instanceof Uint8Array) {
// Binary glTF
var result = parseBinaryGltfHeader(gltf);

// CESIUM_binary_glTF is from the beginning of the file but
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should mention KHR_binary_glTF in the reference doc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a reminder:

We should mention KHR_binary_glTF in the reference doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where else do you want it mentioned? It is in the doc for the Model constructor and the fromGltf method.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 18, 2015

Good start, thanks @tfili.

@tfili
Copy link
Contributor Author

tfili commented Oct 14, 2015

@pjcozzi This is the PR I wanted you to look at.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 29, 2015

How is test coverage?

modelMaterialsCommon needs tests. I would do basic rendering tests with one (or a few) test models with different lighting models and lights, and then unit test modelMaterialsCommon and verify the generated glTF/shaders.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 29, 2015

Can we also add a model using KHR_materials_common to Sandcastle for visual testing. It could even replace one of our current models just using the extension.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 29, 2015

I think I mentioned this before, but it will require some effort to merge this into the 3d-tiles branch - it won't be too much though. We'll do it right after 1.15.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 29, 2015

I get this test failure (Mac/Chrome):

image

@tfili
Copy link
Contributor Author

tfili commented Oct 29, 2015

Can we also add a model using KHR_materials_common to Sandcastle for visual testing. It could even replace one of our current models just using the extension.

@pjcozzi I'll do this as soon as we merge my changes into the open source converter. I'm preparing the pull request now.

@tfili
Copy link
Contributor Author

tfili commented Oct 29, 2015

I get this test failure (Mac/Chrome):

Weird. It works on Windows. I'll look at it.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 29, 2015

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 29, 2015

@tfili that is all my comments. I will try to make a more detailed pass through modelMaterialsCommon tomorrow morning.

@tfili
Copy link
Contributor Author

tfili commented Oct 29, 2015

@pjcozzi I fixed all of them besides the test failure.Looking at that now.

@tfili
Copy link
Contributor Author

tfili commented Oct 29, 2015

@pjcozzi This should be good to go to look at again. All tests are done. The only thing I have left is the array of textures.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 29, 2015

Update the model path for this Sandcastle demo: http://localhost:8080/Apps/Sandcastle/index.html?src=Picking.html&label=All

image

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 29, 2015

Update the model path for this Sandcastle demo: http://localhost:8080/Apps/Sandcastle/index.html?src=Picking.html&label=All

I fixed this.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 29, 2015

This is good!

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 29, 2015

Let's merge into 3d-tiles right after we finish everything needed for 1.15.

pjcozzi added a commit that referenced this pull request Oct 29, 2015
@pjcozzi pjcozzi merged commit bc2c8a6 into CesiumGS:master Oct 29, 2015
@pjcozzi pjcozzi deleted the glTF-1.0 branch October 29, 2015 21:42
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.

4 participants