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

Add missing indices when decomposing faces #74

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MPanknin
Copy link
Contributor

Hi again,

we would like to suggest a small change in the decomposition of faces. In some dxf files we experienced minor rendering artifacts because the surfaces were not closed properly.

UI
Before:
before

After:
after

Thx

@vagran
Copy link
Owner

vagran commented Jul 11, 2023

Hi,
This fix does not look valid. What you do is adding third triangle, so ending up with three overlapping triangles (you can check resulting topology with sceneOptions.wireframeMesh option set to true). Instead it should be investigated, how to ensure proper quad decomposition into two triangles. I have already faced similar problems and tried to modify decomposition algorithm to cover all cases, but seems it is still not robust enough for a wild files. DXF does not specify vertices order of a quad, seems it may be completely arbitrary. It would be nice to have a sample file to properly fix this issue.

@MPanknin
Copy link
Contributor Author

We did not check the resulting mesh topology, to be honest, because this solution simply works in our use case.

It would be nice to have a sample file to properly fix this issue.

I will see if I can find one that exhibits this behavior if that would help?

@vagran
Copy link
Owner

vagran commented Jul 12, 2023

I will see if I can find one that exhibits this behavior if that would help?

Yes, that will definitely help to find proper solution.

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