Adding PolarGridHelper and associated files #10179

Merged
merged 8 commits into from Nov 23, 2016

Projects

None yet

5 participants

@Hectate
Contributor
Hectate commented Nov 20, 2016

Adding a PolarGridHelper and associated documentation an an update to example 'webgl_helpers'. From issue #10146 with major credit to @Mugen87 for nearly all of the 'real' code, as well as @mrdoob . Thanks to @TristanVALCKE for sparking the discussion.

src/extras/helpers/PolarGridHelper.js
+import { Float32Attribute } from '../../core/BufferAttribute';
+import { BufferGeometry } from '../../core/BufferGeometry';
+import { Color } from '../../math/Color';
+import { _Math as Math } from '../../math/Math';
@Mugen87
Mugen87 Nov 20, 2016 Contributor

I don't think we need the _Math reference here...

@Hectate
Hectate Nov 20, 2016 Contributor

We use Math.sin and Math.cos, do you mean we don't need to do _Math as Math and just import Math?

@Hectate
Hectate Nov 20, 2016 Contributor

I just looked at some other source and see that it's not there but Math... works anyway. Not sure where I picked that up from but I'll remove it. ๐Ÿ‘

@Mugen87
Mugen87 Nov 20, 2016 Contributor

_Math was the reference to three.js internal math lib. In PolarGridHelper we just use the plain Math object of JavaScript (see http://www.w3schools.com/js/js_math.asp)

@Hectate
Hectate Nov 20, 2016 Contributor

Oh duh. Yeah that makes sense :)

src/extras/helpers/PolarGridHelper.js
+ radials = radials || 16;
+ circles = circles || 8;
+ divisions = divisions || 50;
+ color1 = new THREE.Color( color1 !== undefined ? color1 : 0x444444 );
@Mugen87
Mugen87 Nov 20, 2016 Contributor

@Hectate Can you please remove all references to the THREE namespace. The dependencies should be resolved by the import statements at the top...

@Hectate
Hectate Nov 20, 2016 Contributor

Yep, completely slipped my mind. Will fix.

src/extras/helpers/PolarGridHelper.js
+ }
+
+ var geometry = new THREE.BufferGeometry();
+ geometry.addAttribute( 'position', new THREE.Float32Attribute( vertices, 3 ) );
@Mugen87
Mugen87 Nov 20, 2016 Contributor

We should also change Float32Attribute to the newer Float32BufferAttribute.

@Hectate
Hectate Nov 20, 2016 Contributor

Can do.

@Hectate
Contributor
Hectate commented Nov 20, 2016 edited

I believe this is ready now.

Edit: OK, now maybe :)

+PolarGridHelper.prototype = Object.create( LineSegments.prototype );
+PolarGridHelper.prototype.constructor = PolarGridHelper;
+
+export { PolarGridHelper };
@WestLangley
WestLangley Nov 20, 2016 Collaborator

newline missing at EOF... that's all I can find. :)

@Hectate
Hectate Nov 20, 2016 Contributor

๐Ÿ‘

@Hectate
Hectate Nov 20, 2016 Contributor

Should be good now, I believe.

src/extras/helpers/PolarGridHelper.js
+ radius = radius || 10;
+ radials = radials || 16;
+ circles = circles || 8;
+ divisions = divisions || 50;
@WestLangley
WestLangley Nov 20, 2016 Collaborator

I think 64 default divisions is visually more appealing.

@Hectate
Hectate Nov 20, 2016 Contributor

Yeah, plus having the default divisions be a multiple of the default radials value would be ideal for vertex placement.

+
+ <h3>[name]( [page:Number radius], [page:Number radials], [page:Number circles], [page:Number divisions], [page:Color color1], [page:Color color2] )</h3>
+ <div>
+ radius -- The radius of the polar grid. This can be any positive number. <br />
@WestLangley
WestLangley Nov 20, 2016 Collaborator

Default is ...

@Hectate
Hectate Nov 20, 2016 Contributor

I cribbed from GridHelper.html, but sure there's no reason we can't be more informative since we're touching it :)

@WestLangley
WestLangley Nov 20, 2016 Collaborator

You're doing a great job. Why not fix them both. :-)

@Hectate
Hectate Nov 20, 2016 Contributor

Even better haha

@TristanVALCKE
Contributor

Did you lint the source accordingly to new eslint implementation ?

@Hectate
Contributor
Hectate commented Nov 20, 2016

No, I didn't do any linting.

@Mugen87
Contributor
Mugen87 commented Nov 21, 2016 edited

@mrdoob I think we can still merge this PR...

@mrdoob mrdoob merged commit 21c6db4 into mrdoob:dev Nov 23, 2016
@mrdoob
Owner
mrdoob commented Nov 23, 2016

Thanks!

@Hectate Hectate deleted the Hectate:polarGrid branch Nov 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment