KhronosGroup / glTF-CSharp-Loader

C# Reference Loader for glTF
Other
219 stars 58 forks source link

Can't load files with EmissiveFactor greater than 1.0 #49

Closed luithefirst closed 1 year ago

luithefirst commented 1 year ago

I'm using the 1.1.4-alpha package from nuget and it fails to load certain files (like the one linked in the issue below). The exception I get is:

Unhandled Exception: Newtonsoft.Json.JsonSerializationException: Error setting value to 'EmissiveFactor' on 'glTFLoader.Schema.Material'.
 ---> System.ArgumentOutOfRangeException: Specified argument was out of the range of valid values.
   at glTFLoader.Schema.Material.set_EmissiveFactor(Single[] value)
   at Newtonsoft.Json.Serialization.ExpressionValueProvider.SetValue(Object target, Object value)
   --- End of inner exception stack trace ---
   at Newtonsoft.Json.Serialization.ExpressionValueProvider.SetValue(Object target, Object value)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.SetPropertyValue(JsonProperty property, JsonConverter propertyConverter, JsonContainerContract containerContract, JsonProperty containerProperty, JsonReader reader, Object target)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.PopulateObject(Object newObject, JsonReader reader, JsonObjectContract contract, JsonProperty member, String id)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObject(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.PopulateList(IList list, JsonReader reader, JsonArrayContract contract, JsonProperty containerProperty, String id)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateList(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, Object existingValue, String id)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.SetPropertyValue(JsonProperty property, JsonConverter propertyConverter, JsonContainerContract containerContract, JsonProperty containerProperty, JsonReader reader, Object target)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.PopulateObject(Object newObject, JsonReader reader, JsonObjectContract contract, JsonProperty member, String id)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObject(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.Deserialize(JsonReader reader, Type objectType, Boolean checkAdditionalContent)
   at Newtonsoft.Json.JsonSerializer.DeserializeInternal(JsonReader reader, Type objectType)
   at Newtonsoft.Json.JsonConvert.DeserializeObject(String value, Type type, JsonSerializerSettings settings)
   at glTFLoader.Interface.LoadModel(Stream stream)
   at glTFLoader.Interface.LoadModel(String filePath)

I found this issue (closed) in glTF repository: https://github.com/KhronosGroup/glTF/pull/1193

The range is checked here in the code (autogenerated): https://github.com/KhronosGroup/glTF-CSharp-Loader/blob/8aebe06eea6dc06fe79ac09b52c2ae5f6b7e6d61/glTFLoader/Schema/Material.cs#L190

It seems this has not been updated to the latest version. Please review the schema version and validation, thanks!

bghgary commented 1 year ago

This is by design.

EmissiveFactor greater than 1.0 is not allowed by the glTF core spec. The KHR_materials_emissive_strength extension was added specifically to address this issue.

luithefirst commented 1 year ago

Thanks for pointing this out, it was not clear to me how this issue has been resolved.