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

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

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

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

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

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

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

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

On 1/31/21 4:03 PM, Cousin Ricky wrote:
> 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.

hope this works:
http://wiki.povray.org/content/Reference:Vector_Expressions#Dot_Item_Access_for_Vectors

Post a reply to this message

Cousin Ricky <ric### [at] yahoocom> wrote:

> >> Maybe it should be spelled out in the docs in Impact or Arial Black:
> >> .t DOES NOT STAND FOR TRANSMIT!

Oh, how this brings back memories of an unexpectedly LOOONG thread...

:O

http://news.povray.org/povray.advanced-users/message/%3Cweb.577ee122ff0686005e7df57c0%40news.povray.org%3E/

Post a reply to this message