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

Added TriangleGeometry Class #29882

Closed
wants to merge 11 commits into from
Closed

Added TriangleGeometry Class #29882

wants to merge 11 commits into from

Conversation

pxninja
Copy link

@pxninja pxninja commented Nov 14, 2024

Description

I noticed Three JS primitives do not include a triangular plane. This PR adds that primitive with the following parameters: width, height, skew, and detail.

While the default parameters create an equilateral triangle, adjusting the width / height / skew will produce any type (isosceles, scalene, right, obtuse). Increasing the detail uniformly subdivides the plane, where "detail" is the number of edge divisions, and the number of facets per division is ( detail + 1 ) ^ 2.

Example of uniform subdivision (aka. what to expect):

image

Copy link

github-actions bot commented Nov 14, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 339.37
79.06
339.38
79.06
+17 B
+2 B
WebGPU 478.22
132.63
478.23
132.64
+17 B
+3 B
WebGPU Nodes 477.68
132.51
477.7
132.52
+17 B
+3 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Before After Diff
WebGL 464.82
112.02
464.82
112.02
+0 B
+0 B
WebGPU 546.95
148.2
546.95
148.2
+0 B
+0 B
WebGPU Nodes 502.83
137.91
502.83
137.91
+0 B
+0 B

@pxninja
Copy link
Author

pxninja commented Nov 15, 2024

After giving it some thought, I can see how normalizing the height could be confusing. Height is now a literal unit and the UV maps to the base and apex of the geometry.

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 15, 2024

Do you mind moving TriangleGeometry into https://github.com/mrdoob/three.js/tree/dev/examples/jsm/geometries?

This kind of geometry generator has not been requested so far so I do not see it as a core module.

@pxninja
Copy link
Author

pxninja commented Nov 15, 2024

Moved, as requested. For what it's worth, because a triangle is the most fundamental geometry, I (personally) would expect a triangle to be a primitive. Coupled with it being the only geometry that easily creates the Three JS logo, it seems like src/geometry/ is a better fit, but not a big deal either way.

@linbingquan
Copy link
Contributor

related: #29690 (comment)

@linbingquan
Copy link
Contributor

linbingquan commented Nov 16, 2024

UVsDebug:
image

@linbingquan
Copy link
Contributor

linbingquan commented Nov 16, 2024

How about do this.

  1. Make detail default value is 1
  2. Rename detail to segments
--- a/examples/jsm/geometries/TriangleGeometry.js
+++ b/examples/jsm/geometries/TriangleGeometry.js
@@ -3,17 +3,19 @@ import { Float32BufferAttribute } from '../core/BufferAttribute.js';

 class TriangleGeometry extends BufferGeometry {

-       constructor( width = 1, height = Math.sqrt( 3 ) / 2, skew = 0, detail = 0 ) {
+       constructor( width = 1, height = Math.sqrt( 3 ) / 2, skew = 0, segments = 1 ) {

                super();

                this.type = 'TriangleGeometry';

+               segments = Math.max( Math.floor( segments ), 1 );
+
                this.parameters = {
                        width: width,
                        height: height,
                        skew: skew,
-                       detail: detail
+                       segments: segments
                };

                // buffers
@@ -25,7 +27,6 @@ class TriangleGeometry extends BufferGeometry {

                // vertex helper variables

-               const segments = Math.max( Math.floor( detail + 1 ), 1 );
                const offsetX = width / 2;
                const offsetY = height / 3;

@pxninja
Copy link
Author

pxninja commented Nov 16, 2024

  • details is now segments
  • The UV looks correct to me ... is there something I'm missing? Would this be better if mapped as an equilateral (ie. the base V coordinate is greater than 0 and the apex V coordinate is less than 1), or proportional to the width / height / skew?

@pxninja
Copy link
Author

pxninja commented Nov 17, 2024

I'm considering updating the class to minimize UV distortion. Is this closer to what you're expecting @linbingquan ?

		// vertex helper variables

+		const widthHalf = width / 2;
+		const skewHalf = skew / 2;

-		const offsetX = width / 2;
+		const offsetX = ( width + widthHalf + skewHalf ) / 3;
		const offsetY = height / 3;

+		const length = Math.max( Math.abs( skewHalf ) + widthHalf, width );

+		const maxU = Math.min( length / height, 1);
+		const maxV = Math.min( height / length, 1);

+		const offsetU = skew + width > 0 ? (1 - maxU) / 2 : (length - width) / length;
+		const offsetV = (1 - maxV) / 2;

		// vertices, normals, and uvs

		for ( let i = 0; i < segments + 1; i ++ ) {

			for ( let j = 0; j < i + 1; j ++ ) {

-				const uvX = ( i + j ) / segments / 2;
-				const uvY = ( i - j ) / segments;
+				const normX = ( i + j ) / segments / 2;
+				const normY = ( i - j ) / segments;

-				const x = uvX * width - offsetX + uvY * skew / 2;
-				const y = uvY * height - offsetY;
+				const x = normX * width + normY * skewHalf;
+				const y = normY * height;

+				const u = maxU * x / length;
+				const v = maxV * normY;

-				vertices.push( x, y, 0 );
+				vertices.push( x - offsetX, y - offsetY, 0 );

				normals.push( 0, 0, 1 );

-				uvs.push( uvX );
-				uvs.push( uvY );
+				uvs.push( u  + offsetU, v  + offsetV );

			}

		}

@linbingquan
Copy link
Contributor

I'm considering updating the class to minimize UV distortion. Is this closer to what you're expecting @linbingquan ?

In both test cases, there was no problem with the results.

new TriangleGeometry(1, Math.sqrt( 3 ) / 2, 0, 4) new TriangleGeometry(1, 1, 0, 4)
image image

@pxninja
Copy link
Author

pxninja commented Nov 25, 2024

@Mugen87 - Anything else needed on my end? Would love to get this merged.

@mrdoob
Copy link
Owner

mrdoob commented Nov 26, 2024

Curious to hear about use cases for this geometry...?
I just can't think of one (except generating the library logo 😅).

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 26, 2024

We've been a bit more strict when adding new modules to the repository compared to earlier days. With that policy in mind, I tend to place TriangleGeometry in a third-party repository or just share it as a Gist.

IMO, the existing geometry generators in the repository are more than sufficient.

@pxninja
Copy link
Author

pxninja commented Nov 26, 2024

Completely understand (and appreciate) you both safeguarding the quality of Three JS. If this were my project, I would do the same.

@mrdoob - I am currently using this in a project, which is how I noticed its absence. Specifically, I'm generating the base shape and programmatically manipulating the vertices, which allows me to instantiate game assets without having to model each. That aside, any planar example where 1 less vertex allows 25% more stuff (think grass, particle shards, billboards, etc.). This kind of efficiency is notable in procedural applications. Generating the library logo was more of a discovery than the intention.

@Mugen87 - I agree the existing generators capture many applications.

Three JS is awesome. I love it. Triangles are a primitive. The library doesn't have them. And I respect everyone's time enough to not waste it on a feature request I could do, allowing core contributors to focus on more important things. If triangles are something the library would like to consciously omit, please let me know. I'm trying to be helpful (not a nuisance).

@WestLangley
Copy link
Collaborator

Triangles are a primitive. The library doesn't have them.

To be fair, currently any regular polygon can be generated, and then optionally transformed.

const geometry = new THREE.CircleGeometry( 1, 3 ); // triangle

geometry.rotateZ( Math.PI / 2 );
geometry.translate( 0, 0.5, 0 );
geometry.scale( 10, 10, 1 );

@pxninja
Copy link
Author

pxninja commented Nov 27, 2024

To be fair, currently any regular polygon can be generated, and then optionally transformed.

Charitably, I take your point. Also, I would have hoped the context clarified I was talking about a generator akin to the one in this PR.

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 27, 2024

@pxninja We do appreciate the contribution but it seems the consensus is to maintain such a generator not in this repository.

I recommend you put TriangleGeometry in a separate repo and share the module as a npm package so developers can easily use it in their projects. If that is too much maintenance effort, you can still share the module as a simple Gist. In the three.js forum, there is a Resource category where developers present or share code that might be useful for others. That would be a good place to promote TriangleGeometry.

https://discourse.threejs.org/c/resources/8

@Mugen87 Mugen87 closed this Nov 27, 2024
@Mugen87 Mugen87 added this to the r171 milestone Nov 27, 2024
@pxninja
Copy link
Author

pxninja commented Nov 27, 2024

Should anything change, the code is available.

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.

5 participants