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

Exposed simplify_mesh_lossless method #34

Merged
merged 7 commits into from
Oct 20, 2024
Merged

Conversation

Kramer84
Copy link
Owner

@Kramer84 Kramer84 commented Oct 4, 2024

This PR exposes the simplify_mesh_lossless method, which appears to be faster and more effective compared to the standard method with the lossless flag enabled. It also behaves differently in terms of triangle reduction and convergence. Additionally, the threshold_lossless argument in the standard method does not seem to influence the results as expected—triangle counts converge to a fixed value regardless of the threshold. However, in the simplify_mesh_lossless method, the epsilon parameter has a clear effect on the result.

This new method may be especially useful in batch processing scenarios where multiple meshes with similar characteristics need to be simplified efficiently.

On a minor note, the example comments at the end of the Simplify.pyx file have been removed as present in the readme. And it bugs me as to why this hasn't been done before, as I may have a mistake in closing this previous PR ...

Observations:

  1. Standard Method with lossless Flag:
    The lossless flag, when used with different threshold_lossless values, results in minimal triangle reduction regardless of the threshold value.

    • Example tests:
      simp.setMesh(bunny.vertices, bunny.faces)
      simp.simplify_mesh(preserve_border=False, lossless=True, threshold_lossless=1e-3)
      Output:
      iteration 0 - triangles 112402 threshold 0.001
      ...
      iteration 95 - triangles 112402 threshold 0.001
      simplified mesh in 0.9819 seconds from 112402 to 112400 triangles
      
      Similar results were obtained for higher threshold values (e.g., threshold_lossless=10), with the mesh consistently reducing to 112380-112400 triangles.
  2. Basic Exposed simplify_mesh_lossless Function:
    This method simplifies the mesh more significantly and efficiently compared to the lossless flag approach.

    • Example test:
      simp.setMesh(bunny.vertices, bunny.faces)
      simp.simplify_mesh_lossless()
      Output:
      lossless iteration 0
      ...
      lossless iteration 5
      simplified mesh in 0.1606 seconds from 112402 to 59960 triangles
      
  3. simplify_mesh_lossless with Different epsilon Parameters:
    Changing the epsilon parameter in simplify_mesh_lossless clearly influences the degree of simplification:

    • Epsilon = 1e-9:

      simp.setMesh(bunny.vertices, bunny.faces)
      simp.simplify_mesh_lossless(epsilon=1e-9)

      Output:

      simplified mesh in 0.0851 seconds from 112402 to 112124 triangles
      
    • Epsilon = 1e-3:

      simp.setMesh(bunny.vertices, bunny.faces)
      simp.simplify_mesh_lossless(epsilon=1e-3)

      Output:

      simplified mesh in 0.1277 seconds from 112402 to 59960 triangles
      
    • Epsilon = 5*1e-1:

      simp.setMesh(bunny.vertices, bunny.faces)
      simp.simplify_mesh_lossless(epsilon=5*1e-1)

      Output:

      simplified mesh in 0.1603 seconds from 112402 to 9466 triangles
      

Conclusion:

The simplify_mesh_lossless method offers a faster and more predictable alternative to the standard simplification with the lossless flag. For cases involving large batches of meshes or cases where consistent simplification is needed, it may be a more suitable option. Further testing could explore whether this method could replace the current lossless flag implementation in certain scenarios, without creating an other method.

PS : Sorry for the trailing white spaces, sublime text removes them automatically. >< Only the new cdef and class method have been added.

@Kramer84 Kramer84 added the enhancement New feature or request label Oct 4, 2024
@Kramer84 Kramer84 requested a review from bwoodsend October 4, 2024 19:53
@bwoodsend
Copy link
Collaborator

Looks good to me. We'll want to adjust the verbose handling to match the changes from #28.

@Kramer84
Copy link
Owner Author

Kramer84 commented Oct 6, 2024

Normally I've adjusted the verbose handling for the other method. Maybe before merging the readme file should be updated with the logging + the new lossless method, and maybe a link to this PR for the testing data.

@Kramer84 Kramer84 marked this pull request as ready for review October 6, 2024 20:22
@bwoodsend
Copy link
Collaborator

Yeah, I think it's important that the logging thing gets mentioned or, given that the debug messages are suppressed by default, nobody is even going to know they're there.

And yes, an example of simplify_mesh_lossless() too. I'd say no though to a link to the PR – people shouldn't need to hop around GitHub to learn an API that's essentially just two functions.

@bwoodsend
Copy link
Collaborator

Just temporarily distracted or do you want me to finish this one off?

@Kramer84
Copy link
Owner Author

Kramer84 commented Oct 19, 2024

Just temporarily distracted or do you want me to finish this one off?

Sorry, wasn't really available the last days, and wanted to take some time to make the readme reflect the changes, which should be ok now! Maybe you can check that just one last time. Also, should we pass to a new version, for the new method + verbose change, maybe a 0.3.0 ?

And add an issue concerning the implementation of the preserve borders option for the lossless method.

@bwoodsend
Copy link
Collaborator

Mind adding a test for simplify_mesh_lossless()? Looks good otherwise.

Also, should we pass to a new version, for the new method + verbose change, maybe a 0.3.0 ?

I'll do a release after this. You can either change the version now or I'll do it then. It doesn't really matter.

@Kramer84 Kramer84 merged commit 9be728c into master Oct 20, 2024
36 checks passed
@Kramer84 Kramer84 deleted the exposing-other-methods branch October 20, 2024 14:10
@Kramer84
Copy link
Owner Author

I let you do the rest, I hope it was ok to merge !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants