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 new drop and updates #1650

Merged
merged 4 commits into from
Jan 22, 2018

Conversation

keveleigh
Copy link
Contributor

Overview

This update includes the latest drop from https://github.com/KhronosGroup/UnityGLTF. This drop cleans up quite a bit of excess code, as the schema scripts are now found in the GLTFSerialization DLL.

It removes a script I created (GLTFComponentStreamingAssets) in favor of new support in this drop.

It also replaces our previous (incorrect, as we are not using specular materials) use of the Unity Standard (Specular) shader in favor of the project's GLTFStandard shader.

Changes

Related to #1644

@keveleigh keveleigh self-assigned this Jan 18, 2018
Copy link

@NeerajW NeerajW left a comment

Choose a reason for hiding this comment

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

Did not code review files from the Khronos code base but otherwise looks good.

@@ -8,7 +8,8 @@ Material:
m_PrefabInternal: {fileID: 0}
m_Name: HoloToolkit_Focusable
m_Shader: {fileID: 4800000, guid: f1f3948b20c230044bda31b620ddd37f, type: 3}
m_ShaderKeywords: _EMISSION
m_ShaderKeywords: _EMISSION _SPECULARHIGHLIGHTS_ON _USEAMBIENT_ON _USEDIFFUSE_ON
Copy link
Contributor

Choose a reason for hiding this comment

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

Does updating this material required for the gLTF update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The MotionControllerTest scene's cube is broken without it. I suppose it's not required for the glTF drop itself, but it fixes the main test scene for the glTF models.

@@ -1,11 +1,11 @@
fileFormatVersion: 2
guid: 3628db10e32278e499da95887f1bc989
timeCreated: 1488842155
guid: 9087a95f22bcee64ebc45f6deec08714
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this meta file change break the scene references in other people's projects?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, if the script file name hasn't changed, we should reserve the original .meta files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an update from the source repo. Looks like they changed it there, so I'm leaving this edit here to match it.

@SimonDarksideJ
Copy link
Contributor

My only concern is the same as @StephenHodgson regarding the .meta files, should try to retain the original GUID's if we can. Unless these are all new from the source repo.

A few concerns with the Kronos scripts, but they need to be addressed in the base repo, not here.

@StephenHodgson StephenHodgson merged commit 128dffa into microsoft:Dev_Working_Branch Jan 22, 2018
@keveleigh keveleigh deleted the GLTFNewDrop branch January 22, 2018 23:04
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

4 participants