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