Doc fixes, code changes, bugfixes, new examples

[amatulic]

Captured suggestions from chat. Still to do is add a new example of a custom function and renumber the example references in the text.

@adrianVmariano , @RAMilewski :
Please add any new suggestions to this PR.

[adrianVmariano]

1. Negative metaballs: delete bit about "are below the isovalue" (we haven't mentioned isovalue). Just say "negative metaballs are never directly visible
2. Add something about how influence behaves in a reversed (?) or unexpected way at the center of negative metaballs
3. Text about isovalue parameter sort of makes it seem like it's a ball parameter instead of a global parameter when the paragraph starts. It is feels like an injection between a paragraph about model design and then the following paragraph on the same topic. So I think move the isovalue paragraph down to be the last paragraph before the built-in functions list, maybe? Or some more clear transition, like "Another global metaball parameter is..."
4. I wonder if the user function section should say: "where $d$ is a distance measured from the center or core of the metaball." That better captures what happens for capsule, disk and cyl. Then the later sentence seems somewhat unclear as written, starting with "within this framework". Also isovalue hasn't been introduced as a number yet. So maybe "If we choose an isovalue, c, then within this framework, the set of points v such that f(v)>c is a bounded set---for the example above, a sphere whose radius depends on c. The default isovalue is c=1. Increasing the isovalue shrinks..."
5. The text "all of the built-in functions accept these named arguments, which are not repeated in the list below" was reportedly confusing as "list below" was ambiguous. I'm not loving rewordings I've thought of. Maybe that paragraph should just go below the list and lead with "In addition to the dimensional arguments listed above, all built-in metaballs accept these named arguments". That actually feels like it might be a more natural presentation order anyway.
6. "decreases from the metaball center..." I think should maybe say "drops below the isovalue (default 1) somewhere in your bounding box". I'm not sure that "decreasing from the center" is actually a requirement? But if it doesn't go below the isovalue in your bounding box, you're going to just get a cube. (And the current statement would allow that it goes below the isovalue at infinity but not actually in the bounding box.) Then continue with "If you want your balls to behave similarly to the standard balls it should fall off as 1/d with distance. See example XX below for how to make a ball that works with cutoff and influence like the built-in balls" Then maybe something like "When multiple metaballs are in the model, their functions are summed and compared to the isovalue to determine the final shape of the metaball object."
7. Start the first custom metaball example (that doesn't exist yet---the lobe one) with "Custom metaballs are an advanced technique where you define your own metaball shape by passing a function literal that takes a single argument, a coordinate in space. It is called dv here, but can be given any name. Note that in this example we compute a modified distance and then return the inverse of that distance. The scale factor of 3 hard-codes the size of the ball.
8. Is it worth after the multilobe example I already wrote, having a repeat of that example but with a helper function and pass-through variables for lobe count and radius, but without cutoff or influence? That could serve as a friendly stepping stone and the example could feature two interacting multilobes with different parameter radii and lobe counts.
9. Text for the next example, either the noisy sphere or multilobe: Next we show how to create a function that works like the built-ins where it takes parameters that change the metaball. You must define a master function that accepts the position argument (here called dv) and then whatever other parameters your metaball uses (here ....). The function literal expression sets all of your parameters. Only dv is not set, and it becomes the single parameter to the function literal.
10. Should the example actually parallel the internal functions more and use a function to return the function literal?
11. Would changing dv which is a little mysterious to p for "point" in the examples be helpful?
12. Jordan was confused by "voxels found to contain the surface" since contain can mean several things here. Rewrite as "the number of voxels the surfaces passes through". This language occurs several times. Also in the voxel size section it says "bounds of the voxels containing the generated surface" and it should just say "bounds of the generated surface"
13. Jordan is surprised that isosurface takes 3 args (x,y,z) instead of a vector point. He notes that OpenSCAD syntax for a point is a vector, e.g. translate([x,y,z]) not translate(x,y,z). And it seems that metaball syntax of taking a vector furthers this confusion. He also showed a timing test that it's actually 13% faster to do it the vector way (and then do p.x, p.y, p.z in the function). However, that timing difference is very small as it was 10^8 iterations and saved only 10 seconds. But that inconsistency could be confusing. Assuming it stays as it is perhaps it's worth adding a remark about the difference. (e.g. "Note that unlike isosurface() the metaball functions expect a vector parameter." and a similar reversed note in the isosurface section.) We don't have to switch this, but it's something to think about and make sure we think we've made the right choice, and then clearly document.
14. Add some bounded examples to isosurface, (e.g. superellipsoid?)

[adrianVmariano]

function super(x,y,z,s1,s2) =
   (abs(x)^(2/s2)+abs(y)^(2/s2))^(s2/s1) + abs(z)^(2/s1);
isosurface(function (x,y,z) super(x,y,z,.95,.5), 1, voxel_size=.05,closed=true, bounding_box=[[-1.1,-1.1,-1.1],[1.1,1.1,1.1]]);

produces extra faces:

[amatulic]

produces extra faces

And it probably should. Everything in your picture would be purple if you view with "Thrown together". That would mean the values outside the shape are bigger than the values inside. If you set an isovalue range [0.9,1,1] with reverse=true it works.

[adrianVmariano]

I don't understand. Everything purple would be an understandable solution, but why does that mean I get extra faces?

[amatulic]

Finished all items 1-14.

8. Is it worth after the multilobe example I already wrote, having a repeat of that example but with a helper function and pass-through variables for lobe count and radius, but without cutoff or influence?

I think that would be confusing. It still confuses me sometimes when I look at the code. I think it's enough just to say that spec should specify the custom function as function (dv) my_func(dv, ....)

I have already included that example a second time, showing the additional parameters.

10. Should the example actually parallel the internal functions more and use a function to return the function literal?

As above, I think that would be confusing, but it's done.

[amatulic]

Everything purple would be an understandable solution, but why does that mean I get extra faces?

I am not sure, and it's curious why it happens only at that voxel size. I note that you have isovalue=1, and your expression evaluates to 1 in places outside the shape but within the bounding box (like at [-1,0,0]. The voxel bounding box got expanded and centered around your original, so it's [[-1.05, -1.05, -1.05], [1.05, 1.05, 1.05]].

[adrianVmariano]

Regarding my proposed suggesting of a second copy of metalobe with a passthrough function, why would that be confusing? I think it would be LESS confusing than jumping to the noisy sphere example, which uses a passthrough function with two strikes against it: you didn't just see a simpler version of the same example and it has the additional complication of cutoff and influence in the mix. I guess I'm not sure what you did because you do say you "already included the example a second time with additional parameters".

It seems to me that saying "we aren't going to show this because it's confusing" is the wrong thing if by confusing you meant "difficult" or "advanced". If it's just a convoluted example that doesn't have a point to make, then sure it should be cut. But if it's a tricky technique that might be useful to users and it's hard to understand then it's more important to include examples of how to use that technique. If you go all the way to showing how to do influence and cutoff but not how to actually return it from a function you are in some sense assuming that it's obvious to the user how to return it from a function, so that they can add seamless new metaballs if they want.

The alternative position would be, "users don't need to do this" in which case maybe the noisy sphere example is not necessary at all. (But I think showing to to apply cutoff and influence is probably a good idea.)

[adrianVmariano]

Everything purple would be an understandable solution, but why does that mean I get extra faces?

I am not sure, and it's curious why it happens only at that voxel size. I note that you have isovalue=1, and your expression evaluates to 1 in places outside the shape but within the bounding box (like at [-1,0,0]. The voxel bounding box got expanded and centered around your original, so it's [[-1.05, -1.05, -1.05], [1.05, 1.05, 1.05]].

How is [-1,0,0] "outside the shape". The shape should have radius 1 and all six axis points are part of the shape.

[amatulic]

I didn't say your lobe example was confusing. I already added it, first the basic one, and then the one with more parameters.

What would be confusing is an example that has the extra support functions allowing the user to avoid putting function (dv) myfunc(dv, a, b) in the spec, just so he can put myfunc(a,b) in the spec instead, like is done for the mb* functions.

[adrianVmariano]

Isn't it a natural question, especially where noisy sphere is almost duplicating a built-in metaball, how to fully get the same effect?

[amatulic]

Sure it's a natural question, but not one that needs to be answered in the examples. "If you want to do it this way, here are the hoops you have to jump through" doesn't strike me as meaningful or helpful. Built-in is built-in, user-defined is user-defined, there is no problem with them being different.

Nevertheless, I have made this change showing the full implementation for the noisy metaball example. I don't think it's enlightening at all, but there it is.

Further looking into the but you reported above with three faces on the figure:
What's wrong isn't that there are three faces, but that there aren't six faces. The entire outer surface of the bounding box is all above the isovalue, therefore each face would be a clipping face and all six faces of the box should be shown. It should appear as a solid box with a hollow shape inside.

Any other voxel size works correctly, but at voxel size 0.05, the detection of voxels coinciding with the bounding box on three faces was off by some tiny number. Clearly a precision error. I have fixed this to check whether the outer face of the voxel is within EPSILON of the bounding box, and now it works correctly.

All 14 items in the list above are completed. Pushing another revision now. Non-cubical voxels isn't done yet. Stay tuned.

[adrianVmariano]

I would say it's counter-intuitive that requesting "closed" adds those faces. The figure was already closed and no part of it hits the bounding box. Basically it's adding the second surface at infinity and clipping it to the cube. We did say that isovalue=c was the same as isovalue=[c,INF] but I didn't understand this meant that it would draw a shell using the surface at infinity. So I think we need to add a sentence in the docs about this---maybe where the isovalue is discussed?---and also I think we need to have an example where we show it (or something like it) showing up as a cube and then you have a following example where the isovalue range is changed to [-INF,c] and then it works. The example might work better if the bounding box goes only to 0 along one axis, so you don't just get a cube. I think that makes it more clear what's going on.

[adrianVmariano]

I think part of the reason for being confused has to do with thinking that I'm computing "an isosurface" instead of an object bounded by an isosurface. So I wonder if maybe changing the first line docs to

Computes a VNF structure of an object bounded by one or two isosurfaces, intersected with a specified bounding box.

Then here's a stab at rewriting paragraph 2:

The specified isovalue can be a range or a scalar. When it is a range, [c1,c2], the returned object is the set of points, p, satisfying c1<=f(p)<=c2. If f has values larger than c2 and values smaller than c1 then the basic result is a VNF with two bounding surfaces corresponding to the isosurfaces at c1 and c2; this is a shell object with an inside and outside surface. If f(p)<c2 everywhere, then no isosurface exists for c2, so the object has only one bounding surface---the one defined by c1. This can result in a bounded object like a sphere or it can result an an unbounded object such as all the points outside of a sphere out to infinity. A similar situation arises if f(p)>c1 everywhere. You are allowed to set c1=-INF or c2=INF, which always produces an object with only a single bounding isosurface. If you want to obtain a bounded object, think about whether the function values inside your object are smaller or larger than your isosurface value. If the values inside are smaller, you produce a bounded object using [-INF,c]. If the values inside are larger than [c,INF] will produce a bounded object. When you give a scalar isovalue, this is equivalent to [c,INF], so it will produce a bounded object when the points inside the object are larger than the points at the isovalue.

The description above suggests that the computed shape is a single object, but depending on your function, the object may have many (even infinitely many) disconnected components. It is also possible to have an unbounded object combined with several bounded shell objects. It is impossible to calculate an unbounded object, so isosurface calculations are restricted to a bounding box region, but when your object is unbounded it may display as simply the specified bounding box if closed=true.

That hopefully transitions to the paragraph on the bounding box.

Then at the end of that paragraph add:

If your object is unbounded then when it is intersected with the bounding box, the result may appear like a solid cube, because the clipping faces are all you can see and the bounding surface is hidden inside. Setting closed=false will remove the bounding box faces and expose the inside structure. If you want the bounded object, you can correct this problem by changing the isovalue range: if you had specified isovalue=c then use isovalue=[-INF,c]. If you had used [c1,c2] then one of [-INF,c1] or [c2,INF] will do the job. (Or you may have to inrvoke isosurface twice to get both regions.)

Then we should have examples of multi-component isosurface, and the previously noted unbounded case showing up as a cube, or maybe half-cube.

I actually was wondering if the isosurface behavior is backwards and if the default for a scalar should be [-INF,c] instead of how you did it. I feel like that's more typically how somebody would construct a (bounded) set. (Yes, I know this is backwards relative to metaballs.)

It seems like reverse is only useful with closed=false when you have chosen the wrong isovalue range. Is there any other possibility?

[amatulic]

That's a lot of words, but they look good to me. My only concern is that the previous words made a distinction between interior and exterior, which is important for a VNF because each facet has an interior and exterior side. I thought it was important to say that by default, "exterior" is wherever f < c. That distinction got lost in the new text.

The reverse argument is used internally to make two surfaces; the inside of the shell is always generated with reverse triangles. A user shouldn't need it, and it can be hidden if you prefer, but it can't be removed.

[adrianVmariano]

The gyroid example says that the vnf can be tiled or wrapped around an axis with vnf_bend. Richard pointed out that this seemed out of place, and I agree. Delete the last sense of "things I can do with a vnf". If there's something you think is particularly notable with the gyroid, add another example, e.g. with vnf_bend applied to the gyroid.

[amatulic]

I removed the bit about vnf_bend, and made other changes to the docs as suggested, with some copy-editing.

I propose:

• closed=true by default for metaballs(). This is a specific case of isosurface where setting closed=true is appropriate.
• closed=undef by default for isosurface(), which is more general-purpose. Behavior is as follows:
  • if undef, then assume closed=true when c1 is finite in isovalue=[c1,c2].
  • if undef, then assume closed=false when c1 is -INF.
  • if set by the user, then use that setting.

These rules would have the desired behavior in all cases, as far as I can tell. The examples may not even need to be modified, and the user is unlikely to need to set closed.

The gyroid/periodic surface examples with an isovalue range would still get the bounding box faces as they should, a sphere with bigger values inside than outside (scalar isovalue) would also get clipped correctly, and a sphere that has smaller values inside and bigger values outside would have -INF as the minimum isovalue and not have the bounding box faces.

[adrianVmariano]

Note that I tossed out the idea about changing the closed default for discussion, not implementation. First of all, metaballs should be closed=true by default. This was never about changing metaballs.

The idea was that closed=true can be confusing, so therefore maybe closed=false should be the default. The problem with the above proposal is that closed=true is potentially confusing when the shape is unbounded, and that can happen with any isovalue range. It's not a special property of [-INF,c]. So having a complicated default just creates more potential confusion while only solving the problem in one situation. Actually did you change the default range? Because my example where I encountered the problem was [c,INF].

Also note that the default for a parameter is not "what it says on the function line" it's how that parameter behaves when you run the function. You can implement a default by writing foo=default on the definition. You can also implement a default by writing foo=default(foo,value) in a let statement, or by other more complicated checks and assignments. So I would more succinctly describe your proposal as "Default: closed is false when c1 is -INF and true otherwise".

Here the shape is unbounded and we get a cube in each case covering all the possible isovalue range types:

isosurface(function(p) exp(-1/norm(p)), isovalue=[.5,1.5], bounding_box=[[-3,-3,-3], [3,3,3]], voxel_size=.5, closed=true);
isosurface(function(p) exp(1/norm(p)), isovalue=[-INF,3], bounding_box=[[-3,-3,-3], [3,3,3]], voxel_size=.5, closed=true);
isosurface(function(p) exp(norm(p)), isovalue=[3,INF], bounding_box=[[-3,-3,-3], [3,3,3]], voxel_size=.5, closed=true);

Not shown is [-INF,INF] which also makes a cube. Might be worth banning the case of isovalue=[-INF,INF].

After pondering the idea of defaulting closed to false I think I don't really like it. The justification is that it makes things less confusing. The cost is that we have a default which is basically wrong most of the time. I think that places the priority incorrectly. So I lean towards keeping the default closed=true and trying to strongly highlight the issue in the docs. Maybe we have a bold face sentence that reads "Why did I just get a cube?" followed by instructions that either the bounding box is too small or the range is flipped. That, combined with the example that shows the cube problem and solution seems like it should be good. And I think I lean towards the solution being correct the range, not using reverse.

This is because I think that it's better to encourage people to know what they are doing---that is, people should think for a moment about the interval that they need. Making closed=true by default instead supports a trial-and-error approach to getting things right. And if you end up needing the object to be clipped...or if your bounding box gets too small...then having switched to closed=false doesn't work and you need to flip the range. This makes me wonder if the scalar default should be removed and only a range permitted. That way you're forced to think explicitly when picking the isovalue whether you want to go up to INF or down to -INF. That could decrease confusion around the behavior of scalar isovalue, which sort of promotes "do what I mean" thinking.

There is also a failure mode when closed=false, which is that your bounding box is too small and you get nothing:

isosurface(function(p) exp(norm(p)), isovalue=[-INF,3], bounding_box=.1*[[-3,-3,-3], [3,3,3]], voxel_size=.5, closed=false);

I wonder if we need to mention this behavior and how it differs when closed=true vs closed=false.

[adrianVmariano]

1. Consider banning [-INF,INF] for isovalue
2. Consider removing the scalar default for isovalue
3. isovalue assertion can get an error in the assertion test because you never checked that input was numeric. It's a bit tricky because INF is allowed. assert(is_finite(isovalue) || (is_list(isovalue) && len(isovalue)==2 && is_num(isovalue[0]) && is_num(isovalue[1])) may be adequate. (Doesn't rule out NaN as a possibility.)
4. Would a shorthand bounding box like a simple scalar for a centered box of that size (e.g. the way cube(5) makes a size 5 cube) be worth implementing?
5. Consider rewriting examples as p=360/wavlength*xyz and then cos(p.x) etc.

[adrianVmariano]

It appears that some closing faces are missing here:

isosurface(function(p) (p.x^2-p.y^2)*p.z+p.z^2, isovalue=[-5,15], bounding_box=[[-5,-5,-5],[5,5,5]], voxel_size=.5);

[adrianVmariano]

isosurface(function(p) (p.x^2-p.y^2)*p.z+p.z^2+p.x*(p.y^2-p.z^2), isovalue=[-INF,5], bounding_box=[[-5,-5,-5],[5,5,5]], voxel_size=.5);

[adrianVmariano]

A bounded example?

isosurface(function(p)  (p.x*p.y*p.z^3+19*p.x^2*p.z^2)/norm(p)^2+(abs(p.y)^3+abs(p.z)^3+p.x^2), isovalue=[-INF,35], bounding_box=[[-8,-5,-5],[8,5,5]], voxel_size=.25);

Wonder if it can be simplified.

or perhaps

isosurface(function(p)  (p.x*p.y*p.z^3+19*p.x^2*p.z^2)/norm(p)^2+norm(p)^2, isovalue=[-INF,35], bounding_box=[[-8,-8,-6],[8,8,6]], voxel_size=.25);

[adrianVmariano]

Could this appear in a model as a kind of support pillar?

isosurface(function(p)  (p.x*p.y*p.z^3-3*p.x^2*p.z^2)/norm(p)^2+norm(p)^2, isovalue=[-INF,35], bounding_box=2*[[-10,-10,-8],[10,10,8]], voxel_size=.5);

Would be better if it could be adjusted so the cut face is entirely on top.

[amatulic]

Finished all items 1,3,4,5. The last one I had already done yesterday.

Consider removing the scalar default for isovalue

There is no scalar default for isovalue in isosurface(), in fact there has never been a default at all. We already agreed that metaballs should have a default isovalue=1.

I like those examples. The second one looks most interesting.

Working on debugging those holes you found in the clipping surface.

Would be better if it could be adjusted so the cut face is entirely on top.

Can't you adjust the bounding box to eliminate the other cut faces? Or are you suggesting to leave the side cuts open?

[adrianVmariano]

By scalar default I meant that you're currently allowed to give a scalar for isovalue and it defaults to [c,INF]. The proposal is that ONLY a range is accepted (scalar is an error) and you enter [c,INF] or [-INF,c] if you want an unbounded interval. This was mentioned with more context earlier in the longer message about default for closed.

Yeah, I tweaked the last example and actually posted the example on the chat with it attached to some cylinders and a cube. For some reason when I tried to enlarge the bounding box I didn't go about it right and it didn't seem to be helping. But if you just lower the box until it's below the flat face that obviously will work.

I do wonder if there's a way to design forms with more intent than my approach, which is to sort of randomly stick equations together.

[amatulic]

I fixed the holes by changing one "<" to "<=". The holes appeared when one corner of a clip face is exactly equal to max isovalue.

I am not sure of the purpose of requiring isosurface() to take a range for isovalue, but I've already done it. Would it be useful to keep the scalar, perhaps just show a range in all the examples, describe isovalue in terms of ranges in the documentation, and mention that a scalar is the same as [c,INF]? And what about metaballs? It can take either a scalar or range. A scalar makes sense probably for 99.9% of what anybody would want to do with metaballs, but now and then I'd like to make a metaball shell.

Update: isosurface() now requires a range, examples are updated. metaballs() can have either a scalar or a range. Still revising the documentation to reflect the changes.

[adrianVmariano]

Remember my caution about the use of = with floating point? The question is whether comparisons should be <x instead of <=x or if it should be <x+eps. Do you understand why it should be strict < instead of <=?

The reason I proposed having isosurface require a range is that a scalar is ambiguous in that it could be [-INF,c] or it could be [c,INF]. Now sure, you had it defined to be the latter---so strictly speaking, it's not ambiguous. But by accepting a scalar you enable the user to not think about this distinction---like I did---which makes it more likely to lead to confusion where you get the shape backwards because you never had to make a choice about which way it goes. It seems likely that if I had needed to specify a range for that original test that I got backwards that I would have gotten it right.

Metaball should definitely still accept a scalar, and range also is OK.

[amatulic]

It needs to be <=. These are comparison of function values at the corners. If the corner is equal to the max isovalue, the regular surface triangulation was picking this up but the clipping surface wasn't. If it isn't equal, even off by eps, it would still work. Also, f+eps does not work when f has a high value.

[amatulic]

All tasks in this PR are done, examples have been added, docs changed as discussed. This should be the "almost final draft" if all OK and docsgen doesn't abort.

[amatulic]

Please review and merge if it looks OK.

• Added new args grow_bounds and show_box
• Added triangle area check to final step of isosurface()
• Made all voxel corner function comparisons <= isovalue rather than < (it was < to avoid zero-area triangles, which is no longer a problem)
• Added example showing effect of low influence. Actually the fingers of the hand model show this, too.
• Removed low influence from most elephant parts and substituted cutoff to smooth it out a bit.

[adrianVmariano]

Probably not going to have time to actually review until tomorrow. One thought, it seems like grow_bounds arg should be instead exact_voxel or fixed_voxel or something like that. Nobody is going to think: I want the bounds to not be what I asked for. The reason you want the bounds to grow is because that happens when you don't allow the voxel to change and you want to prevent he voxel from changing.

[adrianVmariano]

Should the discussion of isovalue ranges use [c_min,c_max] instead of c1 and c2?

e result is a VNF with two bounding surfaces corresponding to the isosurfaces at $c_1$ and
// $c_2$. This is a shell object having two surfaces with a gap between them: the front faces of each
// surface face away from each other, and the backs face each other across the gap

This text is confusing. I can consider the "front" surface to be the one facing me and the back whatever is not facing me, which means that different parts of the surface change names depending on my viewpoint. This seems like's it's saying something self-evident---it's much harder to understand the explanation than to just recognize the fact you're trying to explain---but if you think it's important you need to call them the "outside" and "inside" surfaces....and I'm not sure how to state it clearly. A "gap" sounds like a space that is not part of the object (e.g. outside the object). I guess ultimately I'm not sure what exactly it is that you're trying to explain. That the inside is the space between the two surfaces (instead of the inside being the complement of that space)? Note that at least in my understanding of typography, em dashes don't have spaces around them, but actually that em dash should be a colon, not a dash.

You added a parenthetical "which is true when c_ = infinity" but we haven't yet admitted that this is an option. When I wrote it I figured I would start with finite values and then mention the infinite case later. I thought this was a gentler way to introduce things. If you disagree then the first sentence should say "the specified isovalue must be a range [cmin,cmax], where cmin can be -INF or cmax can be INF...." and then continue from there.

The sentence that starts "if your object is unbounded" I suggest a bold face preceeding sentence that says something like "Why did I get a cube?" And "may appear like a solid cube" I'd suggest "may appear to be a solid cube". The suggestion says to change the isovalue range and then says: one of [-INF,c2] or [c1,INF] or [c1,c2] should do the job. which is kind of obvious since those three cover all the possibilities. Instead it should say something like "you can correct this problem by inverting the isovalue range", so if you had used [c,INF] switch it to [-INF,c]. If you had used a bounded range like [c1,c2] then you will probably get the desired result with [-INF,c1] or [c2,INF], but might possibly need two separate isosurface invocations with both ranges.

[adrianVmariano]

It's probably also better to use INF instead of $\infty$ since users have to write it the former way in their code.

[adrianVmariano]

Should include an example with artifacts for isosurface and then show taking log to smooth it out?

[amatulic]

I made the changes you suggested.

I tried to demonstrate artifacts with a tilted cube but they aren't strong artifacts. Here's an example:

include <BOSL2/std.scad>
include <BOSL2/isosurface.scad>

function tiltcube(point, a, size, n) =
    let(p = rot(a, p=2*point/size))
        1 / (abs(p.x)^n + abs(p.y)^n + abs(p.z)^n) ^ (1/n);

isosurface(function (point) tiltcube(point, [0,15,22], 7, n=50),
    isovalue=[1,INF], voxel_size="auto",
    bounding_box=10, show_stats=true);

The artifacts are faint:

What did you do to get more pronounced ridges?

I can easily demonstrate smoothing with logs with the trivial example of a sphere with a high exponent, but that seems a little too trivial and too obvious that a log would linearize the function. I'd like to have an example that's simple but not obvious. The bullet isn't a good example because logs don't help with the flat base.

[adrianVmariano]

The cube artifact example was something like:

metaballs([zrot(22)*yrot(15), mb_cuboid(5, squareness=0.9, influence=0.05)], voxel_size=.5, bounding_box=12);

The exact artifacts are pretty sensitive to the voxel size.

[amatulic]

I saw what I was doing wrong with the cube, I wasn't taking it to the power of "influence". Anyway, the example in chat is better.

I have added one example showing both artifact and cure at the same time, because a side-by-side comparison, with two separate lines in the code for each version, clarifies the difference well. Basically the rule is, if you take the log of your function, you must also use the log of your isovalue. And, I learned, it must be the same log, not ln(function) and then log(isovalue) -- I did that by mistake.

Each shape has been clipped by the bounding box to save time:

include <BOSL2/std.scad>
include <BOSL2/isosurface.scad>
function shape(p) = let(x=p.x, y=p.y, z=p.z)
    exp(-((x+5)/5-3)^2-y^2)*exp(-((x+5)/3)^2-y^2-z^2)
    + exp(-((y+4)/5-3)^2-x^2)*exp(-((y+4)/3)^2-x^2-0.5*z^2);

left(6) isosurface(function (p) shape(p),
    isovalue=[EPSILON,INF],
        bounding_box=[[0,-10,-5],[9,10,6]], voxel_size=0.25);

right(6) isosurface(function (p) log(shape(p)),
    isovalue=[log(EPSILON),INF],
        bounding_box=[[0,-10,-5],[9,10,6]], voxel_size=0.25);

[adrianVmariano]

I generally think it works better to use two separate examples, which makes it easier to see which code produced which result. And when the pictures are small, it can work better, especially if you can get the code to be on the right in the wiki. In this case, the difference is huge so it's not like you need to study them carefully side by side. But I'll let you be the judge on what works best.

We should mention, if it wasn't already that the isosurfaces are preserved by taking the log (as long as the function is positive everywhere), so taking the log in principle gives exactly the same result. f(p)=c ===> log(f(p)) = log(c)

I will look over the revisions in the morning. The one question I have about artifacts is whether they can look different under different circumstances. Like the artifacts on the mb_cuboid look different from the artifacts on the above thing. So we may want to mention that the artifacts can manifest in different ways and any case of unexpected lack of smoothness that is not improved by smaller voxels might be caused by nonlinearity of your function around the isovalue. (Maybe you did this already.)

[amatulic]

It's obvious in this example which line produced which result. There's a left and a right in the image as well as in the code. Putting them both in the same place makes it clear that the underlying function is identical for both cases.

The artifacts all have one thing in common, as far as I've observed: They are rounded ridges. The spacing and amplitude of the ridges vary depending on voxel size, isovalue, and the underlying function.

I reformatted the example code so that it should fit on the right side of the image. I'm going to wait until tomorrow to see if @RAMilewski provides any further updates in #1559 and include those with the final submission after your review.

What we have works fine for typical use cases. I'm not looking forward to fixing up the clip face triangulations to handle parts or all of two different shells of min and max isovalue in the same voxel. I got started on the 81 required triangulation rules but it took me an hour to figure out the first 11 and I'm sure I made mistakes caused by needing base-3 addressing rather than base-2 for the isosurfaces. That's going to be several days of tedium to finish and test all cases.

[adrianVmariano]

Comments on isosurface() description:

1. Add to this "Setting isovalue to [-INF,c_max] or [c_min,INF] always produces an object with a single bounding isosurface." sentence ", which could be unbounded".
2. Not sure if you made the change I suggested of exact_voxel=true instead of fixed_bounds=true, or if you didn't like that proposal. It seems like varying voxel size should be the default with fixed bounds because that will be the prevailing user expectation.
3. Looking at the voxel_size/auto_voxel API I think it needs to be redone as follows: two arguments should be voxel_size and voxel_count. Only one can be given and there is no default. That eliminates the need to give voxel_size="auto" and then also giving the count argument in auto_voxel. Assert an error if the user provides both parameters.
4. You never say that voxel_size can be either scalar or vector. Probably add that right after it's introduced in paragraph 3.
5. Should there be an example of a very non-cubic voxel size, like voxel_size=[1,1,10]? Is there any reason to do this or not do it? (Maybe you have a very long skinny object?)
6. When describing the behavior of fixed_bounds or a replaced name for it I would not put one of the alternatives in parentheses but rather give them equal weight.
7. "causes the VNF to end at the bounding box" may suggest that it gets clipped if one thinks of a VNF as a solid. Maybe "causes the VNF faces to end at the bounding box?"
8. It seems that you don't document that the bounding box can be a scalar in the text.
9. voxel_count (or "auto") feature appears to be undocumented. Oh, wait...it's documented in a separate place from voxe_size. That's confusing. I think a lot of users won't read the paragraph that talks about voxel size and run time because they'll dismiss it as obvious.
10. First example seems kind of wordy and confusing because it seems like r is both the name of the function and the isovalue. I'm not sure what "the isosurface at r exists at every point where the expression equals r" means. Is "isosurface at r" meant to refer to isosurface with isovalue r? Also objects exist or they don't exist. Objects don't exist at a point. So whatever's happening at every x,y,z point it should be something other than "exist".
11. Second example says "if we the isovalue". Add "set"? But also could be less wordy. It says "Example 2" already. You don't need to repeat that in the text. So "An isovalue range of [8,10] gives a shell with inner radius 8 and outer radius 10". (I think interior and exterior are technically the wrong words here.)
12. I would start example there with "Here we set the isovalue range to [10,INF]...." Then at the end it doesn't make sense to cut an isovalue from something, since that's a number. It's hard to see a decent way to rephrase that while being technically correct. I got the awkward: "with the bounded object bounded by the isovalue 10 cut out of it" when I tried to do that. I suggest instead something like "appears as the bounding box with an interior hole whose boundary is the sphere with radius 10".
13. When introducing the gyroid maybe add "unlike the sphere, the gyroid does not have a bounded isosurface"? Or something about how it's always unbounded. You might consider if you think the order of examples is the best natural order to present them in. I was wondering if gyroid should be later, but I'm not sure.
14. I don't see any examples of artifacts yet? and use of log? I guess that's still coming?
15. Does auto voxel computation round up or down? This is not documented. If I ask for 8000 do I get no more than 8000 or am I guaranteed 8000?
16. I suggest generally not being parsimonious with examples. Consider including all the examples I listed above unless you really think some of them look the same, because this gives more of a flavor of what's possible. (The first one makes me think of a finger shaped recess if you just use part of it, though there's the problem of how to round the top.)
17. The show_box option fails with errors if the box is given as a scalar

