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

[Bug] The "parent" keyword isn't always parsed in patterns #1599

Open
1 task
ZwipZwapZapony opened this issue Mar 17, 2024 · 4 comments
Open
1 task

[Bug] The "parent" keyword isn't always parsed in patterns #1599

ZwipZwapZapony opened this issue Mar 17, 2024 · 4 comments
Labels
bug Something isn't working

Comments

@ZwipZwapZapony
Copy link

Operating System

Windows

What's the issue you encountered?

When executing a pattern, the parent keyword is not always parsed correctly, leading to compile or runtime errors.

u8 mip[parent.mipInfos[parent.i].decompressedSize]; works. parent.i += 1; causes a compile error. u8 mip[123] [[name(std::format("mip{}", parent.i))]] causes a runtime error.

How can the issue be reproduced?

Use the attached pattern on the attached binary file.

ImHex Version

v13.33.1

ImHex Build Type

  • Nightly or built from sources

Installation type

Portable zip from the GitHub Releases page

Additional context?

[Pattern: id Tech 7 BImage.hexpat] - [Binary file: combat_shotgun.tga$bc3$streamed]

@paxcut
Copy link
Contributor

paxcut commented Apr 2, 2024

there may be no problem parsing parent in patterns in your example code.

u8 mip[123] [[name(std::format("mip{}", parent.i))]]

the problem here is that the name attribute adds another level to parent. To access i you need to use parent.parent.i and it works fine.

parent.i += 1;

whatever the problem is with this, it has nothing to do with parent. in general any statement like (any name).(any member)=(any value) inside a struct will give the same error. In this case accessing i in from the parent to increment it is completely unnecessary as it can be incremented in its own structure using i+=1. (btw, using hidden in a local variable has no effect since local variables are always hidden)

@paxcut
Copy link
Contributor

paxcut commented Apr 2, 2024

I looked at your code in more detail and realized that you were trying to use i as an indication of the current array index. ImHex has a special function that returns the current array index in std::core::array_index(). with a small modification to your previous posted pattern (that tried to create an array of negative size) I was able to read the entire input file using the following changes:
in ImageMip

  u8 mip[parent.mipInfos[parent.i+std::core::array_index()].decompressedSize] [[name(std::format("mip{}", parent.parent.i+std::core::array_index()))]];

in Image

    ImageMip mip[header.mipCount - header.streamDBMipCount];

this way i doesn't need to be updated since it can't be updated from the child which is why array_index function was created for.

@ZwipZwapZapony
Copy link
Author

[...] u8 mip[123] [[name(std::format("mip{}", parent.i))]] the problem here is that the name attribute adds another level to parent. To access i you need to use parent.parent.i and it works fine. [...]

I guess that that makes sense (especially for this pointing to the currently-being-made pattern/variable), but it feels odd considering that u8 x = 10; u8 mip[123] [[name(std::format("mip{}", x))]]; doesn't need parent. to access the x next to it.
Seeing as you don't need parent. in attributes when referencing other patterns/variables in the same struct, the first parent. "layer" is essentially useless*; it might be more intuitive if the Pattern Language automatically "adds" an extra parent. layer within attributes (so that parent.var refers to the same thing both inside and outside of attributes).

* If you specifically want to refer to the struct containing the pattern/variable currently being created, then this.parent should work, so there won't be any lost functionality by auto-adding an extra parent. layer.

[...] parent.i += 1; whatever the problem is with this, it has nothing to do with parent. in general any statement like (any name).(any member)=(any value) inside a struct will give the same error. [...]

I found that out later: WerWolv/PatternLanguage#87
But worth noting: Due to the "you need an extra parent. in attributes" thing above, plus WerWolv telling me that parent might not be parsed correctly (and telling me to make this GitHub issue about it), I thought that parent was just a little iffy in general.

[...] ImHex has a special function that returns the current array index in std::core::array_index(). [...]

I've since edited my pattern file to use that (or more specifically u32 i = std::core::array_index() + parent.header.streamDBMipCount within the ImageMip struct, to use just i later throughout the struct).
I just thought that it would've looked cleaner to do it like I was trying to do at first, considering that i is supposed to start at a non-zero value here and std::core::array_index() + parent.header.streamDBMipCount is somewhat of a "mess" to look at.

@paxcut
Copy link
Contributor

paxcut commented Apr 2, 2024

but it feels odd considering that u8 x = 10; u8 mip[123] [[name(std::format("mip{}", x))]]; doesn't need parent. to access the x next to it.

You are right that it doesn't need a parent there but note that it also works with parent.x equally well. I suspect that a parent layer is being skipped in the case of attributes referring to members of the parent struct, at least that's the only explanation I can think of of why it works with and without parent, considering how it needs two if the member is on the parent struct.

Don't get me wrong, I'm not saying that parent works as it should, I just don't see the problem in the example you posted. parent feature has been plagued with problems because it is a hard feature to implement. At one point creating arrays added a parent layer, but it looks like that's not the case anymore, so you may be right about it being fishy, but I still don't understand fully how it is supposed to work in reality to state that it is not working correctly.

std::core::array_index() + parent.header.streamDBMipCount is somewhat of a "mess" to look at.

yes, I suppose it is. There are ways to make it look better (for example, in my version I use parent.i instead of parent.header....) like writing a function or using a macro define, but in the end array_index() is the only way I know of to obtain a working current index and I tried lots of ways a while back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants