KHR_materials_pbrSpecularGlossiness support #74

jm991 · 2017-10-07T01:34:24Z

There are quite a few areas where this could be improved, but it does load spec/gloss materials

Areas that still need work:

Add a test to GLTFSerialization project for the KHR_materials_pbrSpecularGlossiness test file I added (1501770225815.gltf)
Add toggle to GLTFComponent.cs that allows for optional addition of collision geometry and layer
Revert editor downgrade
Clean up test scene & reference approved spec gloss gltf file from GLTF samples
Double checked the full spec of KHR_materials_pbrSpecularGlossiness schema to make sure all properties are properly implemented in the Serialize/Deserialize methods
Removed PbrSpecularGlossiness GLTFProperty in favor of KHR_materials_pbrSpecularGlossiness Extension in GLTFSerialization project
in GLTFSerialization project Material.cs's case "extensions" might not be the best way to handle material extensions (is there a better way to iterate through the material extensions than the while loop I added?)

…BRSpecularGlossiness Implement new class GLTFSerialization/Schema/MaterialPBRSpecularGlossiness.cs (following pattern set by MaterialPBRMetallicRoughness.cs)

Made sure the serialize/deserialize methods of PbrSpecularGlossiness can read from the Remix3D extension "KHR_materials_pbrSpecularGlossiness" declared in Remix3D gltf materials

…he existing PbrMetallicRoughness Also added support in switch statement for the "KHR_materials_pbrSpecularGlossiness" extension

* Update GLTFStandard.shader to support spec/gloss setup (create new GLTFStandardSpecular.shader) * Implement MaterialType.PbrSpecularGlossiness and KHR_materials_pbrSpecularGlossines in GLTFSceneImporter.cs ** Add code to GLTFSceneImporter.cs@CreateMaterial(GLTF.Schema.Material def, int materialIndex) to support the KHR_materials_pbrSpecularGlossiness extension (follow pattern set by def.PbrMetallicRoughness) Test changes in GLTFComponent.cs to make sure Remix3D models' materials load correctly:

blgrossMS

Thanks for adding this functionality! Your change looks pretty good. I left some comments.

Also make sure that the "areas that need work" listed above are completed before the PR is merged in.

blgrossMS · 2017-10-09T17:57:22Z

GLTFSerialization/External/glTF/1501770225815.gltf

@@ -0,0 +1,194 @@
+{


File name should be more descriptive

blgrossMS · 2017-10-09T17:59:04Z

GLTFSerialization/Tests/GLTFSerializationTests/GLTFLoaderTest.cs

-	{
-		readonly string GLTF_PATH = Directory.GetCurrentDirectory() + "/../../../../External/glTF/BoomBox.gltf";
+    {
+        readonly string GLTF_PATH = Directory.GetCurrentDirectory() + "/../../../../External/glTF/1501770225815.gltf";


undo or add another test

blgrossMS · 2017-10-09T18:02:14Z

GLTFSerialization/GLTFSerialization/Schema/Material.cs


-			if (CommonConstant != null)
+            if (CommonConstant != null)


nit- make spaces tabs

blgrossMS · 2017-10-09T18:05:13Z

GLTFSerialization/GLTFSerialization/Schema/Material.cs

+                    case "pbrSpecularGlossiness":
+                        material.PbrSpecularGlossiness = PbrSpecularGlossiness.Deserialize(root, reader);
+                        break;
+                    case "extensions":


extensions are handled by the default case. You should let that process the extensions and hook up an extension handler via extension factory API.

Also why are there two ways of processing KHR_materials_pbrSpecularGlossiness

…rials_pbrSpecularGlossiness schema Addressing PR feedback: https://github.com/KhronosGroup/UnityGLTF/pull/74/files Comment on this should be updated to reflect official schema comments from here: https://github.com/KhronosGroup/glTF/blob/master/extensions/Khronos/KHR_materials_pbrSpecularGlossiness/schema/glTF.KHR_materials_pbrSpecularGlossiness.schema.json

…sion KHR_materials_pbrSpecularGlossinessExtension.cs

…l/74/files * Moved KHR_materials_pbrSpecularGlossinessExtension files from Schema to Extensions folder * Added logic to GLTFProperty to deserialize using the KHR_materials_pbrSpecularGlossinessExtensionFactory if the extension name is found in the file * Removed specular gloss code from Material.cs since the spec-gloss material is now an extension * Removed MaterialPBRSpecularGlossiness.cs since it's now an extension instead of a GLTFProperty * Removed GLTFSceneImporter.MaterialType.PbrSpecularGloss in favor of GLTFSceneImporter.MaterialType.KHR_materials_pbrSpecularGlossiness * GLTFSceneImporter now accesses the KHR spec gloss extension properly * Updated GLTFSerialization.dll in Unity project * Renamed 1501770225815.gltf to a more descriptive filename * Cleaned up edits I had made to GLTFLoaderTest.cs

jm991

http://answers.unity3d.com/questions/970290/emission-at-run-time-works-in-editor-but-not-in-bu.html

We ran into this when using UnityGLTF in our project. The fix to make Unity compile all the variants of the Standard shader was to include a Material in the Resources folder that had a texture in all the material slots. I can add this too so that others won't be tripped up by this.

jm991 · 2017-10-07T01:38:51Z

GLTFSerialization/External/glTF/1501770225815.gltf

@@ -0,0 +1,194 @@
+{


This is a test file from remix3d.com that uses KHR_materials_pbrSpecularGlossiness. I plan to add a few more complex scenes to further test the implementation before checking in

jm991 · 2017-10-07T01:40:06Z

GLTFSerialization/GLTFSerialization/Schema/Material.cs

+                    case "pbrSpecularGlossiness":
+                        material.PbrSpecularGlossiness = PbrSpecularGlossiness.Deserialize(root, reader);
+                        break;
+                    case "extensions":


Is this the correct way to parse the material extension JSON? I wasn't sure if the second while (reader.Read() && reader.TokenType == JsonToken.PropertyName) was the best way to iterate thru the material extensions to find KHR_materials_pbrSpecularGlossiness

jm991 · 2017-10-07T01:45:18Z

GLTFSerialization/GLTFSerialization/Schema/Material.cs

@@ -15,6 +15,12 @@ public class Material : GLTFChildOfRootProperty
 		/// </summary>
 		public PbrMetallicRoughness PbrMetallicRoughness;

+        /// <summary>


If I rename MaterialPBRSpecularGlossiness.cs to KHRMaterialPBRSpecularGlossiness.cs, this will be updated to reflect that change.

I noticed UnityGLTF/Assets/UnityGLTF/Scripts/GLTFSceneImporter.cs.MaterialType included PbrSpecularGlossiness, so I wasn't sure whether to add public PbrSpecularGlossiness as a new Material or public KHR_materials_pbrSpecularGlossiness

jm991 · 2017-10-07T01:46:10Z

GLTFSerialization/GLTFSerialization/Schema/MaterialPBRSpecularGlossiness.cs

+        /// <summary>
+        /// The color of the material's specular highlights.
+        /// </summary>
+        public Vector3 SpecularFactor = Vector3.One;


Comment on this should be updated to reflect official schema comments from here:
https://github.com/KhronosGroup/glTF/blob/master/extensions/Khronos/KHR_materials_pbrSpecularGlossiness/schema/glTF.KHR_materials_pbrSpecularGlossiness.schema.json

k fixed this

jm991 · 2017-10-07T01:46:30Z

GLTFSerialization/Tests/GLTFSerializationTests/GLTFLoaderTest.cs

-	{
-		readonly string GLTF_PATH = Directory.GetCurrentDirectory() + "/../../../../External/glTF/BoomBox.gltf";
+    {
+        readonly string GLTF_PATH = Directory.GetCurrentDirectory() + "/../../../../External/glTF/1501770225815.gltf";


rename this to be more clear that it's a test for KHR_materials_pbrSpecularGlossiness & run it as a separate test instead of commenting it out in this test

jm991 · 2017-10-07T01:48:20Z

UnityGLTF/ProjectSettings/ProjectVersion.txt

@@ -1 +1 @@
-m_EditorVersion: 5.6.1f1
+m_EditorVersion: 5.6.0f3


I won't check in this editor downgrade - we are using this version of Unity on my team

jm991 · 2017-10-18T20:41:43Z

UnityGLTF/Assets/UnityGLTF/Scripts/GLTFComponent.cs

-			loader.SetShaderForMaterialType(GLTFSceneImporter.MaterialType.PbrMetallicRoughness, GLTFStandard);
-			loader.SetShaderForMaterialType(GLTFSceneImporter.MaterialType.CommonConstant, GLTFConstant);
+            loader.SetShaderForMaterialType(GLTFSceneImporter.MaterialType.PbrMetallicRoughness, GLTFStandard);
+            loader.SetShaderForMaterialType(GLTFSceneImporter.MaterialType.PbrSpecularGlossiness, GLTFStandardSpecular);


remove this

jm991 · 2017-10-26T00:44:27Z

GLTFSerialization/GLTFSerialization/Extensions/GLTFJsonExtensions.cs

@@ -64,6 +66,51 @@ public static List<T> ReadList<T>(this JsonReader reader, Func<T> deserializerFu
 			return list;
 		}



@MSblgross I added a bunch of new utility methods to this class that will help Extensions deserialize JTokens.
E.g., we already had JsonReader methods, like:
Color ReadAsRGBAColor(this JsonReader reader)
So I added JToken methods, such as:
DeserializeAsColor(this JToken token)

jm991 · 2017-10-26T00:44:31Z

GLTFSerialization/GLTFSerialization/Extensions/GLTFJsonExtensions.cs

@@ -64,6 +66,51 @@ public static List<T> ReadList<T>(this JsonReader reader, Func<T> deserializerFu
 			return list;
 		}

+        // TODO: (CR) is it ok to pass the GLTFRoot here and inclue using GLTF.Schema?


@MSblgross is it ok to pass the GLTFRoot to DeserializeAsTexture and include reference to using GLTF.Schema?

jm991 · 2017-10-26T00:46:11Z

GLTFSerialization/GLTFSerialization/Schema/MaterialNormalTextureInfo.cs

@@ -27,13 +29,13 @@ public static new NormalTextureInfo Deserialize(GLTFRoot root, JsonReader reader

 				switch (curProp)
 				{
-					case "index":
+					case INDEX:


@MSblgross I tried to go through the code and remove any hardcoded strings that appeared in a lot of places, such as "index" or "texCoord", hopefully that's ok to do

* Added spec gloss Unity test scene (uses existing spec gloss Lantern GLTF test files) * Added UWP test for spec gloss * Modifed test for spec gloss to use Lantern and deleted my test file (a file I had downloaded off Remix 3D) * Added spec gloss Lantern GLTF test files to GLTFSerialization project * Added nullref checks to spec gloss extension deserialization for assets that don't have all spec gloss properties defined (threw exception previously) * Added correct default values for spec gloss extension * Added README for compiling shader variants in Standalone Unity player, as well as a sample material and shader variant file to force shader variant compilation * Updated GLTFSerialization DLLs in Unity * Added optional mesh collider flag when creating GameObjects from GLTF files * Deleted GLTFSceneImporter.cs.orig (looks like someone checked in a partial merge)

…re conflicting before)

KHR_materials_pbrSpecularGlossiness support

John McElmurray added 5 commits October 5, 2017 13:50

Create duplicate of MaterialPBRMetallicRoughness to support MaterialP…

f335895

…BRSpecularGlossiness Implement new class GLTFSerialization/Schema/MaterialPBRSpecularGlossiness.cs (following pattern set by MaterialPBRMetallicRoughness.cs)

Change the fields from metal/roughness to specular/gloss

a741eb4

Made sure the serialize/deserialize methods of PbrSpecularGlossiness can read from the Remix3D extension "KHR_materials_pbrSpecularGlossiness" declared in Remix3D gltf materials

Added reference to PbrSpecularGlossiness to Material.cs, similar to t…

80a5235

…he existing PbrMetallicRoughness Also added support in switch statement for the "KHR_materials_pbrSpecularGlossiness" extension

Build new DLL for UWP and Editor and add to Unity

6c1e5a7

blgrossMS self-requested a review October 8, 2017 22:53

blgrossMS self-assigned this Oct 8, 2017

blgrossMS suggested changes Oct 9, 2017

View reviewed changes

sbtron added the 0.5 label Oct 18, 2017

John McElmurray added 6 commits October 24, 2017 14:28

Converting PbrSpecularGlossiness.cs serialize/deserialize to an Exten…

7ae80c6

…sion KHR_materials_pbrSpecularGlossinessExtension.cs

Removing unused GLTF file

2e819fd

Removing old code comments

faeec11

Adding test for loading KHR spec gloss material

2da2fbc

jm991 commented Oct 26, 2017

View reviewed changes

John McElmurray added 2 commits October 26, 2017 10:25

Undoing all my changes to LanternTestScene

e12dd18

jm991 changed the title ~~KHR_materials_pbrSpecularGlossiness and PbrSpecularGlossiness support~~ KHR_materials_pbrSpecularGlossiness support Oct 26, 2017

John McElmurray added 2 commits October 26, 2017 12:47

Converting spaces to tabs

40404df

Fixing scene importer so it loads EITHER factors or textures (they we…

70bad23

…re conflicting before)

blgrossMS approved these changes Nov 6, 2017

View reviewed changes

blgrossMS merged commit 516134c into KhronosGroup:master Nov 6, 2017

sbtron mentioned this pull request Nov 15, 2017

Add Support for Specular Glossiness Physically Based Rendering #5

Closed

hybridherbst mentioned this pull request Dec 23, 2019

KHR_materials_pbrSpecularGlossiness export support #539

Closed

github-actions bot pushed a commit to Rhinox-Training/UnityGLTF that referenced this pull request Nov 9, 2022

Merge pull request KhronosGroup#74 from jm991/master

f50537d

KHR_materials_pbrSpecularGlossiness support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KHR_materials_pbrSpecularGlossiness support #74

KHR_materials_pbrSpecularGlossiness support #74

jm991 commented Oct 7, 2017 •

edited

blgrossMS left a comment

blgrossMS Oct 9, 2017

blgrossMS Oct 9, 2017

blgrossMS Oct 9, 2017

blgrossMS Oct 9, 2017

blgrossMS Oct 9, 2017

jm991 left a comment

jm991 Oct 7, 2017

jm991 Oct 7, 2017

jm991 Oct 7, 2017

jm991 Oct 7, 2017

jm991 Oct 24, 2017

jm991 Oct 7, 2017

jm991 Oct 7, 2017

jm991 Oct 18, 2017

jm991 Oct 26, 2017

jm991 Oct 26, 2017

jm991 Oct 26, 2017

		@@ -64,6 +66,51 @@ public static List<T> ReadList<T>(this JsonReader reader, Func<T> deserializerFu
		return list;
		}

		@@ -1 +1 @@
		m_EditorVersion: 5.6.1f1
		m_EditorVersion: 5.6.0f3

KHR_materials_pbrSpecularGlossiness support #74

KHR_materials_pbrSpecularGlossiness support #74

Conversation

jm991 commented Oct 7, 2017 • edited

blgrossMS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jm991 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jm991 commented Oct 7, 2017 •

edited