@johnwmcc How is the Helix supposed to look like? I only tried it after merging in your changes with mine. And it only draws edges - I would have expected a solid.

It appear that the extension isn't using model.start_operation which is not good UX. This should be fixed.

Style Guide says
Classes:
Avoid the usage of class (@@) variables due to their unusual behaviour
in inheritance

The code uses these extensively, but not with inheritance,

I don't know how else to get variables persistent across uses of the plugin

Pros - if multiple instances are used, the overall model would be smaller
Cons - if you manually change the shape using 'Make component' after the plugin draws it as a group, you lose the ability to edit the shape. But I'm not sure if that would be true if it's code that generates the component.

If you need multiple instances of an already drawn shape, it's only a right click to convert the shape to a component.

Parameters would have to be coded to apply to the Component Definition, not the instance.

I've read your link clarifying the difference between symbols and strings.

As far as I can tell, we can't use symbols as hash keys, since the values (in defaults = ...) get reassigned with each new instance of a class, and if I've understood correctly, you can't modify a symbol once it's defined.

The Helical Ramp shape drawn with negative pitch to produce a left-handed helix has back faces upwards. It could be more useful to have it with front faces upwards.

Could be coded EITHER by drawing mesh polygons the other way round (which I can see how to do), or by selecting all faces at the end, then applying .reverse! method. Unfortunately, I can't see yet how to select the faces from within the container.

It's easy to open the helical ramp component for editing, select all faces, then apply contest menu 'Reverse faces', so this may not be worth doing.

I've now updated the SketchUcation Plugin Store version of this plugin to include three new shapes - Helix, Helical Ramp, and Helical Ramp with Sides. I've made a couple of other minor updates - to add missing validation for the number of sides or segments to draw in Tube, Torus and Pyramid shapes, and to add a centre point on key faces in the inserted shapes, to aid placement and Move/Copy operations.

I've also changed the code, following an extremely helpful suggestion and sample code from Jim Foltz, to change from inserting each shape as a Group at the world origin, to creating a component definition, and allowing the user to place it at any pickable point using the API function Sketchup.active_model.place_component.

I'm unclear whether, and if so how, you want to update the Community Extension version to correspond.

I can post updated versions of all the relevant files on GitHub, with the names and internally distinct Module names I adopted to keep the two versions separate - jwm_shapes.rb instead of shapes.rb, an overall module name JWM instead of Community Extensions, and a sub-module name of JWMShapes instead of Shapes, and the amended code to insert components.

Alternatively, and much simpler for me, I can email a copy of the jwm_shapes2.5.rbz file to you for you to decide how best to rename the contents to keep the two versions distinct, should you wish to update the SU Shapes version to add the additional functionality. It also includes updated documentation and changelog.

Please let me know how to proceed from here.

I've further updated shapes.rb to draw faces clockwise for a left handed helical ramp, to show front faces pointing up (not down as they were previously).

I can't seem to generate a new pull request for the latest file version in my GitHub repository fork - when I try, I just get redirected to the existing pull request.

What am I missing? Or will the older un-merged pull request now pull the latest file?

Latest changes are in line 964 of newest version (where I add a new variable 'left_hand = TRUE' if that is the case) and in lines 1016-1031 (where I draw the faces clockwise for left handed rotation).

Please advise me how to point you to the latest update.

1. Make the generated shape a component in code, rather than a group
   Pros - if multiple instances are used, the overall model would be smaller
   Cons - if you manually change the shape using 'Make component' after the plugin draws it as a group, you lose the ability to edit the shape. But I'm not sure if that would be true if it's code that generates the component.
   If you need multiple instances of an already drawn shape, it's only a right click to convert the shape to a component.
2. Allow selection of a pick point, not just the origin, as the starting point for the shape
   Pro - has been requested on Sketchucation forum
   Cons - possibly too difficult for me to program
3. Extend the range of shapes to other regular polyhedra
   Pro - makes it more versatile
   Con - longer list of shapes to choose from could slow down use for commoner shapes

I've been through the GitHub Ruby Style Guide and cleaned up all the code I know how to.

I've raised separate issues for the ones I don't know how to deal with, marked with ** in the list below:

Classes

Avoid the usage of class (@@) variables due to their unusual behaviour
in inheritance - used extensively, but not with inheritance
** I don't know how else to get variables persistent across uses
of the plugin

Use def self.method to define singleton methods - two instances.
I don't see any other opportunities to use this

Avoid class << self except when necessary - not used

Indent the public, protected, and private methods as much the
method definitions they apply to. Leave one blank line above them - not used

Avoid explicit use of self as the recipient of internal class or
instance messages unless to specify a method shadowed by a variable - not used

Collections

Prefer %w to the literal array syntax when you need an array of strings - no string arrays used

Use Set instead of Array when dealing with unique elements - no relevant usage

Use symbols instead of strings as hash keys
** I think these have to be strings, as they are reassigned new values for each new instance of a Class

Strings

Prefer string interpolation instead of string concatenation - neither used

Prefer double-quoted strings - all single quoted strings changed to double

Avoid using String+ when you need to construct large data chunks. Instead, use String<< - neither construct used

Regular Expressions

Not used

Percent literals

Not used

Hashes

Use hashrocket syntax for Hash literals - changed in defaults = {...} lines

2014-02-08 v1.4.5

Earlier issues revisited from issue #3 list and fixed

Use unless file_loaded(__FILE__) instead of if (not $shapes_menu_loaded) - changed
(note, needs to be file_loaded?(__FILE__) with question mark to work)

No parentheses in case(length_unit) or case(key) - now all removed

Is this f.erase! safe? Erasing from the collection it iterates might
be problematic. It's normally safer to collect entities to erase and bulk
erase them after iterating. It's also faster to erase in bulk than individual entities
** That may be so, but I don't know how to do that. And there are so few faces that speed doesn't matter here

