Extension of ovus to: #306

LeForgeron · 2017-07-06T16:06:00Z

allow the center of the top sphere to not be on the top of the bottom sphere (via "distance xxx")"
allow the radius of the torus spindle to be manually specified (via "radius xxx")
allow precision of the root solver to be tuned ( via "precision xxx")
And change of UV-Mapping to be similar to the uv-mapping of cone & cylinder

* allow the center of the top sphere to not be on the top of the bottom sphere (via "distance xxx")" * allow the radius of the torus spindle to be manually specified (via "radius xxx") * allow precision of the root solver to be tuned ( via "precision xxx") And change of UV-Mapping to be similar to the uv-mapping of cone & cylinder

c-lipka · 2017-07-06T17:43:21Z

source/core/shape/ovus.cpp

 *  }
 *
 *  The so long awaited 'Egg' forms.
 *
 *  Normally, the bottom_radius is bigger than the top_radius
-*  the center of the top sphere is at the zenith of the bottom sphere
+*  the center of the top sphere is at the zenith of the bottom sphere, unless
+*    a greater distance d is specified with "distance"


Is the distance d an offset to the default position, or the Y(?) coordinate of the top sphere's center?

It's the distance from the origin, it must be at least the radius of the bottom sphere, but can be bigger (previously, it was always the radius of the bottom sphere)

c-lipka

Some minor cosmetic gripes, but also one medium gripe about what I'd consider dirty coding (using float as boolean expression), and one major gripe about a test that looks positively wrong to me). Please do fix/double-check at least the latter two.

c-lipka · 2017-07-06T17:53:43Z

source/core/shape/ovus.cpp

@@ -820,9 +827,15 @@ void Ovus::Compute_BBox()
    {
        biggest = TopRadius;
    }
+    // handle degenerated ovus in sphere
+    if (!BottomRadius)


Please replace this with a proper test. BottomRadius is a float, not a boolean, so test should be either exact comparison with 0.0 or of style fabs(...) < EPSILON. Also, check whether this shouldn't actually be a less-or-equal rather than an equal test.

c-lipka · 2017-07-06T18:02:36Z

source/core/shape/ovus.cpp

-        return;
+    if ((P[Y]>EPSILON)&&(P[Y]<(VerticalSpherePosition-EPSILON)))
+    {
+    // when not on a face, the range 0.25 to 0.75 is used (just plain magic 25% for face, no other reason, but it makes C-Lipka happy)


If you really want to make C-Lipka happy, please amend this comment ;)

Dividing at 1/4 and 3/4 has the advantage of the division being exactly at a pixel boundary if the texture is an image 2^N by 2^M pixels in size, which is common for image textures originally designed for mesh-based renderers. It also happens to work for 20N by 20M pixels, which is common for image textures with "arbitrary" sizes.

c-lipka · 2017-07-06T18:06:14Z

source/core/shape/ovus.cpp


-    if (len == 0.0)
-        return;
+    if ((P[Y]>EPSILON)&&(P[Y]<(VerticalSpherePosition-EPSILON)))


Can we get a bit more clarity in this expression (and a few others) by more liberal use of whitespace?

c-lipka · 2017-07-06T18:08:18Z

source/core/shape/ovus.cpp

-    Make_BBox(BBox, -biggest, -BottomRadius, -biggest,
-              2.0 * biggest, 2.0 * BottomRadius + TopRadius, 2.0 * biggest);
+    Make_BBox(BBox, -biggest, bottom, -biggest,
+            2.0 * biggest, length, 2.0 * biggest);


Please indent to after opening brace (same as it was before) for clarity.

c-lipka · 2017-07-06T18:09:13Z

source/core/shape/ovus.cpp

+    else if (P[Y]>EPSILON)
+    {
+    // aka P[Y] is above VerticalSpherePositon, use TopRadius, from 0% to 25%
+       phi = 0.0;


Please stick to a 4-blanks indent.
Also, please indent the comment inside the braces.

c-lipka · 2017-07-06T18:09:32Z

source/core/shape/ovus.cpp

-        y /= len;
-        z /= len;
+    // aka P[Y] is below origin (<0), use BottomRadius, from 75% to 100%
+       phi = 1.0;


Indent, see above

c-lipka · 2017-07-06T18:09:37Z

source/core/shape/ovus.cpp

+       phi = 1.0;
+       if (BottomRadius)
+       {
+          t = ((BottomRadius+P[Y])/(BottomRadius));


Indent, see above

c-lipka · 2017-07-06T18:10:18Z

source/core/shape/ovus.cpp

+       }
+       else if (TopRadius)
+       {
+        // degenerate ovus in sphere


Indent, see above

c-lipka · 2017-07-06T18:14:07Z

source/parser/parser.cpp

-            distance = Object->ConnectingRadius - Object->BottomRadius;
-            Object->BottomVertical = -Object->VerticalPosition * Object->BottomRadius / distance;
-        }
+        Error("distance of Ovus must be greater than bottom radius");


Why? Shouldn't the ovus object also work with a lower VSP, provided VSP+TR > BR?

c-lipka · 2017-07-06T18:36:37Z

From what I see here, when you created the pull request you seem to have checked a box granting reviewers (such as myself) write access to the branch corresponding to the pull request (feature-extended-ovus in your fork of the GitHub repo), allowing them to directly modify your pull request; let me know if this was intentional or otherwise.

Also note that any further commits you push to the feature-extended-ovus branch will automatically be picked up as amendments to this pull request.

LeForgeron · 2017-07-06T18:53:39Z

From what I see here, when you created the pull request you seem to have checked a box granting reviewers (such as myself) write access to the branch corresponding to the pull request (feature-extended-ovus in your fork of the GitHub repo), allowing them to directly modify your pull request; let me know if this was intentional or otherwise.

Not intentional, probably default, but welcomed.

Also note that any further commits you push to the feature-extended-ovus branch will automatically be picked up as amendments to this pull request.

Ok, might work on it tomorrow, if I still need to.

c-lipka reviewed Jul 6, 2017

View reviewed changes

c-lipka suggested changes Jul 6, 2017

View reviewed changes

amend after review of pull request#306

af1a119

c-lipka approved these changes Jul 11, 2017

View reviewed changes

c-lipka merged commit e39f5af into POV-Ray:release/v3.8.0 Sep 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extension of ovus to: #306

Extension of ovus to: #306

LeForgeron commented Jul 6, 2017

c-lipka Jul 6, 2017

LeForgeron Jul 6, 2017

c-lipka left a comment

c-lipka Jul 6, 2017

c-lipka Jul 6, 2017

c-lipka Jul 6, 2017

c-lipka Jul 6, 2017

c-lipka Jul 6, 2017

c-lipka Jul 6, 2017

c-lipka Jul 6, 2017

c-lipka Jul 6, 2017

c-lipka Jul 6, 2017

c-lipka commented Jul 6, 2017

LeForgeron commented Jul 6, 2017

Extension of ovus to: #306

Extension of ovus to: #306

Conversation

LeForgeron commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c-lipka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c-lipka commented Jul 6, 2017

LeForgeron commented Jul 6, 2017