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

Improved the core model simplification algorithm #28

Merged
merged 3 commits into from
Mar 22, 2020

Conversation

bawar9
Copy link
Contributor

@bawar9 bawar9 commented Mar 18, 2020

Hi, these changes are based on a research paper that augments the original paper by Dr Garland. You can read it here: https://www.hindawi.com/journals/mpe/2015/428917/. This also solves the issue listed here to some extent #19. See for yourself below.

BEFORE MY CHANGES

LOD Level (Quality 0.55 - Total Triangles after reduction 1208)ca

Capture

AFTER MY CHANGES

LOD Level (Quality 0.55 - Total Triangles after reduction 1208)ca

Capture2

Please note that this is not meant to exclusively solve the boat problem originally reported here
#19. In fact the issue with the boat model is still there and something tells me that it's related to the vertex linking.

…takes into consideration the discrete curvature to the

quadric error metrics in order to retain detailed features in model simplifcation. Based on a paper by Li Yao and  Shihui Huang.
@Whinarn
Copy link
Owner

Whinarn commented Mar 18, 2020

Hi @bawar9,

Thank you for your contribution!
I'll try to go over the code and test this sometime this week and get back to you.

@bawar9
Copy link
Contributor Author

bawar9 commented Mar 19, 2020

Hi @bawar9,
Thank you for your contribution!
I'll try to go over the code and test this sometime this week and get back to you.

Thank you. I also found a solution to the 2nd problem I reported here:
#27 After you merge this branch I'll make another pull request for that as well

Copy link
Owner

@Whinarn Whinarn left a comment

Choose a reason for hiding this comment

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

Great job on this!
I have tested it, and it does indeed improve the simplification in some cases.
There are a few performance boosts that you can do (as I comment on below), and a few code style fixes.
I hope that you don't mind, but I would like to try to optimize this even further after it has been merged.

Runtime/MeshSimplifier.cs Outdated Show resolved Hide resolved
Runtime/MeshSimplifier.cs Outdated Show resolved Hide resolved
Runtime/MeshSimplifier.cs Outdated Show resolved Hide resolved
Runtime/MeshSimplifier.cs Outdated Show resolved Hide resolved
Runtime/MeshSimplifier.cs Outdated Show resolved Hide resolved
Runtime/MeshSimplifier.cs Outdated Show resolved Hide resolved
Runtime/MeshSimplifier.cs Outdated Show resolved Hide resolved
Runtime/MeshSimplifier.cs Outdated Show resolved Hide resolved
bawar9 added 2 commits March 22, 2020 18:03
…riangle utility functions to take reference type parameters instead of value type structs.
@bawar9
Copy link
Contributor Author

bawar9 commented Mar 22, 2020

Hi, thank you for pointing out the issues and sorry for being sloppy. Only now did I realize that the "Triangle" and "Vertex" are structs and not classes, If I knew that earlier I would've passed them by reference to all the new methods and please do let me know if you find any other optimization hotfix. I have also removed the normal calculation method and have made all the other requested changed as well.

Copy link
Owner

@Whinarn Whinarn left a comment

Choose a reason for hiding this comment

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

Sorry for the small delay.

I reviewed your latest changes a couple of hours ago, but wanted to verify the algorithm on the page that you linked and test it out again. It all looks good from my side.

Thanks again for your contribution.

@Whinarn Whinarn changed the base branch from master to develop March 22, 2020 18:50
@Whinarn Whinarn merged commit fd95224 into Whinarn:develop Mar 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants