POV-Ray / povray

The Persistence of Vision Raytracer: http://www.povray.org/
GNU Affero General Public License v3.0
1.35k stars 282 forks source link

Loss of precision where variables declared on command line / ini files. [Bug] #428

Open wfpokorny opened 3 years ago

wfpokorny commented 3 years ago

Applies to v3.7 and v3.8. For reference see the newsgroup thread:

http://news.povray.org/povray.bugreports/thread/%3C60fed6cd%241%40news.povray.org%3E/ or Message: web.60fea0b2e0f64d045e0fed26cde94f1@news.povray.org

FWIW. I've fixed this in my povr branch and it required three groupings of updates.


1) In source/backend/control/scene.cpp inside the conditional if(parseOptions.Exist(kPOVAttrib_Declare) == true) and just below the line:

case kPOVMSType_Float:

add:

// precision for doubles. Default is (6), truncating cmd line and // ini value declarations. sstr.precision(17);


2) The povms code must be updated to move the current single float support to double floats. I see the povms support being at single floats as a general issue.

In source/povms/povms.h, POVMSFloat and POVMSIEEEFloat should be changed from float to double. The define of HexToPOVMSIEEEFloat reinterpret_cast<POVMSInt > changed to reinterpret_cast<POV_ULONG >.

In source/povms/povms.cpp, kPOVMSStreamFloatSize should be changed to 8. The initialization 'call' to HexToPOVMSIEEEFloat should be changed to: HexToPOVMSIEEEFloat(0x4466335521324354ul, data_ieeefloat);

and ahead of the POVMSStream_BuildOrderTable calls add the lines:

stream[4] = 0x21;
stream[5] = 0x32;
stream[6] = 0x43;
stream[7] = 0x54;

Aside: Currently the povms code is set up to communicate with machines of different architectures as if the render was spread across many machines. As far as I know this isn't how anyone is running POV-Ray, and I think we should consider defining POVMS_NO_ORDERED_STREAM_DATA for, perhaps, some performance gains. Quick testing of this 'same machine' mode worked, but I didn't do any performance testing.

Updates must also be made in a couple files to remove the literal 'f' suffixes for the clip template function to match on type. See: source/backend/scene/view.cpp and source/frontend/renderfrontend.cpp.

3) In source/frontend/processrenderoptions.cpp where creating ini files from current settings the (float) cast of floatval should be removed - so POVMSFloat is used.

This last(3) is related to the code writing all settings to an ini file for which there are long standing issues as in github #309 an #310.

Create_Ini=FILENAME does not write all INI settings. https://github.com/POV-Ray/povray/issues/310

Create_Ini=bool does not work as documented. https://github.com/POV-Ray/povray/issues/309


Aside:

Our documentation claims declares on the command line or in an ini file only support floats.

https://wiki.povray.org/content/Reference:Scene_Parsing_Options

In working on this issue I was surprised to find internal code indicating this is not strictly the case. We can define strings too, which of course is very handy. I had no clue all these years. :-(

On digging found this very acknowledgment in comments to github #120 with the caveat the format one would most expect to use doesn't work in v3.7/v3.8.

Port of FS328 - Unlike 3.6.1, 3.7 on does do not allow = char in scene name. https://github.com/POV-Ray/povray/issues/120

wherein Andrey Zholos long ago pointed out:

$ povray declare=\'foo=\"bar\"\' # works (internally foo the string "bar") $ povray declare=foo=\"bar\" # doesn't work (Except in povr it does)

I was ignorant of these details and tried the second string definition on the command line in povr to find it worked. That second form for unix/linux I would consider normal for declaring a string on the command line to povray. I don't, though, know what povr change fixed the second case. My guess is my 'partial fix' mentioned in #145 comments for '=' characters in file names, but?

Port of FS42 - command line parameters are not parsed properly on Unix and unlike 3.6. https://github.com/POV-Ray/povray/issues/145

c-lipka commented 3 years ago

I'd like to leave the POVMS interface unchanged for v3.8, and only tackle the regression vs. v3.6 there.

For v4.0, I think we should indeed fully support double precision for any numerical values passed via the command line or INI file. Though the solution there might actually be entirely different, such as just passing the values as strings while at the same time making automatic conversion from strings to numeric values a feature of the scene language itself.