For metaballs docs many of the above comments also apply, so check that. Also

1. I asked previously about talking about metaballs being summed. You added this to paragraph 3, but this is a technical detail that makes no sense to the reader at this point. It belongs in the section on custom metaball functions.
2. paragraph 3 introduces bounding box but doesn't mention the option of scalar size. Instead of "outside the bounds" I think "outside the bounding box". Also voxel_size should be noted as a scalar or vector when introduced.
3. Does the problem with faces that lie on the bounding box even still occur? I could not reproduce it. Maybe fixed by the equality test adjustments?
4. "availableto" -> "available to"
5. In paragraph on influence should we add "very small influence values can produce ridge-like artifacts or texture on the model. To avoid these keep influence above about 0.5 and consider using cutoff instead of very small influence"
6. How easy is it to swallow a metaball with a negative ball? Can this be done with two balls that don't share a center? A brief test suggests the answer is no. metaballs([IDENT, mb_sphere(r=4), right(.1), mb_sphere(r=6,negative=true)], bounding_box=2, voxel_size=.05); produces a small very artifacty ball not improved by taking logs.
7. For the isovalue paragraph, maybe lead with "The isovalue parameter parameter applies globally to all your metaballs and changes the appearance of your entire metaball object, possibly dramatically. It defaults to 1 and you don't usually need to change it. If you increase the isovalue..." Then at the end of this paragraph add something like "More details on the precise meaning of the isovalue appears below in the section on custom metaball functions"
8. "Both major and minor radius/diameter must be specified regardless of how they are named." is confusing because it suggests that you have to use r_maj or d_maj and also r_min or d_min. I think a statement like "You must provide a combination of inputs that completely specifies the torus" would be better.
9. "Cutoff is measured from the object's center unless otherwise noted below" The "below" needs to change to "above". Is it worth noting that cutoff for capsule is measured from the line of the center, not the origin center? Does anybody know how torus cutoff works?
10. "defines a bounded set — for example, a sphere", the dash should be a colon
11. There seems to be as yet no discussion of artifacts or artifact examples.

