|
|
|
|
|
|
| |
| |
|
|
From: William F Pokorny
Subject: FYI. Watch out for bad color to grey value conversions.
Date: 29 Jan 2021 12:57:43
Message: <60144c97$1@news.povray.org>
|
|
|
| |
| |
|
|
I'm going to blame the couple of you reviving a dormant thread about
color to grey conversions for this! Always knock on wood after
mentioning a feature... ;-)
For those compiling versions of POV-Ray v3.8, be on the look out for
issues with the color to grey conversions. Issues which might affect
more than .grey, .gray functionality, though that's where I picked up
the problem.
The following code:
//---
#version 3.8;
global_settings { assumed_gamma 1.0 }
#declare Color = rgb <0.0,1.0,0.0>;
#declare FnP = function { pigment { Color } }
#debug concat("\n\nred = ",str(Color.red,2,3)," \n")
#debug concat("green = ",str(Color.green,2,3)," \n")
#debug concat("blue = ",str(Color.blue,2,3)," \n")
#debug concat("filter = ",str(Color.filter,2,3)," \n")
#debug concat("transmit = ",str(Color.transmit,2,3)," \n")
#debug concat("gray = ",str(Color.gray,2,3)," \n")
#debug concat("grey = ",str(Color.grey,2,3)," \n")
#debug concat("\nFnGray = ",str(FnP(0,0,0).gray,2,3)," \n")
#debug concat("FnGrey = ",str(FnP(0,0,0).grey,9,16)," \n")
// .hf ?
// Most sensitive to red ? Using all three channels or?
#declare FnHf = function { FnP(0,0,0).hf }
#debug concat("FnHf = ",str(FnHf(0,0,0),9,16)," \n")
//Sanity check not something aside from .grey
#debug concat("FnVal0 = ",str(0.9876,2,3)," \n")
#debug concat("FnVal1 = ",str(0.98761,9,-1)," \n")
#debug concat(" ","\n\n")
//---
should generate:
red = 0.000
green = 1.000
blue = 0.000
filter = 0.000
transmit = 0.000
gray = 0.589
grey = 0.589
FnGray = 0.589
FnGrey = 0.5890000000000000
FnHf = 0.0039062470588235
FnVal0 = 0.988
FnVal1 = 0.987610
but the gray/grey values reported are wrong (for as right as they get)
for recent code versions and compiles.
First, thought it must be something in my povr changes because I have a
v38 compile done on New Year's Eve which works. After chasing ghosts for
the better part of a day, I finally tried re-compiling v3.8 master again
and to my surprise it too now fails.
I've since found a commit at c341c943 which works and a commit 5219e71c
(1) that fails - with 15 commits between. I should be able to zero in on
the issue, but unless I get lucky, it will take time.
(1) - Nine before Christoph's last v3.8 commit.
My guess at the moment is there is some dependency which changed in a
way breaking code. In broken versions, I don't seem to reach the
conversion code. Clang++ compiles mirror the g++ behavior.
FWIW! Expect it very much hit or miss whether - or when - others might
see this issue. Be aware of it. I'm unsure of the scope beyond the
.grey/.gray feature and using Ubuntu 20.04.1.
Bill P.
Post a reply to this message
|
|
| |
| |
|
|
From: Cousin Ricky
Subject: Re: FYI. Watch out for bad color to grey value conversions.
Date: 29 Jan 2021 20:49:58
Message: <6014bb46$1@news.povray.org>
|
|
|
| |
| |
|
|
On 2021-01-29 1:57 PM (-4), William F Pokorny wrote:
> I'm going to blame the couple of you reviving a dormant thread about
> color to grey conversions for this! Always knock on wood after
> mentioning a feature... ;-)
*forum Kung-Fu champion takes a bow*
> For those compiling versions of POV-Ray v3.8, be on the look out for
> issues with the color to grey conversions. Issues which might affect
> more than .grey, .gray functionality, though that's where I picked up
> the problem.
>
> [snip]
>
> but the gray/grey values reported are wrong (for as right as they get)
> for recent code versions and compiles.
Holy Chinavia, Batman!
I did further tests, and it appears that .gray/.grey simply returns the
value of the .green channel. This is a seriously stinky green bug!
Looks like we'll have to use one of these macros for the time being:
#macro DotGray (Color)
#local C = color Color;
(0.2126 * C.red + 0.7152 * C.green + 0.0722 * C.blue) // ITU-R BT.709
#end
---or---
#macro DotGrey (Colour)
#local C = colour Colour;
(0.2126 * C.red + 0.7152 * C.green + 0.0722 * C.blue) // ITU-R BT.709
#end
Unfortunately, these won't help with pigment functions.
> FWIW! Expect it very much hit or miss whether - or when - others might
> see this issue. Be aware of it. I'm unsure of the scope beyond the
> .grey/.gray feature and using Ubuntu 20.04.1.
I'd mostly ditched .gray for DotGray() anyway, since Christoph revealed
that the .gray formula was inaccurate in the real world, so I wouldn't
have run across this bug, at least not without re-rendering one of my
old scenes in 3.8. The only situations I can think of in the past 4
years where I *might* have used .gray would be on gray scale pigments,
for which, based on my findings, this bug would not manifest.
I'm using openSUSE, but prima facie this doesn't sound like an operating
system-dependent issue.
Post a reply to this message
|
|
| |
| |
|
|
From: William F Pokorny
Subject: Re: FYI. Watch out for bad color to grey value conversions.
Date: 29 Jan 2021 22:01:38
Message: <6014cc12$1@news.povray.org>
|
|
|
| |
| |
|
|
On 1/29/21 8:49 PM, Cousin Ricky wrote:
> On 2021-01-29 1:57 PM (-4), William F Pokorny wrote:
>> I'm going to blame the couple of you reviving a dormant thread about
>> color to grey conversions for this! Always knock on wood after
>> mentioning a feature... ;-)
>
> *forum Kung-Fu champion takes a bow*
>
As you should!
...
>
> I did further tests, and it appears that .gray/.grey simply returns the
> value of the .green channel. This is a seriously stinky green bug!
>
Yep, you figured it out more quickly than I did.
The problem was introduced at commit:
7d04c132 [parser] Modify token ID handling. Jan 22, 2019.
where the code was changed incorrectly to reach for the green value.
Users running any v3.8 version after that commit are exposed to wrong
answers.
The unsolved mystery is how we are sometimes getting the right result
despite the mistake! My guess is library vector optimizations sometimes
sticking the right value in [1] too, but I don't really know.
If you have a version in hand returning the right results, you are
probably OK continuing to use it. However, users should check their
version is actually working if running any version >= 7d04c132.
> Looks like we'll have to use one of these macros for the time being:
>
> #macro DotGray (Color)
> #local C = color Color;
> (0.2126 * C.red + 0.7152 * C.green + 0.0722 * C.blue) // ITU-R BT.709
> #end
>
> ---or---
>
> #macro DotGrey (Colour)
> #local C = colour Colour;
> (0.2126 * C.red + 0.7152 * C.green + 0.0722 * C.blue) // ITU-R BT.709
> #end
>
Yes, or for those compiling v3.8 at home, change the CASE(GRAY_TOKEN)
code in source/parser/parser_expressions.cpp to read:
CASE(GRAY_TOKEN)
*Terms=1; // TODO. Redundant? Is other than >=1 possible?
// Being explicit is safe for check below.
Express[0]=PreciseRGBFTColour(Express).Greyscale();
i=0; // i set to 0 at top. Again, being explicit the value
// we want is in Express[0] is safe.
END_CASE
and re-compile.
>
> I'd mostly ditched .gray for DotGray() anyway, since Christoph revealed
> that the .gray formula was inaccurate in the real world, so I wouldn't
> have run across this bug, at least not without re-rendering one of my
> old scenes in 3.8. The only situations I can think of in the past 4
> years where I *might* have used .gray would be on gray scale pigments,
> for which, based on my findings, this bug would not manifest.
>
In the povr branch the plan is to enable options (currently 5) by
configuration, with Rec.709 being the default.
What color to grey means is a muddy business when you dig in. I plan to
enable five because those align with using 3 coefficients, but there are
more complicated methods. Rec.2020 has two main stream methods and one
would require gamma twiddling pre and post calculation.
> I'm using openSUSE, but prima facie this doesn't sound like an operating
> system-dependent issue.
No and I appreciate the bug confirmation.
Bill P.
Post a reply to this message
|
|
| |
| |
|
|
From: Le Forgeron
Subject: Re: FYI. Watch out for bad color to grey value conversions.
Date: 30 Jan 2021 13:46:41
Message: <6015a991$1@news.povray.org>
|
|
|
| |
| |
|
|
Le 30/01/2021 à 04:01, William F Pokorny a écrit :
>
> Yes, or for those compiling v3.8 at home, change the CASE(GRAY_TOKEN)
> code in source/parser/parser_expressions.cpp to read:
>
> CASE(GRAY_TOKEN)
> *Terms=1; // TODO. Redundant? Is other than >=1 possible?
> // Being explicit is safe for check below.
> Express[0]=PreciseRGBFTColour(Express).Greyscale();
>
i=0; // i set to 0 at top. Again, being explicit the value
> // we want is in Express[0] is safe.
> END_CASE
>
> and re-compile.
I have problem with such changes.
*Terms is in/out , for Express (which is also in/out)
The initial code is only :
CASE(GRAY_TOKEN)
Express[0]=PreciseRGBFTColour(Express).Greyscale();
i=1;
Which is bogus because result of Greyscale is stored in .red, but i is
set to .green (to simplify a bit; index 0 is .red, .x as well as .u )
The check at the end of the switch for all "dot" expressions is:
if (i>=*Terms)
Error("Bad operands for period operator.");
*Terms=1;
Express[0]=Express[i];
To be able to use correctly Greyscale, we need at least *Terms to be 3.
So, to keep code simple AND correct, the correction should be:
CASE(GRAY_TOKEN)
Express[2]=PreciseRGBFTColour(Express).Greyscale();
i=2;
If the expression has 1 or 2 terms (on entry), it will be reported as
invalid (and that is good).
If the expression has 3 or more terms (on entry), Greyscale is fine, and
result can be provided.
That was my 0.02c !
Post a reply to this message
|
|
| |
| |
|
|
From: William F Pokorny
Subject: Re: FYI. Watch out for bad color to grey value conversions.
Date: 31 Jan 2021 00:44:41
Message: <601643c9$1@news.povray.org>
|
|
|
| |
| |
|
|
On 1/30/21 1:46 PM, Le_Forgeron wrote:
> Le 30/01/2021 à 04:01, William F Pokorny a écrit :
>>
>> Yes, or for those compiling v3.8 at home, change the CASE(GRAY_TOKEN)
>> code in source/parser/parser_expressions.cpp to read:
>>
>> CASE(GRAY_TOKEN)
>> *Terms=1; // TODO. Redundant? Is other than >=1 possible?
>>
// Being explicit is safe for check below.
>> Express[0]=PreciseRGBFTColour(Express).Greyscale();
>>
i=0; // i set to 0 at top. Again, being explicit the value
>> // we want is in Express[0] is safe.
>> END_CASE
>>
>> and re-compile.
>
> I have problem with such changes.
>
> *Terms is in/out , for Express (which is also in/out)
>
> The initial code is only :
>
> CASE(GRAY_TOKEN)
> Express[0]=PreciseRGBFTColour(Express).Greyscale();
> i=1;
>
Ah, digging into my TODO already. :-) Thank you for looking and thinking
about the code and my change/restoration.
The code ahead of the commit changing to the version above had just:
*Terms=1
Express[0]=PreciseRGBFTColour(Express).Greyscale();
with no setting of i at all instead leaning on it being set to 0 up top.
This gets to me not really knowing how Terms is getting set. Somebody
previously thought it should be hard set to 1... Colors get promoted
during parsing, so I think the terms will be at (5) (or is it sometimes
4?) This if terms is counting the color vector itself and not a color or
pigment id. An id reference is the more likely in a function{} kind of
reference and my thinking was 'perhaps' that ID counts as just one
expression term?
> Which is bogus because result of Greyscale is stored in .red, but i is
> set to .green (to simplify a bit; index 0 is .red, .x as well as .u )
>
> The check at the end of the switch for all "dot" expressions is:
>
> if (i>=*Terms)
> Error("Bad operands for period operator.");
> *Terms=1;
> Express[0]=Express[i];
>
> To be able to use correctly Greyscale, we need at least *Terms to be 3.
>
Agree we need a color vector using at least 3 terms and if we check
check that it's to the good to do it.
> So, to keep code simple AND correct, the correction should be:
>
> CASE(GRAY_TOKEN)
> Express[2]=PreciseRGBFTColour(Express).Greyscale();
> i=2;
>
> If the expression has 1 or 2 terms (on entry), it will be reported as
> invalid (and that is good).
> If the expression has 3 or more terms (on entry), Greyscale is fine, and
> result can be provided.
>
> That was my 0.02c !
>
Thanks. I say what I say, did what I did, not knowing how everything
'really' works in this code. What you have might be the most correct
encoding. I don't myself know without doing more testing and code
digestion.
---
While I have your attention!!! :-)
I was looking at .hf and thinking in my povr branch I'll probably delete
it(1) for a pair of inbuilt functions encoding and decoding variable bit
depths (2-17) for grey height field use modeled on the current .hf method.
In looking for the .hf code, I happened to see the code below in
fncode.cpp:
...
// compile member access
if(name[1] == '\0')
{
if((name[0] == 'x') || (name[0] == 'u'))
{
compile_instruction(OPCODE_LOAD, 1, 5, return_parameter_sp);
return;
}
else if((name[0] == 'y') || (name[0] == 'v'))
{
compile_instruction(OPCODE_LOAD, 1, 5, return_parameter_sp
+ 1);
return;
}
else if(name[0] == 'z')
{
compile_instruction(OPCODE_LOAD, 1, 5, return_parameter_sp
+ 2);
return;
}
else if(name[0] == 't')
{
compile_instruction(OPCODE_LOAD, 1, 5, return_parameter_sp
+ 3);
return;
}
}
if(strcmp(name, "red") == 0)
compile_instruction(OPCODE_LOAD, 1, 5, return_parameter_sp);
else if(strcmp(name, "green") == 0)
compile_instruction(OPCODE_LOAD, 1, 5, return_parameter_sp + 1);
else if(strcmp(name, "blue") == 0)
compile_instruction(OPCODE_LOAD, 1, 5, return_parameter_sp + 2);
else if(strcmp(name, "filter") == 0)
compile_instruction(OPCODE_LOAD, 1, 5, return_parameter_sp + 3);
else if(strcmp(name, "transmit") == 0)
compile_instruction(OPCODE_LOAD, 1, 5, return_parameter_sp + 4);
...
The block at the top is acting like there are only 4 color channels
moving 't' to the fourth channel and not supporting 'f'. The bottom
block with the longer names is acting like there are 5 color channels
still with both filter and transmit.
How is this code consistent? Seems to me only one or the other can be
right - meaning one set is wrong. Am I missing something?
Aside: I see now using the one character suffixes probably faster due
the longer names using one or two strcmp calls - though such SDL will
read less clearly.
(1) (.hf) was never an official feature. It's 16 bit using just two
channels in red and green assumed encoded at 8 bits. It's something easy
to get wrong as implemented. We have today multiple 16 bit encoding
formats available. I don't see a need for it.
Bill P.
Post a reply to this message
|
|
| |
| |
|
|
From: William F Pokorny
Subject: Re: FYI. Watch out for bad color to grey value conversions.
Date: 31 Jan 2021 01:06:39
Message: <601648ef$1@news.povray.org>
|
|
|
| |
| |
|
|
On 1/31/21 12:44 AM, William F Pokorny wrote:
> On 1/30/21 1:46 PM, Le_Forgeron wrote:
...
> The block at the top is acting like there are only 4 color channels
> moving 't' to the fourth channel and not supporting 'f'. The bottom
> block with the longer names is acting like there are 5 color channels
> still with both filter and transmit.
>
> How is this code consistent? Seems to me only one or the other can be
> right - meaning one set is wrong. Am I missing something?
>
...
And I am missing something. Just popped into my head. The t doesn't
stand for transmit. It's just a character index into the last entry of a
4D vector (time?). I shouldn't post on getting up in the middle of the
night - no coffee at all in my system. :-|
Bill P.
Post a reply to this message
|
|
| |
| |
|
|
From: William F Pokorny
Subject: Re: FYI. Watch out for bad color to grey value conversions.
Date: 31 Jan 2021 07:54:01
Message: <6016a869@news.povray.org>
|
|
|
| |
| |
|
|
On 1/31/21 12:44 AM, William F Pokorny wrote:
> On 1/30/21 1:46 PM, Le_Forgeron wrote:
...
>> If the expression has 1 or 2 terms (on entry), it will be reported as
>> invalid (and that is good).
>> If the expression has 3 or more terms (on entry), Greyscale is fine, and
>> result can be provided.
>>
>> That was my 0.02c !
>>
Finally got some sleep & starting my first cup. I woke up this morning
thinking if .blue (i=pBLUE) works with all the expressions passed, .grey
using (i=pBlue=2) must too. I'm going to change to something essentially
your version in:
i=pBLUE;
Express[i]=PreciseRGBFTColour(Express).Greyscale();
The test cases I've created (only 16) are clean with the code above. The
check can only fail if there was a reason for *this=1 - and then we'd
know what it is.
Thanks again for the review!
Bill P.
P.S. In my first response I started to ramble about what I saw in the
Greyscale() coding options, but deleted it. My writing tends to reflect
the storm in my head - and in my povr branch I'm eventually deleting all
the code changes related to the start of support for spectral rendering.
For completeness, I'll mention it.
In colour.h, if you search for NUM_COLOUR_CHANNELS, you'll see there is
an implementation of Greyscale() - not the one currently compiled -
which supports a 'channels' number of color channels. I had the thought
this might have been why *Terms was hard coded to 1, but if so .blue
need modification too and don't have it. (As would the .grey
functionality in fncode.cpp ... and expect much more)
Post a reply to this message
|
|
| |
| |
|
|
From: Cousin Ricky
Subject: Re: FYI. Watch out for bad color to grey value conversions.
Date: 31 Jan 2021 10:50:00
Message: <6016d1a8$1@news.povray.org>
|
|
|
| |
| |
|
|
On 2021-01-31 2:06 AM (-4), William F Pokorny wrote:
> On 1/31/21 12:44 AM, William F Pokorny wrote:
>> On 1/30/21 1:46 PM, Le_Forgeron wrote:
> ...
>> The block at the top is acting like there are only 4 color channels
>> moving 't' to the fourth channel and not supporting 'f'. The bottom
>> block with the longer names is acting like there are 5 color channels
>> still with both filter and transmit.
>>
>> How is this code consistent? Seems to me only one or the other can be
>> right - meaning one set is wrong. Am I missing something?
>>
> ...
>
> And I am missing something. Just popped into my head. The t doesn't
> stand for transmit. It's just a character index into the last entry of a
> 4D vector (time?). I shouldn't post on getting up in the middle of the
> night - no coffee at all in my system. :-|
That's one of the more confusing aspects of POV-Ray. For clarity, I
only use .t when I'm dealing with non-color 4-D vectors; for colors, I
spell out .filter and .transmit.
Maybe it should be spelled out in the docs in Impact or Arial Black: .t
DOES NOT STAND FOR TRANSMIT!
Post a reply to this message
|
|
| |
| |
|
|
From: Ash Holsenback
Subject: Re: FYI. Watch out for bad color to grey value conversions.
Date: 31 Jan 2021 13:23:32
Message: <6016f5a4$1@news.povray.org>
|
|
|
| |
| |
|
|
On 1/31/21 10:49 AM, Cousin Ricky wrote:
> On 2021-01-31 2:06 AM (-4), William F Pokorny wrote:
>> On 1/31/21 12:44 AM, William F Pokorny wrote:
>>> On 1/30/21 1:46 PM, Le_Forgeron wrote:
>> ...
>>> The block at the top is acting like there are only 4 color channels
>>> moving 't' to the fourth channel and not supporting 'f'. The bottom
>>> block with the longer names is acting like there are 5 color channels
>>> still with both filter and transmit.
>>>
>>> How is this code consistent? Seems to me only one or the other can be
>>> right - meaning one set is wrong. Am I missing something?
>>>
>> ...
>>
>> And I am missing something. Just popped into my head. The t doesn't
>> stand for transmit. It's just a character index into the last entry of
>> a 4D vector (time?). I shouldn't post on getting up in the middle of
>> the night - no coffee at all in my system. :-|
>
> That's one of the more confusing aspects of POV-Ray. For clarity, I
> only use .t when I'm dealing with non-color 4-D vectors; for colors, I
> spell out .filter and .transmit.
>
> Maybe it should be spelled out in the docs in Impact or Arial Black: .t
> DOES NOT STAND FOR TRANSMIT!
here: ???
http://wiki.povray.org/content/Reference:Vector_Expressions#Dot_Item_Access_for_Vectors
Post a reply to this message
|
|
| |
| |
|
|
From: Cousin Ricky
Subject: Re: FYI. Watch out for bad color to grey value conversions.
Date: 31 Jan 2021 16:03:59
Message: <60171b3f@news.povray.org>
|
|
|
| |
| |
|
|
On 2021-01-31 2:23 PM (-4), Ash Holsenback wrote:
> On 1/31/21 10:49 AM, Cousin Ricky wrote:
>>
>> Maybe it should be spelled out in the docs in Impact or Arial Black:
>> .t DOES NOT STAND FOR TRANSMIT!
>
> here: ???
>
http://wiki.povray.org/content/Reference:Vector_Expressions#Dot_Item_Access_for_Vectors
Yes, that would be an excellent place. It wouldn't need to be as
dramatic as I suggested, but a note that .t and .transmit are not
equivalent should be helpful to newbies.
Post a reply to this message
|
|
| |
| |
|
|
|
|
| |
|
|