Skip to content
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

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

Open
wfpokorny opened this issue Jul 28, 2021 · 1 comment
Labels
bug reported or triaged as proper bug, i.e. not working as designed, and affecting all platforms

Comments

@wfpokorny
Copy link
Contributor

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: [email protected]

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);


  1. 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.

  1. 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. #310

Create_Ini=bool does not work as documented. #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. #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. #145

@wfpokorny wfpokorny added the bug? reported as bug; triage pending label Jul 28, 2021
@c-lipka c-lipka added bug reported or triaged as proper bug, i.e. not working as designed, and affecting all platforms and removed bug? reported as bug; triage pending labels Jul 28, 2021
@c-lipka c-lipka added this to the v3.8.0-beta.2 milestone Jul 28, 2021
@c-lipka
Copy link
Contributor

c-lipka commented Jul 28, 2021

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.

c-lipka added a commit to c-lipka/povray that referenced this issue Jul 29, 2021
Precision of `Declare` INI setting was erroneously truncated to 6 digits. Reverted back to 7 digits, matching v3.6 behavior.
c-lipka added a commit that referenced this issue Aug 9, 2021
Precision of `Declare` INI setting was erroneously truncated to 6 digits. Reverted back to 7 digits, matching v3.6 behavior.
@c-lipka c-lipka removed this from the v3.8.0-beta.2 milestone Aug 9, 2021
trevorsandy pushed a commit to trevorsandy/povray that referenced this issue May 21, 2022
Precision of `Declare` INI setting was erroneously truncated to 6 digits. Reverted back to 7 digits, matching v3.6 behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug reported or triaged as proper bug, i.e. not working as designed, and affecting all platforms
Projects
None yet
Development

No branches or pull requests

2 participants