[amatulic]

Thanks for going through this in detail.

We have disagreement about a logical list of function arguments versus a good user experience in actual practice. Those two goals don't always align.

Isosurface()

1. Done.
2. What used to be grow_bounds=true is now fixed_bounds=false. Just a change of argument name with opposite logic. The user expectation is for bounds to be an arbitrary workspace. That's how it's intended to be used, and that's how it is being used in all our back-and-forth experiments. For example, I notice your own bounds are almost always bigger than needed, and approximate. You don't really care what the bounds are as long as they contain the feature of interest. The user typically adjusts bounds to optimize speed. I held my nose and added this capability to have unchanging bounds although I still disagree with it, because the suggestion originally arose without the benefit of experience actually making metaballs or isosurfaces. The universal expectation, in all I have read, and in my own tests, is for voxels to be cubes. Bounding box is just a work volume. Anyone who really wants to hold the bounds fixed can either set fixed_bounds=true or set it up so that the voxels fit inside.
3. voxel_count: Not done because it isn't a good user experience. It's voxel_size="auto" because I already tried it the way you suggest and found it inconvenient and impractical. If I am experimenting, I don't want to be changing an argument name back and forth. Sometimes I have voxel_size as a variable in my code, and I just want to change its value. In practice, I want to see a rough picture with an automatic voxel size, and then change the voxel size without having to switch to a different parameter name. I am really enjoying the voxel_size="auto" feature now. I am using it a lot. It wouldn't be as convenient if I had to use some other argument just to see a "draft" version.
   The size of the voxel is paramount, it controls resolution. The total number of voxels in an arbitrary bounding volume is irrelevant to me. A parameter like voxel_count would confuse me because I would think it's the count of voxels in my shape. The total voxel count isn't something I would ever care about. I called it auto_voxels because this is a good default for quick rough views when voxel_size="auto" and I doubt I would ever need to change the default.
