Skip to content

Conversation

@arodic
Copy link
Contributor

@arodic arodic commented Sep 28, 2020

Currently TessellateModifier is not recursive and requires manual invocation of the modify( geometry ) multiple times like in the example here.

The problem with this is that you can't know when the maxEdgeLength parameter is satisfied and the tessellation is complete. So you have to guess how many iterations you need. In the example 6 seems to do the job, in some other cases any number of iterations between 0 .. n is needed.

This change contains the following:

  • Adds internal while ( ! finalized ) tessellation loop that runs until no edge with length > maxEdgeLength exists.
  • Instead of mutating geometry from modify( geometry ) argument, it returns new geometry (same as SubdivideModifier).

It also contains a minor change affecting both TessellateModifier and SubdivideModifier:

  • Output geometry is same type as input geometry (Geometry vs BufferGeometry)
  • Geometry.mergeVertices( precisionPoints ) now accepts optional precisionPoints argument.
  • Increased vertex merging precision to geometry.mergeVertices( 6 ); to avoid collapsing small triangles.

Questions:

  • Should precisionPoints for vertex merging also be a property of TessellateModifier and `SubdivideModifier?

@mrdoob mrdoob added this to the r122 milestone Sep 29, 2020
@arodic
Copy link
Contributor Author

arodic commented Sep 29, 2020

@mrdoob I also added maxFaces parameter as another way to limit number of faces more explicitly.

So constructor look like this now: new TessellateModifier( maxEdgeLength, maxIterations, maxFaces ) with all arguments being optional. It makes me wonder if these should be an object instead or perhaps arguments to modify() function?

Also, linting rules did not allow me to define constructor arguments as optional with default values in d.ts file. I tried this:

constructor( maxEdgeLength?: number = 0.1, maxIterations?: number = 6, maxFaces?: number = 100000 );

And I got error:

Error: src/core/Geometry.d.ts(248,17): error TS1015: Parameter cannot have question mark and initializer.
Error: src/core/Geometry.d.ts(248,17): error TS2371: A parameter initializer is only allowed in a function or constructor implementation.

Is this TS linting rule intended?

@gkjohnson
Copy link
Collaborator

Is this TS linting rule intended?

As far as I know you can't set default function parameters in d.ts files :(

@arodic
Copy link
Contributor Author

arodic commented Oct 4, 2020

As far as I know you can't set default function parameters in d.ts files :(

Yeah, I'm still learning about TS. It turns out you can set default values in the constructor but cannot make arguments optional and initialized at the same time. That kinda makes sense since it would be redundant.

Anyway, the lint error is fixed. TessellateModifier is recursive so you dont have to manually repeat the modify() function. And last but not least, you can increase maxIterations and maxFaces if you dont care about computation time.

@mrdoob
Copy link
Owner

mrdoob commented Oct 27, 2020

@arodic any chance you could resolve the conflicts?

@mrdoob mrdoob modified the milestones: r122, r123 Oct 28, 2020
@arodic
Copy link
Contributor Author

arodic commented Nov 9, 2020

I'll update this PR asap.

@arodic
Copy link
Contributor Author

arodic commented Nov 10, 2020

Conflicts resolved. Ready to merge.

*/

THREE.TessellateModifier = function ( maxEdgeLength ) {
THREE.TessellateModifier = function ( maxEdgeLength = 0.1, maxIterations = 6, maxFaces = 1000000 ) {
Copy link
Owner

Choose a reason for hiding this comment

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

How about maxFaces = Infinity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@mrdoob mrdoob merged commit f4b504f into mrdoob:dev Nov 10, 2020
@mrdoob
Copy link
Owner

mrdoob commented Nov 10, 2020

Thanks!

@arodic arodic deleted the recursive-tesselation branch April 19, 2022 06:54
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.

3 participants