WerWolv / PatternLanguage

The Pattern Language used by the ImHex Hex Editor
https://imhex.werwolv.net
GNU Lesser General Public License v2.1
168 stars 44 forks source link

[Bug] Cannot `match` with ranged enum values. #69

Closed yellows111 closed 1 year ago

yellows111 commented 1 year ago

Attempting to use a match operator with a enum with a range value compiles, but seems to not include the resolving variable in the pattern data result.

The value (7) is in the range, but when it's matched to, it fails to return any result. Specifying the value in the enum directly (like the None type is with a value of 3) will compile, so will inverting the statement 7 ... 0 (despite it being invalid in the Value output), but I assuming that's only because it's checking the first value instead of the whole range.

#include <std/io.pat>

bitfield BGR5A1 {
b: 5;
g: 5;
r: 5;
a: 1;
} [[static, sealed, color(std::format("{0:02X}{1:02X}{2:02X}FF", r * 8, g * 8, b * 8))]];

enum TextureFormat : u32 {
    Uncompressed = 0 ... 7,
    None = 3,
    Compressed = 8 ... 15
};

struct Texture {
    TextureFormat format @ 8 [[no_unique_address]];
    match (format) {
        (TextureFormat::Compressed): BGR5A1 texture[0]; //unimplemented
        (TextureFormat::Uncompressed): BGR5A1 texture[4000];
    }
};

Texture texture @ 0;

Interpreter: ImHex (Win64, Portable) 1.30.1

I've been trying to write a pattern to parse the PS2 icon format. I've gotten to the part where I've got the shapes, just no texture to apply onto the shape.

Example of an uncompressed icon is the Super Mario 64 PS2 homebrew port, which uses a textured cube for it's display model.

paxcut commented 1 year ago

Thank you for submitting the issue. I would like to help but in order to reproduce the issue I would need both the pattern, which you already supply, and a suitable input file. Very often the exact nature if the issue can only be found under certain conditions. Alternatively you can try to find a case where the error doesnt depend on the input file. In that case please supply step by step instructions to reproduce the issue.

PS: incidentally I am working on the 3d visualizer that comes in ImHex and I would be willing to share information on PS2 patterns . Let me know.

yellows111 commented 1 year ago

