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

Fix model crash by adding default pbr material #6906

Merged
merged 4 commits into from Aug 27, 2018
Merged

Conversation

lilleyse
Copy link
Contributor

Fixes #6891

Like #6857 (comment), this fix is temporary until #6805 is in.

@cesium-concierge
Copy link

cesium-concierge commented Aug 10, 2018

Thanks for the pull request @lilleyse!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Changes to third party files were made.
    • Looks like a file in one of our ThirdParty folders (ThirdParty/, Source/ThirdParty/) has been added or modified. Please verify that it has a section in LICENSE.md and that its license information is up to date with this new version.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@hpinkos
Copy link
Contributor

hpinkos commented Aug 10, 2018

@lilleyse do you think #6805 will make it in this release? I don't think we should merge this workaround if we're going to have the actual solution ready to go shortly

@lilleyse
Copy link
Contributor Author

Fine by me, #6805 should made the release.

@lilleyse lilleyse closed this Aug 10, 2018
@lilleyse lilleyse deleted the default-material-fix branch August 10, 2018 21:38
@lilleyse lilleyse restored the default-material-fix branch August 26, 2018 23:47
@lilleyse lilleyse reopened this Aug 26, 2018
@lilleyse
Copy link
Contributor Author

It's looking like #6805 won't make the release so I reopened this PR.

@hpinkos
Copy link
Contributor

hpinkos commented Aug 27, 2018

I'm still not convinced we should rush to get this workaround in for the release since we have the actual fix coming soon. Was this issue a common problem that a number of our users are experiencing?

@lilleyse
Copy link
Contributor Author

@hpinkos
Copy link
Contributor

hpinkos commented Aug 27, 2018

Okay, that's significant

@hpinkos
Copy link
Contributor

hpinkos commented Aug 27, 2018

Who should review this? @likangning93?

@lilleyse
Copy link
Contributor Author

I think @ggetz has more familiarity with the gltf-pipeline code and would be better to review this.

@ggetz
Copy link
Contributor

ggetz commented Aug 27, 2018

Since Source/ThirdParty/GltfPipeline/addDefaults.js will be overridden by the rebuilt GltfPipeline files, I think this fix will be fine temporarily.

@ggetz
Copy link
Contributor

ggetz commented Aug 27, 2018

Code looks good and the model from #6891 loads correctly. Thanks @lilleyse !

@ggetz ggetz merged commit 82d5f8e into master Aug 27, 2018
@ggetz ggetz deleted the default-material-fix branch August 27, 2018 18:40
@hpinkos
Copy link
Contributor

hpinkos commented Aug 27, 2018

@lilleyse open an issue or add a comment to #6805 to revert this workaround when the actual fix comes in.

@ggetz
Copy link
Contributor

ggetz commented Aug 27, 2018

#6805 completely rebuilds the files in GltfPipeline from the new master (as opposed to a previous branch just fro cesium) so this fix will already be overridden.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cesium no longer adds a default material for gltf 2.0 as needed
4 participants