Changed Assimp binding to Silk.Net.Assimp #1158
Conversation
|
Things left:
Issues present in both old/new versions
|
| else | ||
| { | ||
| if (AllowUnsignedBlendIndices) | ||
| ((byte*)(vbPointer + blendIndicesOffset))[bone] = (byte)vertexIndexToBoneIdWeight[(int)i][bone].Item1; |
There was a problem hiding this comment.
This one seems to need a check for the bone index since the resulting list might end up having a size of less than 4
|
|
||
| var totalWeight = 0.0f; | ||
| for (var boneId = 0; boneId < nbBoneByVertex; ++boneId) | ||
| totalWeight += curVertexWeights[boneId].Item2; |
There was a problem hiding this comment.
Same size issue here and on line 882.
Maybe can be corrected on line 872?
There was a problem hiding this comment.
Good catch, had a translation error on the line above where i just set Capacity on the list instead of adding empty elements like the original version does with curVertexWeights.resize(nbBoneByVertex, std::pair<short, float>(0, 0.0f));
|
|
||
| IComputeColor curComposition = null, newCompositionFather = null; | ||
|
|
||
| var isRootElement = true; |
There was a problem hiding this comment.
So if i understood well, a material element is root if it's not currently referencing the previous element of the stack, or it's not being referenced by the next element of the stack.
| var isRootElement = true; | |
| //var isRootElement = true; Should go in the loop | |
| bool isRootElement = top.type != Material.StackType.Operation && stack.Peek().type != Material.StackType.Operation; | |
| curCompositionFather = compositionFathers.Pop(); | ||
| } | ||
|
|
||
| var set = sets.Pop(); |
There was a problem hiding this comment.
This one is removed too early, if the current material element is not result of an operation there's no need to pop this one here
|
|
||
| if (type == Material.StackType.Operation) | ||
| { | ||
| var realTop = (Material.StackOperation)top; |
There was a problem hiding this comment.
| var realTop = (Material.StackOperation)top; | |
| var set = sets.Pop(); | |
| var realTop = (Material.StackOperation)top; |
|
I got to a point where assimp loads a gltf mesh, albeit very broken (looks like a vertex declaration issue), will continue to work on it. |
Did you try the assimp loading flags, for example FlipWinding? It's important to check the intended coordinate system for a mesh and convert it to strides coordinate system... If the handedness changes, the winding has to be flipped, and/or the normals. Ideally these are settings or checkboxes in the loading dialogue. |
|
Will check this out, it should all match the existing code base just moved to C# from C++, but quite possible that I made some errors when converting : ) If it matches the existing code base then I would prefer to fix stuff in a later pass. |
|
Sure! Those are only suggestions, I've only tested gltf. |
|
Tested gltf models from https://kaylousberg.itch.io/kaykit-dungeon seems to be working, skeletal meshes import but the skeleton itself is correct, bone weights looks fine though as mesh is correct if I switch to skeleton imported from FBX instead. Will have to check if the issue exists in the old assimp importer or not : ) EDIT: Same issues present in old importer. |
|
FYI A lot of this code doesn't seem Stride-specific. It's an ongoing issue that the C API that we bind to is pretty thin, and isn't easy to work with. As such, if you'd like feel free to contribute some of the wrappers you've made back to Silk.NET! Issue: dotnet/Silk.NET#249 |
johang88
force-pushed
the
SilkNetAssimp
branch
from
5afd321
to
efba514
Compare
* Converted Stride.Importer.Assimp to a C# library and changed assimp binding to Silk.Net.Assimp * Converted Stride.Importer.Common to a C# library as this did not contain any native code
|
@Eideren Please close this since my PR based on Johan's work was merged 😊 |
|
Thank you @johang88 for the initial work |
PR Details
Changed the Assimp native binding to Silk.Net.Assimp.
Description
Changed the Assimp native binding to Silk.Net.Assimp and converted the importer to a native C# library. I have also converted the
Stride.Importer.Commonlibrary to a native C# library as it contained no native code.This is a WIP PR and it currently lacks some of the material import code, otherwise it's close to a 1 to 1 conversion of the previous Assimp importer.
Motivation and Context
This change allows us to remove much of the C++/CLR code used by the asset compiler and brings us one step closer to running the asset compiler on non windows systems.
Types of changes
Checklist