Logic error and change in defaults for Dome and Sphere:

Fixed logic error in defining default number of segments for Dome and
Sphere, (which was accidentally still hard coded to 5 in the defaults = {...} line)

Changed default number from 5 to 4 per 90 degrees for consistency with base circle segments for Cone, Cylinder, Tube and Torus

This PDF in the bundle will not likely be ready by users. They are not likely to open the RBZ before installing.

It'll probably fit better as a .md MarkDown file in the repository - which we can reference for the description at Extension Warehouse when we update. It'll also be easier to edit in MarkDown and Git can diff it, while PDFs are treated as binary.

The installation instructions can probably go into the README somewhere (unless we already have that.) The rests is fitting for a DOCUMENTATION.md file.

Ah, didn't notice it was put the the support folder. I had intended it to be in the root of the project so it's easily seen by people visiting the GitHub project.

It should be CHANGELOG.md, with the capitalization for consistency and placed in the root of the Git project folder.

Submit latest changes with the new license comments to EW.

Pro - makes it more versatile
Con - longer list of shapes to choose from could slow down use for commoner shapes, and there may be little need for the shapes in practice

Maybe we could somehow canvas opinion for other useful shapes to add?

Allow selection of a pick point, not just the origin, as the starting point for the shape
Pro - has been requested on Sketchucation forum
Cons - possibly too difficult for me to program

At the moment, the parameter list can be presented in any order, since the parameters are stored as a hash, which is essentially unsorted. When there more than a handful of parameters, this makes Editing the shape more difficult.

Would a hash-array allow the original order to be preserved?

This would need to be updated in parametric.rb, I think, and would have knock-on consequences in shapes.rb.

https://github.com/styleguide/ruby

The Parametric class is used by multiple extensions.
su_windows
su_shapes

Making some comparisons, it looks like the class definition in SU_Shapes has been "getting the most love."

It has a bunch of "TODO" comments by Thomas thoughout.

Should it be moved to a CommunityLibrary module ?
... and be referenced by the other extensions in some way ?

Or should SU_Windows become dependent upon SU_shapes being installed and the Parametric class defined and maintained in the shapes extension ?

There are some coders that are wanting to mixin this parametric functionality into their own modules and classes.

Style Guide says
"Never use 'for' unless you know exactly why"

There are three for loops left in the code:
L117 ff:

  for i in 0...numseg do
    angle = i * vertex_angle
    cosa = Math.cos(angle)
    sina = Math.sin(angle)
    vec = Geom::Vector3d.linear_combination(cosa, xaxis, sina, yaxis)
    pts.push(center + vec)
  end

L689 ff and L771 ff:

  for i in 0..n90 do
    angle = delta * i
    cosa = Math.cos(angle)
    sina = Math.sin(angle)
    arcpts.push(Geom::Point3d.new(radius*cosa, 0, radius*sina))
  end

I don't know whether, let alone how to, replace these with another type of iteration.

Added this additional shape, with additional option for slope of sides.

Fixed minor bug - number of rotations was being reset to default of 1.0 on editing or redrawing helical shapes.

Updated Documentation.md and Changelog.md files to match.

Will wait until you have finished testing v2.0.0 before making a new Pull Request to add this shape.

What do you think about using Components instead of Groups, and then using Model.place_component to let the user place the components. Currently, the objects are created as Groups and draw at the Origin.

I've developed a new plugin, Polyhedra, to draw the five Platonic solids, size defined by either radius or side of polyhedron. Rather than just add the shapes to the 3D Parametric Shapes plugin, which might over-complicate it, I'd like to keep it separate.

I hope later to enhance it to all Regular Polyhedra, which would add stellated versions of all five solids.

I'd like to offer it as it stands as an additional plugin to the Extension Warehouse, subject to your review and comment.

Some of the code is a little clunky, but it works. Any suggestions for improving it would be welcome. I think I can already see how to draw the Icosahedron better, but will leave that for a few days.

It's saved on GitHub in my account, https://github.com/johnwmcc/polyhedra. It's public, so I think you should be able to access it.

What do you or I have to do to allow me to generate a pull request to you? Or can you review it 'in situ' and let me have any comments on the code?

While awaiting a response, I'll develop documentation for it.

Best wishes.

Why did the original plugin choose 'segments per 90 degrees' for Dome (and now Sphere)? I've just kept it, without thinking much about it, and adapted the same structure of code to make a Sphere. But I'm now wondering.

One reason might be to have a group that's quicker to generate - the same quarter dome mesh revolved four times for Dome, or eight times for Sphere, rather than separately generated - but I'm not sure this is true unless the initial quarter-dome mesh were made into a component, not a group.

Would it be more consistent with other shapes based on a circle, and more flexible, to change the code so you can have any number of segments per 360 degrees?

I'm not inclined to delay release for this reason, but might come back to it later.

L514:

inner[0].faces.each { |f| f.erase! if(f != face) }

You commented earlier:
"Is this (f.erase!) safe? Erasing from the collection it iterates might
be problematic. It's normally safer to collect entities to erase and bulk
erase them after iterating. It's also faster to erase in bulk than individual entities"

That may be so, but I don't know how to do that. I don't really even know what this line is doing - how can you erase a 'face' that isn't a face?

And because there are so few faces, I doubt if speed matters.

This version (2013) tries to load the tools under the Draw->shapes menu. However for 2014, a shapes menu has been added an it contains the rectangle, circle and polygon tools. As a result the extension features are not showing up in the 2014 UI.

We should think about how to relocate them. Perhaps Draw-> 3D shapes-> .... ?

Readme.md refers to "sketchup-stl" in several places, when it should refer to "sketchup-shapes".