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

Upgrade project for gltf-pipeline #7

Merged
merged 9 commits into from
Jun 23, 2016
Merged

Upgrade project for gltf-pipeline #7

merged 9 commits into from
Jun 23, 2016

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented Jun 10, 2016

A made a bunch of structural changes so that more functionality is handled by the gltf-pipeline. The diff is a little crazy mostly due to indenting now that some callback stages are removed.

Changes:

  • Removed the shader technique cmdline option and modelMaterialCommon.js. This is now handled in the gltf-pipeline.
  • Removed auto-generating normals if they are missing. Instead I added this to the gltf-pipeline roadmap
  • Removed util files and instead am including Cesium
  • Added gulp, jsHint tasks, .idea, .npmignore, and index.js
  • Added convert.js, which is the starting point for code that wants to use obj2gltf as a library

TODO

  • OBE - linking to the git repo for now. This won't be ready to merge until the gltf-pipeline is published to npm.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 11, 2016

Removed auto-generating normals if they are missing. Instead I added this to the gltf-pipeline roadmap

Should we do this soon? Is this often needed for OBJ models?

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 11, 2016

@lasalvavida can you please test this?

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 11, 2016

Here's a few test models: https://github.com/SaschaWillems/Vulkan/tree/master/models

If any didn't work before this PR; it is OK if they don't work now; we just don't want to break ones that previously worked.

@lasalvavida
Copy link
Contributor

@pjcozzi There are two obj models in that repo, chinesedragon and hammardillo and both convert correctly with these changes.

@lilleyse I would advise perhaps just adding gltf-pipeline as a git dependency for now if that's possible so these changes can get merged and then updating once gltf-pipeline is in npm.

@lilleyse
Copy link
Contributor Author

Updated

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 13, 2016

Thanks for the review, @lasalvavida. Please merge when ready.

@lilleyse
Copy link
Contributor Author

This is also ready.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 23, 2016

@lasalvavida you should have commit access to this now to review and merge when ready.

@lasalvavida lasalvavida merged commit 15a2f6f into master Jun 23, 2016
@lasalvavida lasalvavida deleted the pipeline-upgrade branch June 23, 2016 19:20
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