-
Notifications
You must be signed in to change notification settings - Fork 129
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
Overhauled API, rewrote docs and examples #1555
base: master
Are you sure you want to change the base?
Conversation
Here's a list of various outstanding issues and topics (some maybe unresolved) from our discussion:
That's all I could find going back and searching through the discussion. |
Providing updates to the points above as I progress. This will take a while.
|
Note that the order of items above is arbitrary and not necessarily the natural order for implementation. I think if we are going to add exponential influence adjustment then isovalue default DEFINITELY should be 1, because that's the only way that the shapes are invariant and you change the influence value. Having a default doesn't mean you can't change it! It has become apparent that the isovalue is not a very important parameter, so the current ordering doesn't necessarily make sense. In other words, changing the current ordering may be the right thing to do. |
By the way, here's the integral for the cone-shaped metaball, with the "wire" of length L going from The field contribution at point Wolfram Alpha doesn't want to solve it. |
I wonder how dumbed down that is compared to the commercial Mathematica. In any case, the solution if it exists probably depends on special functions that are hard to compute, or is a very long complicated expression, or both. |
It would be nice if I could do anything with it. If there's a closed form for specific values of p, like p=1 or p=2, that would be helpful. |
Regarding 19, why can't you just apply move(origin,vnf) to the result of isosurface? Part of the argument for eliminating origin is that it's very easy to do the same thing with move, so there shouldn't be a significant complication. |
The solutions aren't horrible individually, but there doesn't seem to be a straightforward way to have a single
Because that doesn't work. Say my isosurface bounding box is [[20,20,20], [40,40,40]], and the voxel size is 3. Then internally, because the box size isn't divisible by the voxel size, the actual bounds are [[20,20,20], [41,41,41]], which fits a 7x7x7 array of voxels. Then it returns the isosurface with those new bounds centered on the origin, so the bounds are now [[-10.5,-10.5,-10.5], [10.5,10.5,10.5]]. But my original origin was supposed to be [20,20,20]. If I then perform move(origin, vnf), the new bottom of the bounding box is at [9.5,9.5,9.5], nowhere near where I wanted it to be. That is why the However, I now have metaballs() calculate the bounding box just like isosurface() does so that it knows what's being returned and adjusts for it. It's a kludge, inelegant to do the same thing twice, I don't like it, but it meets the requirement of removing |
If adding a function call is adding 1/2 second I'll bet the run time for those integral solutions would be substantially longer. And I also suspect it'll create an object that behaves differently than the existing balls types. Regarding influence you could assume that the output is 1/r and apply influence with an exponential. Regarding the bounds, I hadn't realized this issue at all. My first thinking was that voxels should be adjusted to fit the bounding box, but that creates non-square voxels, which I'm not sure is a good idea. I don't like the idea of the output having an unknown size, but I'm also not sure I see a solution I like. Like specify the bounding box in voxels instead of units? It does seem like if you're going to alter the size of the bounding box, the new box should be centered on the requested box. That is, if you request [20,40], you should not be computing [20,41] but rather [19.5,40.5], so that the error is symmetric. You could calculate the number of voxels on a range, figure out the adjusted start, and then use count() to generate the data points. But rounding error aside for voxel fit, I don't understand how you can end up "nowhere" near the right place. In your example the bounding box requested is [20,40], so you calculate the origin to be 30. You compute and get a solution that you think is [-10,10] but it's actually [-10.5,10.5] due to rounding errors. You correct the origin by adding 30 to this and now you think you have [20,40] but you actually have [19.5,40.5], which the answer incorrectly shifted by 1/2 unit. But if you centered the bounding box it would be correct. I have no clue where you pulled the 9.5 bound from in your example above. And it seems like the origin parameter is mainly needed to compensate for not centering the bounding box on the requested bounding box. Note that it's also ok to have internal parameters between the module and function or other internal functions. But if I have overlooked something and there's a compelling reason why users need the origin parameter, we should understand that. |
Would it make sense to allow |
The simplest and most elegant solution is to pass Each voxel that contains the surface is a data structure that includes an x,y,z coordinate among other things, and that coordinate is determined by the origin of the bounding box. In the case of metaballs, a bounding box must be specified, but that can't be passed into isosurface() when passing an array, so it would be cleaner to pass the origin of the bounding box as a private argument. isosurface() needs an origin anyway to work, whether it calculates one or receives one. I've just made this change, which eliminated a handful of lines of duplicate code, and also eliminated the need to dereference vnf to move the coordinates in it. |
I still don't understand why you need the origin parameter. Did you fix the off-centering of the bounding box? It seems like if isosurface produces a centered output then you will always know what to pass to move() to fix the output if necessary. If you don't for some reason, then users might have this problem too, so we need to understand that. |
Earlier timing tests were using the wrong version of code without the matrix operation to build the fieldarray. Now using the correct version. Timing tests (approx average of 5 runs each case), using Example 1. Case 1. This is my baseline, and takes 4.4 seconds to execute.
Case 2. Moving the r>=cutoff and influence==1 checks into mb_shaping(), execution time increased to 5.0 seconds. Case 3. Removing the check for influence==1 and just letting it calculate, execution time increased to 5.6 seconds. Case 4. Moving the r>=cutoff check back into mb_sphere and letting mb_shaping() calculate: 5.5 seconds (one trial was 5.6 but average was 5.5). Case 5. Bypassing mb_shaping() altogether and putting everything inline in mb_sphere: slightly under 4.4 seconds, negligible difference from "baseline". This is all with |
So if I am understanding this correctly, simply adding a function call is increasing time from 4.4s to 5s, which is a 14% increase. That suggests that it might be necessary to inline everything. There is one possibility worth exploring if you want to maximize run time, which is to change the function literal. You aren't showing everything in the above quote because you don't give the definition of mb_sphere. It could look something like
Something along those lines. So then there is a version of mb_sphere that just computes r/norm(v), another version that computes sign_inf*(r/norm(v))^inv_influence, and a third version that does the works. This is obviously more of a nuisance to write and maintain., But presumably it will deliver the best overall response since there is no extra function call and in baseline cases, no unnecessary tests or computation at all. |
Note also, which parameter do you think is more important to users, the sphere influence or the cutoff? I imagined influence to be more important....but I don't know....which ever one is more important should go first so it can be used positionally without having to set the other one. (Oh, also the tests I show above ==INF I think don't work. Need to look up how to check that. Or maybe just have no default and check if the parameter is set.) |
Test for infinity is |
Inlining all of that would actually have four separate outcomes, for each combination of influence =1 or !=1 and cutoff>r or <r. I think what I have now is a good compromise between maintainability and speed.
As I play around with it, I find I use cutoff more often. Influence is nice, however, to cause all metaballs except one to grow or shrink by changing the influence of that one metaball. It looks like the remaining open tasks in the list above are mostly examples, and having arguments like r and d like in OpenSCAD. You know I am not in favor of that; I can imagine the complaints, from those who don't read documentation carefully, about not getting the diameter asked for. I prefer Keep in mind also that every metaball function would need logic checks and conversions between d and r. I think this is unnecessary overhead in time-critical functions for no actual benefit. |
By writing the stuff in here we kind of hide it from everybody else. There will not be complaints from people about balls not having the "right" size. The other thing is that we should write this so that it's the easiest to use and easiest to remember how to use. I would say it's more likely we'd get complaints from users that mb_sphere(d=2) doesn't work. The problem with "coeff" is that it means nothing and doesn't convey what the argument does. So actually it's more hostile to users who don't read the documentation. And a user reading the cheatsheet, or just looking at the usage message won't have guidance about what the argument means. It will only get worse if you have multiple arguments to a ball. It's also more limiting because you can't for example have the option of giving diameter or radius to a sphere, or specifying cubes by size in analogy with how cube() works. In terms of what other people think, we know that lightwave and blender both have you specify the radius of your ball. So it appears standard to do that. |
I was looking at the docs for blender metaballs and realized that they split negative influence off as a totally separate parameter. We might consider doing this. As noted, right now the sign is stripped from influence anyway, so it is a separated calculation. Setting a ball to negative influence appears to be rare and also a very confusing mistake to make. If we required influence to be positive we could specify negative balls with |
Does there need to be |
I don't see the need. Besides the fact that the user function isn't invoked the same way, the only requirements for the user function are that it takes the distance vector |
Ok. I was just thinking that providing mb_custom would automatically provide the proper scaling, influence and cutoff. I don't really think custom functions are likely to get much use. Consider how much trouble we're having coming up with a cone function. |
You might consider using list_shape in isosurface to get nx, ny and nz. That will check that the matrix given is actually a cube. (It will return undef for variable dimensions. The code that does the bounding box is kinda hard to follow. Just reading the code, is the bounding box centered on the requested bounding box now? I think we need to understand why you think you need the origin parameter, because if there's a real need there that I don't understand, then users might actually need it. And if there isn't a real need, your code could be cleaner because you shouldn't need it either. I would have done the bounding box calculation without all those half-voxel additions and roundings, something like
which I guess is longer code? The point sets could also be separated into xset, yset and zset instead and instead of calculating x, y and z in the loops you just pull them from the list, which will be (slightly) faster. (In the case of calculating the function you can loop directly on x, y and z, which is a little bit faster as well.) I can't quite tell what's going on with _origin. If the point is just to handle positioning of the bounding box for arrays I don't think it's necessary assuming that you center the box. After calculation you just apply move(origin) with the above definition of origin. You can compute only origin in isosurface() and the rest of the bounding box stuff can happen once in the private function instead of being repeated. Am I missing something? |
I think the API is looking good and just a few details remain to be cleaned up:
|
Now that's all done, I still need to come up with missing examples from the previous to-do list. |
so 8 different args in total that can be combined in different ways. |
Maybe there should be an example that uses a skew() transformation just to point out that this possibility exists. |
The text "You can specify different ones for each metaball on the list" has no antecedent for "ones". I still feel like the text explaining metaballs kinda says a lot without being helpful. Might be worth asking Richard to rewrite it. The synopsis has one word with content, which is "metaballs". I'm not sure if there's a way to add more content. But "creates a model" should be deleted because everything does that. That it's within a bounding box isn't noteworthy at the synopsis level. Maybe "Creates a group of 3D metaballs (smoothly connected blobs)". For isosurface maybe "Creates a 3D isosurface from a function or array of values". Some additional word other than isosurface would be good. Richard said "isosurface" meant nothing to him. Maybe he has an idea. When showing args as When introducing the parameters you might say something like, "All of the metballs take positional and named parameters that specify the size of the metaball (e.g. radius, height). The size arguments are the same as those for the regular objects of the same type (e.g. a sphere accepts both Note that influence does not "determine the extent of influence". Influence is always infinite unless cutoff is used. Perhaps the "strength" of influence? The text here is kind of obscure. The math is, as far as I can tell, not of much interest. I can tell you that I have very little intuition from the math about what exactly influence is going to do. I would say that the influence model math should be in a technical note that is the last paragraph of the docs. When defining the parameter, maybe "determines the strength of influence and interaction of the metaball. If you increase this from its default value of 1 the ball grows faster when it interact, and it interacts at longer range. Decreasing the influence has the reverse effect. Small changes in the influence can have a large effect. For example, setting For optional arguments, put brackets around them. I don't know what the cuboid description "rounded corners that get more rounded at farther distances" means. Maybe rewrite the cuboid description something like "a cuboid with rounded edges and corners, where the It's not relevant to the user that a taxicab geometry calculation is used for the octahedron. For torus, just stick the other args on the parameter line. I actually didn't realize they were supported because they were missing from that line. And then the text can say "You can specify the major and minor radius or diameter of the torus with r_maj, r_min, d_maj or d_min; you can also give its size using the inner and outer radius and diameter, or, od, ir, and id." Something along those lines so the user doesn't have to go look up torus() to know what the args are/do. I would say "a torus metaball" when describing the object. Note that it's ok to give IDENT as the transform for a ball, so a move operation isn't strictly required. It's also OK to only give scale, or only rot. You have the first example move([-15,0,0]). Change that to left(15) and right(15) for the second ball. Our convention in examples generlaly is to only use move if there are at least 2 nonzero values in the arg, unless there's some reason it makes the code cleaner (like list of vectors where one happens to have just one nonzero). So like the example of 5 balls is fine as is. But the example before it could be:
mb_cyl is missing the l, height and length args from its definition, so it errors out. Torus is missing od, or, id and ir. Maybe you've already fixed this stuff. The example looks weird.... For tetrahedron is there some reason not to use for loops? It seems like it would be nice to show how to do this since it is perhaps not obvious to everybody and is a useful technique:
|
I made all the changes you described above. For the last example, I cannot use Instead I did mb_cyl() now takes the total length. |
Not sure how I overlooked that you were skipping a point with the tetrahedron model. I think your solution is fine. You can also do |
Docs for isosurface suggest that realized bounding box is not centered. That's been changed, right, so the docs need to be updated to agree? I'm posting some excerpts/proposed revisions of the main docs to the general discussion because I think that will better attract Richard's attention and I think it would be good to get his input. |
Should there be multiple versions of mb_cyl for the case of influence enabled, cutoff enabled, etc? The revsurf functions should be hidden with leading underscore. I think mb_cutoff should be invoked with (1+dist)/coef in the outside case. In the inside case it should be something else. I think maybe |
I think describe capsule as "vertical cylinder with hemispherical cap at each end". For "connector" you say "spherical endpoints" which is weird---not sure what those are. How about "A cylinder with hemispherical caps (like capsule) but specified to connect from point p1 to point p2. The specified points are at the two ends of the straight portion of the shape (the centers of the two capping hemispheres)." It appears that if p1==p2 it will work and produce a sphere? Is that right? If yes, document that this case works. If not, document that it does not work. As noted earlier, you can use |
Not done yet:
|
Consider this a draft for review. All examples tested and working with new API.
Slight speed improvement using matrix operations to calculate metaball contributions to the bounding volume.
I kept the axis parameter on the torus because I found it convenient to have the torus start aligned a particular way before applying rotation.axis parameter has been removed.I also retained the ellipsoid because there's an ambiguity when scaling and rotating a sphere (you get different results depending on the order, and while that order may always be the same, the user may not know that). I found it clearer to have an ellipsoid with defined axis, which I then rotate.