More options and usability improvements for Surface

[jteuber]

Hi all,

for a 3D printing project I needed to add texture to some surfaces. When I started I noticed that Surface was weirdly unpredictable in its sizes, making it virtually unusable for me. Also it was too slow with the heightmaps I'm using. That's ok for compiling STLs, but not really for preview. So I started diving into the code and adjusting it to my needs. The results are in this PR. Even though this is slightly breaking existing behavior I wanted to see if you would agree that this is making more sense. Also, part of this can be incorporated without changing behavior of existing scripts.

Changed behavior:

• A fully white (1) heightmap resulted in the same surface as a fully black (0) heightmap. Now the white heightmap will result in a cube of thickness+100 in z and the black heightmap in a cube with thickness in z.
• Fixed a bug that led to weird behavior when inverted==true

Added features:

• Argument: bool doubleSided; default = false
  • Applies the heightmap to both sided of the surface node
• Argument: double thickness; default = 1.0
  • if doubleSided: Distance between the vertices of the top and the bottom surfaces, else the distance between a black/0 pixel and the bottom
• Argument: int pixelstep: default = 1
  • Step between used pixels of the given heightmap in both axes while preserving the size of the resulting surface. Makes it possible to preview a surface with significantly less faces but more or less the same geometry.

[t-paul]

I wondering if it would be a better strategy having a new module with a more obvious interface so at some point later surface() could be deprecated.

[jteuber]

I wondering if it would be a better strategy having a new module with a more obvious interface so at some point later surface() could be deprecated.

That sounds like a good option. I would gladly refactor this PR to make it a new module. Did you already have a name and signature in mind? If not, I would suggest the following:

heightmap(vector3 size, string file, bool center = false, bool invert = false, int convexity = 1, bool doubleSided = false, double thickness = 1.0, int pixelStep = 1)

This would also add a size parameter to make the size of the resulting object independent of the given file. The z value of that size I would interpret as the difference between the resulting z value of a "white" and a "black" pixel. Thickness would then be added to it on the bottom. This makes it very explicit what the resulting geometry looks like. Otherwise the signature is the same to the surface module from this PR. If you have any thoughts please let me know.

[kintel]

I agree that a new module makes more sense. heightmap sounds like a good name.

API design is hard though. Some notes:

• doubleSided: It feels like this can be made equivalent to union() { heigthmap(...); mirror([0,0,1]) heightmap(...); }. I'd generally prefer using OpenSCAD syntax rather than extra built-in parameters to achieve this, unless good arguments can be made otherwise.
• surface() was initially made to visualize scientific results, e.g. from GNU Octave. We should make sure we maintain that goal.
• pixelStep: Doesn't this do the same as the X, Y components of size ?
• Instead of discrete parameters like thickness and invert, could we supply a z range for doing the mapping?
• We could consider passing an OpenSCAD array as argument in addition to a filename, so that OpenSCAD code could calculate heightmaps directly.

[jordanbrown0]

I'd need to look more at surface to have a real opinion on replacing it, but I suggest that the first parameter be either a string filename or an array of values, with everything else defaulting, so that you could say heightmap("myfile.png") and have something sensible happen.

[jteuber]

I agree that a new module makes more sense. heightmap sounds like a good name.

API design is hard though. Some notes:

* `doubleSided`: It feels like this can be made equivalent to `union() { heigthmap(...); mirror([0,0,1]) heightmap(...); }`. I'd generally prefer using OpenSCAD syntax rather than extra built-in parameters to achieve this, unless good arguments can be made otherwise.

That's not how doubleSided works. It basically takes two copies of the heightmap, translates the second below the first one and then connects the two. Maybe this screenshot helps:

It is possible to get the same effect with difference and the new thickness parameter, but honestly I disagree with the general statement that we should rather use OpenSCAD syntax if possible. All the center arguments are for example not necessary, you can easily center pretty much everything with a translate. But it would make the scripts more complicated than necessary. I would argue the same here.

* surface() was initially made to visualize scientific results, e.g. from GNU Octave. We should make sure we maintain that goal.

Sure, that shouldn't be a problem.

* `pixelStep`: Doesn't this do the same as the X, Y components of `size` ?

No, that's the point. It skips pixels on each axis while maintaining the size. Effectively, with a pixelStep of 2 it would be like scaling the picture down to a quarter (half on each axis) of its original size and then scaling the resulting surface with scale([2,2,1]).

* Instead of discrete parameters like `thickness` and `invert`, could we supply a z range for doing the mapping?

No. Again, they work differently. The z of the size would work like a range and maybe we could do that a negative z would have the effect of invert. But thickness is about the minimal distance between the top and the bottom of the surface.

Also, I would like to make thickness a bit smarter at some point to actually guarantee that each vertex of the bottom surface is thickness units away from the closest point of the top surface. This would help in creating objects that have a distinct surface for 3D-printing while not wasting much material by closing the surface on the back with a plane.

* We could consider passing an OpenSCAD array as argument in addition to a filename, so that OpenSCAD code could calculate heightmaps directly.

Why in addition? This sounds more like as a substitute.

I agree that the proposed signature is complicated, but I don't see many options to reduce it without sacrificing on the usability of it.