Hello POV-Team!

So, here comes feedback on povray-3.6. 
It is a list not sorted in any way...

Some aspects of this message clearly show that POVRay (I mean the quality 
of the program) could benefit from publication of the sources prior to 
releasing the binaries. But I know that there are prople around here which 
do not want to hear that...

------

More than a year ago, on Sunday 09 February 2003 22:08:27, I posted 
to this group:
>/* NK phmap */
>/* this function checks if 'object' is equal to 'parent' or is a child


>static int IsObjectInCSG(OBJECT *Object, OBJECT *parent)
>{


>

>











>}

Shortly later, Thorsten Froehlich wrote back:
>Wolfgang Wieser wrote: 
>> What about applying this trivial patch?
>> I think there is no need to continue the for() loop once
>> we set found=true.
>
>Yes, it has been noted.  It will be included sometime in the future.

One year later looking at the code, nothing has changed...

(Thorsten will probably answer that "sometime in the future" does not 
necessarily mean version 3.6. Oh, well...)

-------

Some patch against the following warning...

Warning: The maximum gradient found was 0.000, but max_gradient of the
isosurface was set to 0.000. The isosurface may contain holes!
Adjust max_gradient to get a proper rendering of the isosurface.

...was not included, too. 

(I proposed replacing %.3f -> %.3g at that time = 21 May 2003 20:45:07)

------

No patch against the parametric object trouble (crash and getting black) 
reported by me (20 Jun 2003 21:49:03) was introduced (these bugs are 
sill present and stating in the docu that no bugs are known is incorrect). 

It's really strange: THIS patch made it into the code (fpmetric.cpp): 

598c588,591
<               Assign_BBox_Vect(Param->BBox.Lower_Left,
Param->container.box.corner1);
---
>               // [ABX 20.01.2004] Low_Left introduced to hide BCC 5.5 bug
>               BBOX_VECT& Low_Left = Param->BBox.Lower_Left;
> 
>               Assign_BBox_Vect(Low_Left, Param->container.box.corner1);

<polemic>A patch against a bug in a compiler (BCC) nobody is 
using...</polemic>

However, around line 420 in fpmetric.cpp, 

    /* 1 copy */
    if ((SectorNum[i] *= 2) >= Max_intNumber)
           SectorNum[i] = Max_intNumber;
        SectorNum[i + 1] = SectorNum[i];
        SectorNum[i]++;

POVRay still happily accesses SectorNum[i + 1] outside array boundaries. 
(On my box using my posted test scene, POVRay will, however, _not_ crash 
because it happens that the overwritten regions of memory are non-lethal.)

Furthermore, the parametric object test case will still render a completely 
black object. Somebody proposed a patch against that and I have the code 
here. I'll come to this issue in a later posting. 

------

I suggest to apply the following patch in pattern.cpp which was also 
already proposed by me earlier. This will bring the return value into the 
legal range 0..1 while not changing the results (the 0.25 phase is absorbed 
by changing the sign of the y argument of atan2()): 

 static DBL radial_pattern (VECTOR EPoint)
 {
   register DBL value;
 
   if ((fabs(EPoint[X])<0.001) && (fabs(EPoint[Z])<0.001))
   {
     value = 0.25;
   }
   else
   {
-    value = 0.25 + (atan2(EPoint[X],EPoint[Z]) + M_PI) / TWO_M_PI;
+    value = (atan2(EPoint[Z],-EPoint[X]) + M_PI) / TWO_M_PI;
   }
 
   return(value);
 }

------

The render status line should be patched to append some whitespace at the 
end of the line. Otherwise, shorter lines will contain the end of the 
message of the previous longer line resulting in something like: 

  0:01:03 Rendering line 449 of 480 at 4x4

...when we're actually at 1x1 pixels. 

------

Closig the window while rendering will now work but the rendering is not 
stopped in this case. This is okay for "-X" traces but when using 
command line option "+X", closing the window should IMO abort the render 
just as pressing "q" in the window. 

The following patch for xwin.cpp will change that: 

         if (theEvent.xclient.message_type == WM_PROTOCOLS &&
             theEvent.xclient.data.l[0] == WM_DELETE_WINDOW)
         {
-          // [NC] abortRender = true;
+          if(opts.Options & EXITENABLE)  abortRender = true;
           refresh = false;

BTW, Thanks to Nicolas Calimet for including my other fixes (key and 
event handling). 

------

  Finite objects:            4
  Infinite objects:          0
  Light sources:             1
  Total:                     5

Hmm... I must admit that I do not at all understand the "Total" line. 
Why does it make sense to add apples and bananas?

------

Okay, I think this was enough feedback for now. 
I hope I can finally write some _positive_ feedback when looking 
at the image IO code. 

Regards,
Wolfgang

Post a reply to this message