4. The options for voxel_size are already documented in the argument description, but I put it in the text too.
5. Done, added example showing where it would be necessary to change the shape of the voxel.
6. Done, removed parenthetical note, actually gave fixed-size bounds more weight by starting out "Alternatively,...."
7. Done.
8. The scalar option for bounding box is already documented in the argument description, but I put it in the text also.
9. It isn't clear what's missing here. voxel_size="auto" and auto_voxels are always mentioned together, wherever they occur, and are already documented in the paragraph about execution time, as well as in the argument descriptions about voxel_size and auto_voxels. As I said in (3) above, nobody needs to care about the number of voxels. I doubt anyone would bother setting the auto_voxels argument. It just needs to be a value that results in a reasonable execution time.
10. Done. I agree it was confusing. I reworded example 1.
11. Done.
12. Done, reworded and simplified as suggested.
13. Done.
14. Already done. The artifacts and use of a log is the second-to-last example.
15. How this works isn't so simple. The number of voxels you get is approximately what auto_voxels requests. This is documented in the argument description for auto_voxels. As I said in (3) above, the exact number shouldn't matter to anyone.
    Here's how it works: A cubical auto-voxel size is first calculated as cube_root(bounding box volume / number of voxels), but we wouldn't know if these cubes fit exactly in all three box dimensions. Likewise, if the bounding box volume is fixed and the auto-voxel cube is resized instead, we wouldn't know if the total number of resized voxels is exactly the amount of voxels requested. It's possible for it to be exact because the default 8000 voxels is a cube number.
