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

Examples: Remove SubdivisionModifier. #21072

Merged
merged 1 commit into from
Jan 13, 2021
Merged

Examples: Remove SubdivisionModifier. #21072

merged 1 commit into from
Jan 13, 2021

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Jan 12, 2021

Related issue: see #21067 (comment)

Description

This PR removes SubdivisionModifier.

@mrdoob mrdoob added this to the r125 milestone Jan 13, 2021
@mrdoob mrdoob merged commit 8492cd1 into mrdoob:dev Jan 13, 2021
@mrdoob
Copy link
Owner

mrdoob commented Jan 13, 2021

Thanks!

@arpu
Copy link

arpu commented Jan 13, 2021

anyone knows a replacement for this? best for Buffer Geometry

@donmccurdy
Copy link
Collaborator

Offline subdivision with tools like gltfpack, Blender, or RapidCompact would be my recommendation. The implementation of SubdivisionModifier was never really robust even for Geometry, see #21067 (comment).

@arpu
Copy link

arpu commented Jan 13, 2021

my use case is generate a Terrain mesh from height map image generated mesh optimize with @mapbox/martini (https://github.com/mapbox/martini)
and after this optimization i use

		const modifier = new THREE.SubdivisionModifier( 1 );
		let smoothGeometry = modifier.modify( martiniGeo );
		martiniGeo = new THREE.BufferGeometry().fromGeometry( smoothGeometry );

all is done on client side user browser

@arpu
Copy link

arpu commented Jan 13, 2021

@donmccurdy
Copy link
Collaborator

donmccurdy commented Jan 13, 2021

@mapbox/martini provides control over the error and resolution of the mesh, I think — can you not generate a smoother mesh with a higher setting? You might also want to look at https://www.npmjs.com/package/taubin-smooth, where cells are indices, which should work on data from martini.

I don't believe the three.js maintainers are able to provide a subdivision library right now. If someone would like to move SubdivisionModifier into a new repository, fix the known issues with it, and add support for BufferGeometry, that would probably be a useful project in its own right. The hard part is properly supporting UVs and other vertex attributes.

@arpu
Copy link

arpu commented Jan 13, 2021

thx a lot @donmccurdy for the clarification! will test the alternatives

@donmccurdy
Copy link
Collaborator

If someone would like to move SubdivisionModifier into a new repository, fix the known issues with it, and add support for BufferGeometry, that would probably be a useful project in its own right.

On second thought — finishing the work described by PixarAnimationStudios/OpenSubdiv#247 and getting OpenSubdiv compiled to WASM would be even better. That's a robust subdivision implementation, and would be valuable to have available in the web ecosystem.

@changkun

This comment has been minimized.

@changkun
Copy link

changkun commented May 4, 2021

Hi, sorry for commenting again. How could my previous comment become disruptive content? We are in a university and teaching students for using your excellent work, thus we appreciate your work a lot. but removing it without providing an alternative solution actually breaks the educational content, and very unfortunately there are no provided solutions from what I saw in this thread. Of course, the OpenSubdiv is an alternative solution but the setup cost for students goes much higher. The only solution would install v124, but it is actually discouraged from the community, am I right?

@arpu
Copy link

arpu commented May 4, 2021

not sure from where his comes from https://github.com/mtnguru/ab/blob/1cd76561c4780a69e4db6e3287abec384499897d/js/threejs/modifiers/BufferSubdivisionModifier.js

@arpu
Copy link

arpu commented May 4, 2021

@donmccurdy would this be a good alternative?

@changkun
Copy link

changkun commented May 4, 2021

@arpu Looks great, thanks a lot! is it possible to merge it back or at least document the breaking change?

@donmccurdy
Copy link
Collaborator

donmccurdy commented May 4, 2021

three.js is an open source library and relies on contributors to maintain. SubdivisionModifier was effectively unmaintained, had serious bugs, and depended heavily on long-deprecated parts of three.js. With no contributors supporting something, that thing is likely to be eventually removed to allow the project to grow. The removal of SubdivisionModifier was documented under the r125 release notes.

If someone would like to test BufferSubdivisionModifier and maintain it, please feel free to open a PR. If there are no contributors willing to support a feature, it is unlikely to remain in three.js.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 4, 2021

The removal of SubdivisionModifier was documented under the r125 release notes.

And in the migration guide 😇 .

https://github.com/mrdoob/three.js/wiki/Migration-Guide#r124--r125

BTW: I think we should also discuss if new modifiers should be added to the repo (see #21014 (comment)). I see them in third-party repositories.

@stevinz
Copy link
Contributor

stevinz commented Jul 15, 2022

I recently implemented a new subdivision modifier for BufferGeometry using Loop subdivision. It does a pretty good job handling uv's, with an option to enable / disable the Loop averaging on uv coordinates.

I did play with a Catmull-Clark version as well but I'm not that happy with it and haven't included it in the repo for now... I was able to achieve better results using Loop with a pre-split pass before smoothing. This enables much better smoothing of boxes / sharp corners.

Github Repo - Online Example

This thread was one of the first I came across when googling for a working BufferGeometry subdivision modifier, I thought I'd drop a link here for others looking for something similar.

@mrdoob
Copy link
Owner

mrdoob commented Jul 27, 2022

@stevinz Hmm, do you think we could/should bring back SubdivisionModifier and use your implementation instead?

@stevinz
Copy link
Contributor

stevinz commented Jul 27, 2022

@mrdoob I think the SubdivisionModifier was a really nice addition to three.js. Since the switch to BufferGeometry and the removal of the modifier I've come across a number of discussions around the web searching for a replacement.

Personally, I feel pretty good about my implementation. It does a nice job with all the built-in geometries and was specifically tested using geometries with 'position', 'normal', 'uv', and 'color' attributes. It does attempt to handle other attributes contained within the geometry, but it has not been tested much against geometries with other attributes.

A few negatives I should mention, though:

  • Speed was not an overall concern within the implementation.
  • The algorithm prefers geometries with shared edges / vertices. This is true for all built-in geometries, but may not be with custom loaded geometries.
  • Internally the algorithm converts Indexed geometries to Non-Indexed before the algorithm proceeds. The new geometry returned is always Non-Indexed (although a new Indexed geometry could easily be generated using BufferGeometryUtils.mergeVertices).
  • There is no consideration for morphTargets / morphAttributes.

I know there was discussion about reducing the amount of code to be maintained within three.js. However, if there is interest, I can submit a PR to bring back the SubdivisionModifier and am willing to help maintain it as best I can. Or if it's preferred to keep the code base smaller, I can submit a PR for an External example as I've noticed has been done a few times recently, and can maintain the modifier on my repo.

@WestLangley
Copy link
Collaborator

Or if it's preferred to keep the code base smaller, I can submit a PR for an External example as I've noticed has been done a few times recently, and can maintain the modifier on my repo.

I vote for that, as it reduces the burden on the three.js maintainers.

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.

7 participants