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

SimplifyModifier: Move to BufferGeometry. #21067

Merged
merged 2 commits into from
Jan 12, 2021
Merged

SimplifyModifier: Move to BufferGeometry. #21067

merged 2 commits into from
Jan 12, 2021

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Jan 12, 2021

Related issue: -

Description

SimplifyModifier no longer relies on Geometry. The modifier now has a dependency to BufferGeometryUtils.

@mrdoob mrdoob added this to the r125 milestone Jan 12, 2021
@mrdoob mrdoob merged commit 23f9bab into mrdoob:dev Jan 12, 2021
@mrdoob
Copy link
Owner

mrdoob commented Jan 12, 2021

Thanks!

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 12, 2021

I had a look at SubdivisionModifier today and it's tricky to honor texture coordinates when using BufferGeometry.

The problem is the algorithm assumes vertices are merged. And this is possible with Geometry even if texture coordinates a different for two or more identical vertices. However, this does not work when performing the merge with BufferGeometry since all vertex data (normals, colors, uvs) have to match.

Since you can only use BufferGeometryUtils.merge() for an efficient vertex merge when dropping all other vertex information, a conversion to BufferGeometry only works if we remove features from the modifier.

@mrdoob
Copy link
Owner

mrdoob commented Jan 12, 2021

Thanks for taking a look 🙏

Since SubdivisionModifier never got to work properly for Geometry either, what do you think about just removing it?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 12, 2021

Even better! 😊

@mrdoob
Copy link
Owner

mrdoob commented Jan 12, 2021

Lets do that then! 🙌

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 12, 2021

I'll make a PR 👍 .

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.

None yet

2 participants