16. Already done, I think. Not sure which example you're referring to. The finger-shaped recess is the example demonstrating the use of a scalar for a bounding box. And the support pillar example is used to demonstrate show_box.
17. Fixed, I noticed that last night after pushing the PR. But I wonder why the docsgen check passed with that error?

Metaballs() - applied relevant changes above here too.

1. Done. Originally I wasn't sure where that sentence should go.
2. Done.
3. Yes, the problem with flat surfaces coplanar with the bounding box still occurs. Worst case is you have some degenerate faces that got generated because they are part of the isosurface as well as on the side of the bounding box. There's an even worse problem if someone tried to make metaballs with shells (isovalue range) because that isn't well-handled yet.
4. Done, and I found a couple other typos also.
5. Done, great suggestion, I just added an example but didn't think of describing it in the text.
6. I removed the part about swallowing other metaballs, changed it to "...which can result in hollows, dents, or reductions in size of other metaballs." The only time I've seen a metaball swallowed is when its infinite center is inside a voxel, in which case all 8 voxel corners around it are finite, and can be reduced to less than the isovalue.
7. Done, that's better wording.
8. Done.
9. Done, fixed wording. Does cutoff on a capsule affect an object near the end of the capsule, along the capsule's axis? I believe a cutoff with a torus is simply a torus-shaped cutoff, the shape being similar to what you'd get if you reduced isovalue. You'd get a fatter torus, possibly with small dents in place of a hole if it gets too fat.
10. Done.
11. Example 14 demonstrates artifacts with metaballs. I have mentioned this example in the text you suggested about artifacts.