examples.zip Here are the icons I am using with my pattern (it's a lot larger than the reproduction sample!), only sm64.icn will have this issue, as poinie_*.ico is a bit more unique in the fact that it's texture-less (all colours are from vertex).

paxcut commented 1 year ago

the zip file contains several files. which one produces the error all of them?

yellows111 commented 1 year ago

only sm64.icn will have this issue

paxcut commented 1 year ago

it is being reported an uncompressed . what do you see in this window? image

yellows111 commented 1 year ago

it is being reported an uncompressed . what do you see in this window?

Uncompressed. The issue is that the Texture structure isn't being added to the pattern data output.

Extra reproduction notes: Change Texture texture @ 0; to Texture texture @ 0x398; for sm64.icn.

paxcut commented 1 year ago

For one thing you are using the same name for two variables which in this case is not causing the error but it is a good idea to use different names. Also where do you want the 16 bit array to start at 0x8?

yellows111 commented 1 year ago

Also where do you want the 16 bit array to start at 0x8? @ 0x398;

Start: 0x398, End: 0x8397 (0x4000 in length, I noticed I wrote 4000 instead of 0x4000 (128*128), doesn't really matter here, as the issue is with the match operator not understanding all values for an enum range)

but it is a good idea to use different names.

I guess you could call one of them (I|i)mage, if it bothers too much.

paxcut commented 1 year ago

I can confirm the error is as reported. to see this change the condition to read to be

      (7): {

and it starts to work.

paxcut commented 1 year ago

until the bug is fixed as a workaround you can use non-range based values to continue on but you already know that. Again thanks for taking the time to report this.

paxcut commented 1 year ago

I dont think you can use a literal like that in the match statement. What works is to state the range explicitly in the match statement like this

(0 ... 7) :

I will ask in the discord support channel about how it is supposed to work as I dint know for sure and I am only guessing.

yellows111 commented 1 year ago

What works is to state the range explicitly in the match statement like this

E: error[E0013]: Ambiguity error.
E:   --> <Source Code>:84:16
E: 84 |         (0 ... 7): BGR5A1 texture[0x4000];
E:                     ^
E:                     Match is ambiguous. Both case 1 and 3 match.
E: 
E: 
I: Evaluation took 0.243516s

This only happens on icons that use the None type (poinie_*).

paxcut commented 1 year ago

that happens because you defined None to be the value of 3 , but 3 is also in 0...7 so if you try to use 3 it doesn't know if you mean None or 0 ... 7 hence the error. you need to exclude 3 from 0 ... 7 . fr example you could do

((0 ... 2) | (4 ... 7)) :

It looks like the fact that the literal is not working is a bug and you should be able to use the literal.

yellows111 commented 1 year ago
((0 ... 2) | (4 ... 7)) :

I don't think that works.

E: error[P0002]: Unexpected expression
E:   --> <Source Code>:89:11
E: 89 |         ((0 ... 2) | (4 ... 7)): BGR5A1 texture[0x4000];
E:                ^
E:                Mismatched '(' in mathematical expression.
E: 
E: 
I: Evaluation took 0.0024645s
E: error[P0002]: Unexpected expression
E:   --> <Source Code>:89:17
E: 89 |         (0 ... 2) | (4 ... 7): BGR5A1 texture[0x4000];
E:                      ^
E:                      Expected ':' after case condition, got Operator (|).
E: 
E: 
I: Evaluation took 0.0024122s
paxcut commented 1 year ago

my bad sorry use this instead. i tested it and it works

( 0 ... 2 | 4 ... 7 ) :
yellows111 commented 1 year ago

I tested it and it works

Sure, now that works, however the root issue still remains, I was really wishing to use match with enum ranges. Oh well. Not everything works every so often.

Leaving the issue open, because even without overlapping values, people may want to do the exact same with ranged enum values in a match block. I shouldn't ignore their problems for what's being fixed by ignoring the root issue. That isn't being nice to others.

paxcut commented 1 year ago

Absolutely, you are quite correct. I wasn't suggesting that the issue is resolved, in fact I did say that the literals not working seems like a bug and that needs to be addressed regardless, I was just finding something you can use while the bug is being fixed so that you can continue working and not have to wait for the fix.

paxcut commented 1 year ago

It seems I was wrong when I assumed that you had found a bug. If you read the documentation on range based enums it clearly states that using the enums in an expression yields their initial value. That means that based on the current documented behavior using the enum values inside the match cases , which are expressions, creates a single value to match which is what happens. Additionally the only way to create cases with ranges is to place the range explicitly in the case as documented in the match docs.

To be able to allow the use of enum range literals in match cases the minimal change would be to change the meaning of the literals in conditional statements, which was suggested and apparently okayed, but I raised concerns that changing only conditionals would create even more confusion because conditional expressions are just a type of expressions and different behavior for them exists nowhere else. After a number of hours my concerns have not been answered so I'll keep waiting on it , but I wanted to make sure to let you know where it stands and to give you a chance to post your thoughts on the matter as it may help in finding a resolution.

for example How would we interpret cases like (TextureFormat::Compressed + 5):? or u32 temp=TextureFormat::Compressed;?

yellows111 commented 1 year ago

If you read the documentation on range based enums it clearly states that using the enums in an expression yields their initial value.

"When using a range value in a mathematical expression, it will yield the start value of the range."

I guess that makes sense in the context in a format that wouldn't be supporting ranges, like a static number.

My problem is that you can use inline range values in a match, but not ones from an enum. Not to mention that you can use at least some mathematical expressions on inline match ranges that are invalid to an enum.

Minimum implementation to allowing match to use whole range numbers in enum, would be just parsing input that comes from an enum as a inline match range.

for example How would we interpret cases like (TextureFormat::Compressed + 5):? or u32 temp=TextureFormat::Compressed;?

Inline match ranges interprets the addition to a range by adding 5 to the side of the range it was added on.

// this won't compile but this syntax is completely valid in match blocks
(0 + 5 ... 6) // becomes (5 ... 6)
(0 ... 6 + 5) // becomes (0 ... 11)
(0 + 5 ... 6 + 5) // becomes (5 ... 11)

I guess you could just add 5 to both sides of the enum range while being interpreted.

For definitions like the u32 example, old behaviour makes sense there. I don't see a reason to change how that works.

paxcut commented 1 year ago

My problem is that you can use inline range values in a match, but not ones from an enum. Not to mention that you can use at least some mathematical expressions on inline match ranges that are invalid to an enum.

that is incorrect on both accounts. You can use range valued enums in a match case but they evaluate to a single valued as documented. ranged valued enums can be used in any expression but they always return a value. Inline range values can be used in two places and two places only.1) As values of enums and 2) as cases of a match statement. Their use anywhere else is illegal and the use of range valued enums outside their definition will always return a value. More over you may not combine the inline range value arithmetically with any other values. Writing (0 ... 5+1) is not adding 1 to 0 ... 5 . it is adding 1 to 5 to produce 0 ... 6. To see this try to write (( 0 ... 5 ) + 1) and you will get a syntax error.

The documentation about the range values evaluating to a number is specifically stated about ranged enums because inline range values cannot be used in expressions and the only variables that can hold range values are enums.

So right now the behavior you report as being a bug is the documented behavior and not a bug. What you are asking now instead is a feature request that involves changing the way range enums are evaluated so that instead of returning a value as they currently do they would be able to perform arithmetic operations with other ranges and single values that are logically and mathematically sound. That is not the current documented behavior and that is something that would need to be discussed as it is not clear how to define said arithmetic operations. For example one may choose to use interval arithmetic as used in the estimation of errors in measurements. another way would be to treat the ranges as finite differences (differential approximations) and use discrete mathematics to combine them.

But thats not what this issue is about. This issue states that there is a bug in ImHex when in fact the behavior seen is fully documented and the results match the documented behavior without error. To further demonstrate my point the documentation on match statements never once mentions that you can use range valued enums as ranges and in fact mentions two operators that maybe used in match case expressions, the ... and the | operators. Those two are explicitly stated as the only possible operators that can be used to create ranges and combine them which is why (0 ... 3 | 5 ... 7) is not only legal but also the only way to express that combination of ranges in the current documented behavior

paxcut commented 1 year ago

I was suggesting to turn this issue into a feature request where the behavior that is documented is changed so that the statements involving ranged enums no longer return a value. But to do that we first need to agree that there is no bug in the current behavior so I tried to help you see why that is the case. Once that is agreed and also that the current implementation can be improved then we need to discuss what the new arithmetic would be. Here is an example of how interval arithmetic works

image those 4 operations together with the identification of n as the range [n ... n] would define the arithmetic operations in a mathematical consistent way. What remains to decide if ranges can be ordered and if not as I think they can't then define equality, inequality, set inclusion, union, intersection, etc.. so that conditionals can be defined sing union for or intersection for and exclusion for not inclusion for less than and greater than.

In addition to improve the algebraic capabilities of the pattern language the interval operator could also augment the set of array types to include slices which could naturally be defined using index intervals. Both contributions would make the pattern language more powerful than it already is, but I can also understand if you do not wish to participate in further discussions which may be the reason why you closed this issue.