[glTF2] Importer + Spec Conformity #1423
Conversation
|
I'm against the removal of KHR_materials_common for practical reasons. I'd rather have an implementation of a WIP spec than an approximation of these materials with PBR, which if I understand correctly is what the glTF2 exporter does here.
Can you expand on this? Assimp -> glTF2 -> Three.js was working fine for me. |
Thing is, I can't find a repo or a doc that details the In fact, most recently, there seems to be talk of not having a single Note: the materials_common models in the KhronosGroup's Sample Model repo also have incorrect As further guidance, the Unity Exporter and Blender Exporter from KhronosGroup do not implement
The glTF2 Exporter in assimp's master doesn't output I'm against the preservation of KHR_materials_common since it doesn't align with any work-in-progress spec, and there is no finalized -let alone ratified- spec. We should wait until the dust settles on the material extensions front. Meanwhile, pbrMetallicRoughness is part of the core spec, and pbrSpecularGlossiness is a ratified extension. We should target these for the moment.
Using the glTF validator from Khronos Group on an exported glTF2 suggests that much is invalid/incorrectly implemented via these errors {
"type": "MESH_PRIMITIVE_INVALID_ATTRIBUTE",
"path": "#/meshes/0/primitives/0/attributes",
"message": "Invalid attribute name `JOINT`."
}
{
"type": "MESH_PRIMITIVE_INVALID_ATTRIBUTE",
"path": "#/meshes/0/primitives/0/attributes",
"message": "Invalid attribute name `WEIGHT`."
}
{
"type": "NODE_WITH_NON_SKINNED_MESH",
"path": "#/nodes/34",
"message": "Node has `skin` defined, but `mesh` has no joints data."
}
{
"type": "BUFFER_VIEW_TARGET_OVERRIDE",
"path": "#/animations/31/samplers/2/output",
"message": "Override of previously set bufferView target or usage. Initial: `VertexBuffer`, new: `Other`."
}
{
"type": "ANIMATION_CHANNEL_TARGET_NODE_MATRIX",
"path": "#/animations/0/channels/0/target",
"message": "Animation channel can not target TRS properties of node with defined `matrix`."
}…and many more That being said, some of these errors are easy to fix ( It's possible Three.js has implemented the animations importing incorrectly. Just last week I found a small issue with their Mesh importing logic that was not to spec. |
|
My issue is, if I have an asset with this kind of "common" material, how can I (today) export that to glTF2 with assimp and successfully load it with loaders that are supporting a WIP spec? Why not just code against those loaders? I'm thinking only of Three.js here, so maybe that's a bit limited. But still, I don't see the harm in including the extension when the alternative is that people who need this functionality simply can't use assimp (or more realistically, that we have to maintain a branch).
My guess is not very far, since I was successfully exporting animations. I definitely don't think it should be removed just because the validator is complaining. |
You probably cannot do that today, and there should be no expectation that you can. For all intents and purposes, Besides the point, assimp currently doesn't export The only argument for keeping in (some form of) KHR_materials_common is that three.js supports it, but that's not a good argument. Especially since a few weeks ago they removed another extension (~~~Lights, I believe~~~ webgl_techniques, I believe) since its spec was incomplete. I can see three.js's implementation of There's no sense in generating glTF2 files that are, plain and simple, invalid.
You might be right. I don't think it should be removed, but the fixes should definitely be made to appease the validator and the spec. I might take a crack at the fixes shortly. |
Yes I can. Otherwise I wouldn't oppose taking out the extension! It's fine if Three.js is going to change its implementation -- all this stuff is obviously in flux. I don't see any practical reason not to at least try to keep up with the WIP spec.
I can certainly believe that, but I think the solution is to fix the implementation. |
One reason is because it's generating objectively invalid glTF files. Whether it's the current assimp output (coded against glTF 1.0 KHR_materials_common), or models generated with the format from KhronosGroup/glTF#947, or the the format from KhronosGroup/glTF#965, or the spec from KHR_materials_cmnBlinnPhong. They can -and likely will- all change or go away. pbrMetallicRoughness (part of core spec) and pbrSpecularGlossiness (official, ratified extension) are at least set in stone and will continue to work as long as there are loaders for glTF2.
As in my first paragraph: which work-in-progress spec do you target against, then? |
Practical was the key word. As long as I can load assimp output in Three.js, I think there's value to including the extension. What value does removing it have? Thus:
I was targeting Three.js's implementation, and would continue doing so.
I certainly have no objection to supporting these. |
But that's just it: you can no longer load current assimp glTF2 output in the current version (r87) of three.js. They may load for you, because your version of Three.js loads the incorrect implementation of KHR_materials_common, but not for others. I believe your
And even if we updated the implementation (if a work-in-progress spec could even be agreed upon): that spec is bound to change in the future, again breaking exported models. It's not wise to:
You're arguing for keeping something so that your pipeline will work, even if it's objectively incorrect.
The value is that it won't produce invalid glTF files that will be obsolete in the future (and are already obsolete) |
|
Sure, the current implementation is out of date. And if it's updated it will go out of date again. But I disagree that it's unwise to keep doing so. I need a pipeline that works now, so I'm very grateful that a) Three.js has implemented an incomplete spec and b) assimp has accepted an implementation of an incomplete spec. Without these implementations I wouldn't be able to use glTF 2. And it's not hard to imagine that there are other people in the same boat. But if you insist on taking this out, I'll just use my fork. It's not such a big deal regardless. And anyways, instead of just complaining about things I should also say: thank you for these additions and fixes! I'll look forward to using them when the KHR_materials_common/cmnBlinnPhong extension is ratified. 😛 |
Remove un-needed test models
pbrMetallicRoughness and pbrSpecularGlossiness as structs; persist textureinfo properties from start to finish; persist pbrSpecularGlossiness (via extensionsUsed) usage from start to finish
Currently incorrect, however. May need to be removed
glTF nodes can only hold one mesh. this simply assigns to and check’s a Node’s Mesh
collada can appear in many files, such as glTFs via the “generator” field (in the form of collada2gltf)
Binary glTF is now part of the glTF2 spec. However, it’s implemented incorrectly, so will be temporarily removed
Per spec TEXCOORD -> TEXCOORD_0 COLOR -> COLOR_0 JOINTS -> JOINTS_0 WEIGHTS -> WEIGHTS_0 Remove JOINTMATRIX since it’s not supported (and doesn’t seem to be output, anyway) TANGENT should be added at a later date
dhritzkiv
force-pushed
the
feature/gltf2
branch
from
ae5f80e
to
86a8a58
Compare
|
The linux build seems to be broken. Could you take a look which kind of issue has occurred? And how should we handle the removal of the KHR_materials_common-extension. Would it be possible to provide it as a special "post-process" ? |
Segfaults on Linux for some reason. No other tests test exporting, so it’s fine
For some reason, it segfaults during an scene export test when run on linux. OS X is fine. I removed that test, since no other format unit tests test exporting. This seems to have fixed the linux builds.
I removed
I'm not sure how to achieve this, as I'm not too familiar with the post process filter system. However, once |
|
You can't just blindly disable unit tests because they fail. That's how you get PHP. Here's a Valgrind backtrace of the failure: Here we can see a use-after-free bug. Looking at the code in glTF2Exporter.cpp the reason is that you're creating a temporary |
This reverts commit 4b01eca.
Keep std::string alive
|
@turol Thanks for pointing out the source of the segfault. I couldn't reproduce it on macOS using clang, so I didn't know where to start. I also didn't know about Valgrind. So thanks for showing me! The reason I removed the test (that I introduced) was that I figured since nearly all the other format unit tests didn't have exporting tests, then maybe there was a reason for that. Anyway, seems the CI tests are still segfaulting. I'll keep looking for other causes. |
|
On macOS you should be able to use AddressSanitizer. There's still more bugs in there, some of which probably aren't your fault. On pastebin because it's damn long. |
|
Hey @turol. I took stab at addressing a couple of the glTF2-specific issues. The segfault in the export unit test was caused by an incorrect use of the |
|
I've fixed one of the issues with glTFAsset as part of #1427. Feel free to cherry-pick that commit into this one if you want, that PR might be a while since it enables Asan on Travis and cleaning up the issues will take some effort. |
|
Forget that, it seems that MSVC can't deal with shared_ptr of an array while clang can. |
|
So, I guess I will merge it. We can use the 4.0.1 version for the HR_materials_common-extension. So when we want to support it we can get the code from there. |
|
Thanks a lot for the contribution. Please create issues for all the missing extensions. |
dhritzkiv
changed the title
This support adds glTF2 Importing, and improves conformance to the spec.
New Features
scaleproperty on Normal texturesstrengthproperty on Occlusion texturesMajor changes
meshproperty of a node and notmeshesMinor changes
byteStrideonbufferViewonly gets written if it's> 0wrapSandwrapTin samplers only get written if notRepeatdoubleSidedand many other material propertiesmagFilterandminFiltercan be undefined, and now properly import and export (previously mag was always set to Linear and min was always to Linear-Linear, no matter the input.versionis now stored as a string (reduces the amount of string<->number comparisons/conversions)To dos (maybe someone else can take these on)
extrasvalues for each resource.Notes
pbrMetallicRoughness
glTF 2 focuses on supporting PBR materials with its default material definition (as well as additional PBR properties via the specularGlossiness extension). Because these material properties are not equivalent to existing (Assimp) material properties, such as diffuse, specular, etc., I had to create lots of custom named material properties, such as:
This facilitates persisting those values in a glTF2 -> glTF2 export flow. These properties are scoped to only glTF2 importers/exporters. In the future, perhaps Assimp can define more material properties (such as
metallicFactor,roughnessTexture, etc.), and this special naming can be removed to allow cross format PBR support. I believe Blender supports PBR in its format now.Importing from glTF2 and exporting to another format does a pretty good job of maintaining appearance of materials (e.g.
baseColorFactoris used for diffuse)Exporting glTF2 from another format also does a pretty good job of maintaining appearance.
pbrSpecularGlossiness
Like with pbrMetallicRoughness, I've added support for the pbrSpecularGlossiness extension to allow for glTF2 -> glTF2 exporting in assimp, where specularGlossiness properties are persisted.
Other
I'm not a C++ programmer, so apologies if I didn't follow convention in a lot of this code. I tried to maintain consistency wherever possible, but I'm sure I made many mistakes.
Resources