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

PolarGridHelper: Allow zero radials or zero circles #24509

Merged
merged 3 commits into from
Aug 23, 2022

Conversation

JeremieBourque1
Copy link
Contributor

Prevents from getting NaN values when 0 radials or 0 circles are used.

Related issue: none

Description

Previously, if a PolarGridHelper is created with 0 radials or 0 circles, we get the following error:

THREE.BufferGeometry.computeBoundingSphere(): Computed radius is NaN. The "position" attribute is likely to have NaN values. 

The NaN values come from the for-loops doing an iteration for 0 wich causes a 0/0 division resulting in a NaN.

By changing the looping condition to a "less than", no iterations are done so no NaN is created, allowing for a polar grid with no radials or no circles without getting an error.

Prevents from getting NaN values when 0 radials or 0 circles are used.
@@ -16,7 +16,7 @@ class PolarGridHelper extends LineSegments {

// create the radials

for ( let i = 0; i <= radials; i ++ ) {
for ( let i = 0; i < radials; i ++ ) {

const v = ( i / radials ) * ( Math.PI * 2 );
Copy link
Contributor

Choose a reason for hiding this comment

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

Now the maximum value of v would be less than 2*Pi, which will result in radials not making full circle. Is this what we want?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need v to go completely around the circle because that will create two lines at 0 degrees instead of only one.

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 19, 2022

I've added the change to my local branch and experimented with it.

With this change, adding 0 for both radials and circles results in an empty geometry. Only setting one of the parameters to 0 results in a malformed polar grid (only radial lines or circles).

Right now, it seems not allowing 0 is the better approach.

@JeremieBourque1
Copy link
Contributor Author

@Mugen87 In my use case, I want to be able to set 0 radials and have only circles because otherwise it ends up cluttering the ground plane too much. As for 0 circles, I don't have that use case but I could see someone potentially only needing the radials.

The way I see it, it would give the user a bit more freedom without the need of writing their own helper just for a slight modification.

@WestLangley
Copy link
Collaborator

The production code has the following shortcomings:

  1. a circle of radius 0 is rendered unnecessarily,
  2. the first and last radial are coincident -- and sometimes rendered in different colors, and
  3. a single radial is rendered if radials is 1.

This PR currently fixes (1) and (2).

Instead, I would rename radials to sectors, and (optionally) circles to rings. sectors and rings must be at least 1.

class PolarGridHelper extends LineSegments {

	constructor( radius = 10, sectors = 16, rings = 8, divisions = 64, color1 = 0x444444, color2 = 0x888888 ) {

		color1 = new Color( color1 );
		color2 = new Color( color2 );

		const vertices = [];
		const colors = [];

		// create the sectors

		if ( sectors > 1 ) {

			for ( let i = 0; i < sectors; i ++ ) {

				const v = ( i / sectors ) * ( Math.PI * 2 );

				const x = Math.sin( v ) * radius;
				const z = Math.cos( v ) * radius;

				vertices.push( 0, 0, 0 );
				vertices.push( x, 0, z );

				const color = ( i & 1 ) ? color1 : color2;

				colors.push( color.r, color.g, color.b );
				colors.push( color.r, color.g, color.b );

			}

		}

		// create the rings

		for ( let i = 0; i < rings; i ++ ) {

			const color = ( i & 1 ) ? color1 : color2;

			const r = radius - ( radius / rings * i );

			for ( let j = 0; j < divisions; j ++ ) {

				// first vertex

				let v = ( j / divisions ) * ( Math.PI * 2 );

				let x = Math.sin( v ) * r;
				let z = Math.cos( v ) * r;

				vertices.push( x, 0, z );
				colors.push( color.r, color.g, color.b );

				// second vertex

				v = ( ( j + 1 ) / divisions ) * ( Math.PI * 2 );

				x = Math.sin( v ) * r;
				z = Math.cos( v ) * r;

				vertices.push( x, 0, z );
				colors.push( color.r, color.g, color.b );

			}

		}

		const geometry = new BufferGeometry();
		geometry.setAttribute( 'position', new Float32BufferAttribute( vertices, 3 ) );
		geometry.setAttribute( 'color', new Float32BufferAttribute( colors, 3 ) );

		const material = new LineBasicMaterial( { vertexColors: true, toneMapped: false } );

		super( geometry, material );

		this.type = 'PolarGridHelper';

	}

}

@JeremieBourque1
Copy link
Contributor Author

@WestLangley I have done the modifications you suggested.

@mrdoob mrdoob added this to the r144 milestone Aug 23, 2022
@mrdoob mrdoob merged commit ddd4ba2 into mrdoob:dev Aug 23, 2022
@mrdoob
Copy link
Owner

mrdoob commented Aug 23, 2022

Thanks!

abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
* Allow zero radials or zero circles

Prevents from getting NaN values when 0 radials or 0 circles are used.

* Changed radials and circles to sectors and rings

* Fixed lint errors

Co-authored-by: Jeremie Bourque <[email protected]>
snagy pushed a commit to snagy/three.js-1 that referenced this pull request Sep 21, 2022
* Allow zero radials or zero circles

Prevents from getting NaN values when 0 radials or 0 circles are used.

* Changed radials and circles to sectors and rings

* Fixed lint errors

Co-authored-by: Jeremie Bourque <[email protected]>
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