CgltfImporter: import unrecognized material extensions #117
Conversation
|
Forgot to mention: there are 3 minor AssimpImporter changes in there that I noticed while working on the same code here. Hope that's OK, but I can separate them out too. The failing linux-sanitizers CI is me not cleaning up after cgltf's parsing function that mallocs random things, already fixed locally. |
Codecov Report
@@ Coverage Diff @@
## master #117 +/- ##
==========================================
+ Coverage 95.56% 95.71% +0.14%
==========================================
Files 115 115
Lines 10830 11158 +328
==========================================
+ Hits 10350 10680 +330
+ Misses 480 478 -2
Continue to review full report at Codecov.
|
Still works, yay
- this shouldn't be an error, it doesn't fail the import - with really large names, the printed size can be small, making this message very confusing
Why didn't I test this?
This handles invalid primitives (those are not thoroughly checked by jsmn) the same way as cgltf. Numbers are parsed up until the first invalid characters, or 0 if there are no valid characters.
In preparation for importing custom material texture attributes from unknown extensions. Using raw names is identical to the built-in enum values, no changes to tests needed.
Each extension gets its own layer, extension object properties are imported as custom attributes. To keep our sanity, the following types are not imported, we just print a warning and ignore the attribute: - arrays with size 0 or > 4 - non-primitive arrays (strings, objects, nested arrays) - bool arrays, not supported by MaterialAttributeType and awkward to parse when mixed with numbers - objects that don't end in "Texture" and resolve to a textureInfo (according to the glTF schema) type
Treat them like unknown extensions and import their attributes with the original glTF names. Some of these may turn into a standard MaterialAttribute or MaterialLayer in the future, but for the time being this makes them accessible without changes to magnum. Tests will follow in another commit.
This causes funny sorting issues inside MaterialData, and shouldn't happen in practice anyway
…tributes Also no need for those extra attributes, all types are checked in MGNM_material_type_zoo
cgltf allocates memory for extensions inside cgltf_parse_json_texture_view(). We don't use that, so we can free it right away.
We treat these as "raw" material layers, so just import whatever is there without divining too much about their meaning
There are no wrappers for these material types/layers, so we don't have to test all these interactions that the clearcoat extension is going through. Just checking the defaults and the texture import.
Previously this only accepted objects with a single "source" attribute. Now it skips all unknown attributes and handles duplicate "source" attributes. This makes it behave similarly to TinyGltfImporter.
Same as vertex attributes and material extensions, duplicate attributes overwrite previous ones. This requires parsing before-hand, then sorting, then importing attributes. It adds some overhead and is incredibly unlikely to matter in practice, but not handling it would lead to asserts in MaterialData, and we don't want that.
pezcode
force-pushed
the
cgltf-custom-material
branch
from
2eddf85
to
d3a920c
Compare
We don't need all these if/else blocks anymore, we can continue; out of the attribute loop
pezcode
force-pushed
the
cgltf-custom-material
branch
from
58512cd
to
06016e6
Compare
There was a problem hiding this comment.
Thank you!
The comments regarding number parsing are there because I was doing an investigation for custom scene fields and realized that any attempt to detect numeric type could lead to unnecessary variability and friction in user code. In my case of scene fields it goes even further, where I have things like the following, i.e. can't even assume the tags is a vector because it can have a different number of items in every case and depending on the scene it could be presented as a float, vector2 or whatever:
"extras": {
"id": "79098824816e98077c45a760ff59ef946884d09d",
"name": "Flower Pot, very bigly",
"objectType": "furniture",
"tags": [666, 1212, 3, 48]
},Fortunately / hopefully in case of materials when there's a numeric array we can assume it's indeed meant to be a fixed-size vector and not an arbitrarily long array. (TODO for me: add an ability to store arbitrarily-sized arrays in MaterialData.)
Related -- materials can have extra fields as well (and I have files with those, containing various string identifiers usually), but I don't see the PR handling those? Those would be present in the base layer and i'd limit the support to just bools, numbers, strings and 2/3/4-component number arrays, i.e., not assuming the extra fields could contain texture objects.
there are 3 minor AssimpImporter changes in there that I noticed while working on the same code here. Hope that's OK
Yep, this is fine. The PR is just a messenger, what matters in the end is the actual patch set, and as long as those are in separate commits it's no problem that they're in the middle of something else.
Would it make sense to add something like
ignoreUnrecognizedMaterialDatainAssimpImporter?
I don't see a reason right now -- the extra material attributes are not just random noise in this case. It might make sense once there's glTF export, but even then the goal is preserving as much as possible. Cleaning up custom attributes might be better as a standalone utility in SceneTools, for example.
This is the preferred name, in case it gets added as an approved glTF vendor prefix in the future
Because consistently detecting if a number is int, unsigned int or float is impossible. Integer attributes may use exponent notation and decimal points followed by a 0, leaving us at the mercy of the glTF exporter used for the asset. Note that integer attributes in texture objects (texture index, coordinate set) are unaffected because we know the expected type.
For better diagnostics, this prints the string if it's not empty
Ideally, this would become DebugTools::CompareMaterial in the future. Only doing this for custom materials because base material tests aren't as verbose thanks to convenenience accessors.
I kind of forgot about that 🙊 Importing those turns out to be not quite as straightforward, because they can be any token type, including objects. Blender, for example, uses it to export custom attributes which then looks like this: "extras" : {
"prop1" : 2.6,
"PROP2" : 6.9
}This can be imported as another layer "Extras". But if, instead, it's just a single string or primitive, do we import that as "Extras" in the base layer? Or as "Extras" in the "Extras" layer for consistency? Opinions? There's also the issue of how the concept of opaque metadata can be extended to other resource types but that might be a discussion for another day. |
Should've just copied straight from the review snippet.
|
Sorry for the review delay, it definitely wasn't as time-consuming as I anticipated 🙈
Err, not sure I understand what you mean? Yes, the assumption is that "extras": {
"id": "79098824816e98077c45a760ff59ef946884d09d",
"name": "Flower Pot, very bigly",
"objectType": "furniture",
"tags": [666, 1212, 3, 48]
},If the
Nope, in your case it'd be the base layer getting new float attribute Sorry for the confusion, hope it's clearer now :) |
|
Ah, unfortunate misunderstanding on my part. I didn't even realize your snippet had |
The existing tests only tested one extensions that had ALL the warnings, but not that parsing still works
The extras type can be any valid JSON token, but we ignore anything that's not an object. Type support and conversion is the same as unrecognized material extensions, except we ignore ALL nested objects, including textureInfo. Attributes are written directly into the material base layer.
pezcode
force-pushed
the
cgltf-custom-material
branch
from
7c7e155
to
b85ea40
Compare
|
Excellent work as always, thank you 👍 Merged as e5eb856...ad289da. I squashed some commits to have code and corresponding docs/tests together as that makes bisects easier, but that was basically the only "cleanup" needed. Additionally in f9e10f4 I made a behavioral change where empty extension objects are preserved as well -- this is to support use cases similar to Tested on a random extension-heavy dataset I have here, and it's just ✨ perfect ✨ : |
Similarly to #116, this adds the ability to import unrecognized material data as custom attributes. Each extension object is imported as a custom layer (with a
#prefix because uppercase layer names are reserved, and all glTF extensions start with uppercase), with JSON properties imported as custom attributes.Attributes can be:
bool,Float,Int,UnsignedInt,StringFloat,Int,UnsignedInt, imported asVector2/3/4[u][i]textureInfoobjects ending in"Texture"Any other type of attribute is ignored. This is tested fairly extensively, but if you see any corner cases missing, let me know.
Extensions that are parsed by cgltf but don't map to known
MaterialLayer/MaterialAttributes get the same treatment, with attributes imported with the original glTF extension spec names.Since I didn't need it for testing, there's currently no config option to turn off the unrecognized material parsing. Would it make sense to add something like
ignoreUnrecognizedMaterialDatainAssimpImporter?Todos
A few remaining issues, but no major changes: