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.1 Adds support for normalized vertex arrays and a test #4247

Merged
merged 3 commits into from
Aug 31, 2016

Conversation

lasalvavida
Copy link
Contributor

Part of #4009. Test model is quantized box with a high precision decode matrix via the glTF 1.0.1 normalized property.

@lasalvavida lasalvavida changed the title Adds support for normalized vertex arrays and a test glTF 1.0.1 Adds support for normalized vertex arrays and a test Aug 29, 2016
@lasalvavida
Copy link
Contributor Author

@pjcozzi, could you look at this if you get a chance so we can start getting this chain of pull requests merged?

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 30, 2016

@lasalvavida I should have thought of this sooner, but I created a gltf-1.0.1 branch which we can use to accumulate all glTF 1.0.1 updates, and then we'll merge to master in one go once the spec is official.

I also updated CHANGES.md in this branch so each PR does not need to provide fine-grained updates.

Can you please change this and all glTF PRs to target the gltf-1.0.1 branch? This is a new option in the GitHub UI.

@lasalvavida lasalvavida changed the base branch from master to gltf-1.0.1 August 30, 2016 14:37
@lasalvavida
Copy link
Contributor Author

Changed the base to the gltf 1.0.1 branch

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 30, 2016

@lexaknyazev we are starting to implement glTF 1.0.1 in Cesium and gltf-pipeline. This should help tighten up anything that we overlooked. It's totally optional, but if you have the time, it would be great if you could look over these PRs to make sure we are keeping the spec/implementation in-sync. We'll CC you on them; each PR is small.

@@ -1598,6 +1599,13 @@ defineSuite([
});
});

it('loads a glTF with WEB3D_quantized_attributes and accessor.normalized for higher precision decode matrix', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not following the "higher precision decode matrix." Why not add per-vertex normalized ubyte8 colors to an existing model since that is the common use case? In general, I would also like to avoid bloating the repo with a lot of new test models to test corner cases if we are able to augment existing ones reasonably.

@lasalvavida lasalvavida force-pushed the gltf-1.0.1-normalized branch from e09a4bf to a8afa5e Compare August 30, 2016 17:03
@lasalvavida
Copy link
Contributor Author

I'm not following the "higher precision decode matrix." Why not add per-vertex normalized ubyte8 colors to an existing model since that is the common use case? In general, I would also like to avoid bloating the repo with a lot of new test models to test corner cases if we are able to augment existing ones reasonably.

This has been updated to modify the existing quantized box model instead of adding a new one. Commits have been squashed so it is removed from the history.

That may be a more common use case, but this also tests the functionality of normalized perfectly well, and now it doesn't add another model to the repo.

See my gltf-pipeline comment on why this is useful.

@lilleyse
Copy link
Contributor

lilleyse commented Aug 30, 2016

This looks good to me. I think @pjcozzi's suggestion is good since the testing can be more concrete, i.e. checking the pixel color vs. just checking it was rendered. It should be pretty easy to modify Box-Color to do the job, you may not even need to write new tests.

Edit: I think it's fine to keep the current test too.

@lasalvavida
Copy link
Contributor Author

Modified Box-Color to use the normalized property

@lilleyse
Copy link
Contributor

Looks good! @pjcozzi do you want to do a quick final review?

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 31, 2016

See my gltf-pipeline comment on why this is useful.

I see, thanks!

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 31, 2016

FYI - this does not update the version in Box-Color.gltf to 1.0.1. I think that is fine for now as we keep merging into gltf-1.0.1, then we'll update all the models.

@pjcozzi pjcozzi merged commit 34b63c1 into CesiumGS:gltf-1.0.1 Aug 31, 2016
@pjcozzi pjcozzi deleted the gltf-1.0.1-normalized branch August 31, 2016 20:34
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.

3 participants