GLTF Importer: Build a list of the actual vertices so it works well with shared attribute lists #5003
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ith shared attribute lists
FlorianBorn71
commented
Mar 9, 2023
FlorianBorn71
commented
Mar 10, 2023
FlorianBorn71
commented
Mar 10, 2023
| ASSIMP_LOG_WARN("Positions of target ", i, " in mesh \"", mesh.name, "\" does not match the vertex count"); | ||
| } else { | ||
| aiVector3D *positionDiff = nullptr; | ||
| target.position[0]->ExtractData(positionDiff); | ||
| target.position[0]->ExtractData(positionDiff, vertexRemappingTable); | ||
| for (unsigned int vertexId = 0; vertexId < aim->mNumVertices; vertexId++) { | ||
| aiAnimMesh.mVertices[vertexId] += positionDiff[vertexId]; | ||
| } | ||
| delete[] positionDiff; | ||
| } | ||
| } | ||
| if (needNormals) { |
There was a problem hiding this comment.
I have to revisit whether the following block still uses the correct indexing of normals/tangets for animated parts
FlorianBorn71
commented
Mar 10, 2023
| @@ -380,7 +380,7 @@ TEST_F(utglTF2ImportExport, importglTF2PrimitiveModeLines) { | |||
| const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/glTF2/glTF-Asset-Generator/Mesh_PrimitiveMode/Mesh_PrimitiveMode_08.gltf", aiProcess_ValidateDataStructure); | |||
| EXPECT_NE(nullptr, scene); | |||
| EXPECT_EQ(scene->mMeshes[0]->mNumVertices, 4u); | |||
| std::array<unsigned int, 5> l1 = { { 0u, 3u, 2u, 1u, 0u } }; | |||
| std::array<unsigned int, 5> l1 = { { 0u, 1u, 2u, 3u, 0u } }; | |||
| EXPECT_EQ(scene->mMeshes[0]->mFaces[0].mNumIndices, 2u); | |||
There was a problem hiding this comment.
extracting the used vertices changes the order of vertices and thus the index buffer
|
@kimkulling , what do you think about this improvements? |
|
I have started to take a look on it. Stay tuned :-). |
kimkulling
approved these changes
Apr 3, 2023
|
Merged, thanks a lot for your contribution. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses this bug:
Out-of-memory for some gltf exported with "Sketchup glTF Exporter by Centaur"
It shows that the memory consumption during import for said customer model now goes down from >800GM (->out-of-memory) to <700MB, so more than 1000x !
In my opinion this change makes sense, but I wanted to ask if this looks like a good approach to the problem for you as well.
Of course this needs more testing with other assets,