Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accelerate the Merge vertex post processing step #4527

Merged
merged 6 commits into from May 14, 2022

Conversation

motazmuhammad
Copy link
Contributor

@motazmuhammad motazmuhammad commented May 13, 2022

The function int JoinVerticesProcess::ProcessMesh( aiMesh* pMesh, unsigned int meshIndex)
can be accelerated by using an std::unordered_map instead of binary search to create the unique vertices.
To do that the following was done.

  1. Define a hash for a vertex
  2. Define an equal_to for a vertex
  3. loop over all the vertices if the vertex is already taken use the map to get its index, else add it.

Effectively, this should reduce the complexity from O(nlog(n)) to O(n). Therefore, accelerating the postprocessing step

Copy link
Member

@kimkulling kimkulling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine, I will merge it,

@kimkulling
Copy link
Member

kimkulling commented May 14, 2022

Nice approach, hopefully, the memory consumption will not explode for big models. But this will be a different story.

@kimkulling kimkulling merged commit c8dafe0 into assimp:master May 14, 2022
@kimkulling
Copy link
Member

Merged, thanks a lot for your contribution.

@motazmuhammad
Copy link
Contributor Author

motazmuhammad commented May 14, 2022 via email

@CarpetHead
Copy link

CarpetHead commented Aug 23, 2022

We are having an issue in assimp v5.2.4 where the merge vertices postprocessing step breaks rigged models. Could this PR be the cause? We are now seeing duplicated vertex weights. See attached image for 5.2.4 and 5.2.3 vertex weights per bone
image

@nezticle
Copy link

We are having an issue in assimp v5.2.4 where the merge vertices postprocessing step breaks rigged models. Could this PR be the cause? We are now seeing duplicated vertex weights. See attached image for 5.2.4 and 5.2.3 vertex weights per bone image

We have experience this same thing in Qt Quick 3D with 5.2.4. It breaks Morph animations because all of the weights are identical. We're reverting to 5.2.3 for the time being.

@JG-Adams
Copy link
Contributor

JG-Adams commented Aug 26, 2022

I have not experienced this issue. And I'm not sure how to recreate it.
But I tried to understand the change made here. It look like inside JoinVerticesProcess::ProcessMesh the mechanism to avoid doubles was replaced with something outside of it? Where does it function in?
unordered map by itself shouldn't take more than one of any kind.
This need some explanation, because it don't make a whole lot of sense to me.
We should review the old code. Because I notice the comment says it does something important that otherwise would mess up the mesh.

Edit: then again. It shouldn't have caused any problem should it?

@inhosens
Copy link
Contributor

This patch set does not compare color, texcoords and anim meshes. I am not sure it can accelerate the routine when the whole comparisons are added.

@CarpetHead
Copy link

btw it looks like this PR fixed the issue https://github.com/assimp/assimp/pull/4707/files

@JG-Adams
Copy link
Contributor

btw it looks like this PR fixed the issue https://github.com/assimp/assimp/pull/4707/files

That look like it does. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants