search

WerWolv / ImHex-Patterns

Hex patterns, include patterns and magic files for the use with the ImHex Hex Editor
https://github.com/WerWolv/ImHex
GNU General Public License v2.0
640 stars 168 forks source link

pattern: FBX - NodeRecord subtrees not working correctly #275

Closed Hikodroid closed 1 month ago

Hikodroid commented 1 month ago

This is a follow up to the comment I made in my pull request: https://github.com/WerWolv/ImHex-Patterns/pull/271

I added a print statement to Blenders FBX exporter and it returned this - I formatted and shortened the output:

FBXElem(id=b'', props=[], props_type=bytearray(b''), elems=[ FBXElem(id=b'FBXHeaderExtension', props=[], props_type=bytearray(b''), elems=[ FBXElem(id=b'FBXHeaderVersion', props=[1003], props_type=bytearray(b'I'), elems=[]), FBXElem(id=b'FBXVersion', props=[7400], props_type=bytearray(b'I'), elems=[]), FBXElem(id=b'EncryptionType', props=[0], props_type=bytearray(b'I'), elems=[]), FBXElem(id=b'CreationTimeStamp', props=[], props_type=bytearray(b''), elems=[ FBXElem(id=b'Version', props=[1000], props_type=bytearray(b'I'), elems=[]), FBXElem(id=b'Year', props=[2024], props_type=bytearray(b'I'), elems=[]), FBXElem(id=b'Month', props=[7], props_type=bytearray(b'I'), elems=[]), FBXElem(id=b'Day', props=[2], props_type=bytearray(b'I'), elems=[]), FBXElem(id=b'Hour', props=[18], props_type=bytearray(b'I'), elems=[]), FBXElem(id=b'Minute', props=[41], props_type=bytearray(b'I'), elems=[]), FBXElem(id=b'Second', props=[18], props_type=bytearray(b'I'), elems=[]), FBXElem(id=b'Millisecond', props=[922], props_type=bytearray(b'I'), elems=[]) ]), .....

This showed me that "Version" is supposed to be a child node of "CreationTimeStamp" but my hexpat does not do that, it is just another NodeRecord that follows "CreationTimeStamp". I have no idea why it does that. I closed my pr because I can't figure this one out. Sorry for the hassle that this probably causes.

WerWolv commented 1 month ago

Ah sorry, I just ran the pattern on your test file and it ran fine. Sorry for jumping the gun a bit too quickly.

Would you like me to remove your pattern again? Or do you think it could still be useful for somebody even if it's not 100% correct?

Hikodroid commented 1 month ago

I managed to find a workaround for the previously mentioned issue, see https://github.com/WerWolv/ImHex-Patterns/pull/276 The way it works is that it alternates between two differently named but otherwise identical NodeRecord structs as the parser descends into the tree-like structure of NodeRecords. I also added a non-functional version in a previous commit because it looks like it is supposed to work and I'd like to have some feedback on that. The line NodeRecord nestedList[while($ < endOffset)]; seems to cause some sort of confusion when adding child nodes. I assume this is because the child nodes use the same name for the struct member and the same data type, which is what my workaround tries to address.

Hikodroid commented 1 month ago

I looked further into this matter and it turns out that using a template was the cause of my issues. Because of that I could simplify the fbx hexpat, see https://github.com/WerWolv/ImHex-Patterns/pull/277 I'm not sure why I can't use a template type T in NodeRecord32/64 to merge both into a single NodeRecord\<T> instead of u32/u64 respectively. Even just changing it to NodeRecord\<T> without actually using the template type T in the struct members causes the issue.

paxcut commented 1 month ago

I looked at the code posted in the PR assuming that is the latest code, but the PR is closed and you seem to be making revisions so my comments may not apply to the latest code. Having said that and based on the code I saw and the comments you are posting, I suspect you are trying to uses recursive templated patterns which the pattern language does not currently support due to implementation limitations. Previously the attempted use of such constructs resulted in ImHex crashing so to avoid that the code only generates one recursion level and then it stops . It doesn't warn you of the danger and it doesn't clarify the action that was taken so at first it looks like it worked but when you look closer you find that something is wrong. I believe that their implementation is somewhat necessary and if they can't be made to work, then there should be better information supplied than what it currently does.

I know how to deal with the case of non-type template parameters using the parent keyword. For the type template arguments the only solution I can think of is template instantiation which is what you are currently using based on your comments. I just wanted to answer the questions posted in your last post. Not the best source of information, but there are discussions about this topic in the Discord channels if you are interested in learning more about the issue and that is probably the only place.

Hikodroid commented 1 month ago

The information that it only generates one recursion level actually explains everything that I observed. I wondered why the tree structure has not been parsed correctly to the depth level that I would have expected. It added child nodes to the wrong parent. So that means my latest PR is probably the best solution to this problem. Thank you very much! I wonder if I should close this issue then?

paxcut commented 1 month ago

I can see how it can be confusing unless you knew what it was doing which is what motivated me to post. Closing it as "not currently planned for" is what I would do, but feel free to do otherwise. My impression is that a proper fix requires major restructuring of how recursive patterns are implemented so it may be a while before this can be cleared as resolved.