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

Some fixes and features to make it easier to access data #5

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

kamilkrzyskow
Copy link

@kamilkrzyskow kamilkrzyskow commented Nov 10, 2024

I fat fingered the enter key on title and it created an empty PR 🙄

Anyway, I always wanted to be able to access script data from Python, there is PyDecDat but it doesn't support Ikarus/Lego extended scripts, and iirc was rather slow.
So for now I typically used DecDat and parsed the .d file with Python to extract data.

When I saw this project I was happy that I don't have to parse the .d myself 😄...or so I thought.
Turned out there are some kinks to be straightened out, so I will fix them along the way.
Edits for maintainers are enabled, so if you want to adjust some things, then go ahead ✌️

My goal is not to create another DecDat, or so I think for now 🤔
My goal is to easily extract data, and cross-connected it afterwards in my external scripts.

Example of previously extracted data about dialogues of an NPC together with their possible routines:

"NONE_2246_BRADLOCK": {
  "dialogues": {
    "DIA_BRADLOCK_AFTERMINE": "",
    "DIA_BRADLOCK_AMBIENT": "How are you doing?",
    "DIA_BRADLOCK_CANTPASS": "",
    "Some": "Other"
  },
  "id": 2246,
  "instance_name": "NONE_2246_BRADLOCK",
  "name": "Bradlock",
  "routines": [
    "START",
    "Q308",
    "GUARD",
    "VOLKERTRIALOG",
    "TOT"
  ]
},

@kamilkrzyskow kamilkrzyskow changed the title Some fixes Some fixes and features to make it easier to access data Nov 10, 2024
Copy link
Member

@lmichaelis lmichaelis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! Thank you very much for submitting a PR! I like the changes you're proposing, however I can see an issue with setting the string encoding. The way it is now, whenever zenkit._native.load is called with an encoding, it will change the encoding for all strings loaded by the library. This is unexpected behaviour, I'd think.

I propose instead, to add a function for setting the encoding globally and then applying ot to all string encoding and decoding performed by the library (e.g. a zenkit.set_string_encoding function). This should then also replace the hardcoded "windows-1252" in all str.encode calls (you don't need to do that right now, but that'd be the plan).

@lmichaelis lmichaelis added the enhancement New feature or request label Nov 10, 2024
src/zenkit/_native.py Outdated Show resolved Hide resolved
@kamilkrzyskow
Copy link
Author

kamilkrzyskow commented Nov 12, 2024

What's the "real" functional difference between DaedalusScript and DaedalusVm? The latter is a subclass, and the load function has a different value. I began working with DaedalusScript, but then trying to load DaedalusInstance.from_native resulted in INVALID type. Fortunately, the from_native is used in the code, so I could find that the symbol first needs to be initialized with DaedalusVm.init_instance. Would it be possible to move the init_instance to DaedalusScript?

Would it be possible to detect if a symbol is a function parameter inside the DLL, other than the .PAR suffix?
I was thinking about adding a is_param which would check the name, but perhaps there is something more performant 🤔

The DaedalusScript.symbols property generates a list of symbols every time it's accessed. This is a huge performance hog.
I'm not sure whether this should be cached (either with LRU / directly as self._symbols), or replaced with a generator to force the user to make their own list(symbols).

Same goes to other aspects, like the get_parent, I'm not sure whether or not to cache parent indexes for faster lookup in subsequent symbols 🤔

Lastly, it seems that DaedalusVm requires registering externals, even the defaults from Gothic. Why can't ZenKit provide them with the DLL, is there some licensing issue?

@lmichaelis
Copy link
Member

The difference between DaedalusVm and DaedalusScript is, that the script cannot run any Daedalus code. Its purpose is to facilitate access to all symbols in the binary as well as the bytecode instructions associated with them. The VM is capable of actually running code.

Daedalus instances are runtime-initialized objects, meaning they have to be explicitly initialized by the engine by running some Daedalus code. That's the job of the VM. So no, init_instance can't be moved into DaedalusScript. You can, however, create a VM the same way as a script, and as you already noticed, you will retain access to all functionality of DaedalusScript.


Getting the parameters of a Daedalus function isn't as simple as checking for a .PARn suffix. ZenKit has a function for getting the parameters of a given function, though I am not sure if it's exposed to Python right now. I am unsure if an is_param on symbols is feasible with the current DAT parser.


Caching is something I haven't considered yet, because there are many cases in ZenKit where caching is the wrong answer. In those examples specifically though, I think caving would be a valid strategy, because the script itself is immutable. A generator could be great too.


Externals are not registered by ZenKit, because most of them have something to do with game engine stuff, which ZenKit is not concerned with. Of course, things like CONCATSTRINGS could be implmented without issue, but WLD_InsertNpc is another story.

@kamilkrzyskow
Copy link
Author

Hello again, I wish you a belated happy new year ^^
as for the progress on the PR since last time, the PR became a victim of me attaching my personal project to the PR, so other projects got in the way, but I'm back at it, and even sort of got it working ✌️

However, there is some bug, which is possibly there in 1.3.0 or I'm missing something about memory management and DAT file access.

"$INSTANCE_HELP": {
  "crashed": "Init Failed - NPC is None"
},
"BDT_10030_ADDON_BUDDLER": {
  "crashed": "exception: access violation reading 0x000000474E49525C"
},
"BDT_10031_ADDON_WACHE": {
  "dialogues": {
    "DIA_ADDON_BDT_10031_WACHE_HI": "Alles klar?"
  },
  "id": 10031,
  "instance_name": "BDT_10031_ADDON_WACHE",
  "name": "Wache",
  "routines": [
    "START"
  ]
},

As you can see there are NPC Instances, which crash during initialization (OSError), I think this happens with Info Instances as well, but if that happens I skip them without any logging. The worst part is that, the output of my script changes every time, I mean the amount of crashed values found in the final file change every time the script runs python script.py, and there is also some rule to it, as BAU_4300_ADDON_CAVALORN, PC_HERO, crash more often than the other instances, but this could be a false positive.

Sometimes, but not always, the script also shows this Traceback:

$ python script.py 
Exception ignored in: <function DaedalusScript.__del__ at 0x000002203C8216C0>
Traceback (most recent call last):
  File "...\DecDat-Zenkitpy_project\venv\Lib\site-packages\zenkit\daedalus_script.py", line 277, in __del__
    self._deleter()
  File "...\DecDat-Zenkitpy_project\venv\Lib\site-packages\zenkit\daedalus_vm.py", line 203, in _deleter
    DLL.ZkDaedalusVm_del(self._handle)
OSError: exception: access violation reading 0xFFFFFFFFFFFFFFFF

Sometimes, but not always, when I enable the Debug LogLevel, I get different errors;

2025-01-20 23:33:46 [ZenKit] (ERROR  ) ÔÇ║ DaedalusVm: Internal Exception: illegal access of type 2 on DaedalusSymbol PRINT_KOSTEN which is another type (3)
2025-01-20 23:33:46 [ZenKit] (ERROR  ) ÔÇ║ DaedalusVm: Internal Exception: illegal access of type 2 on DaedalusSymbol PRINT_LP which is another type (3)

there can be also no errors in the Terminal, and crashed values would still be there in the output 😞

Here is the script.zip, it expects an exported _COMPILED/BASE_GOTHIC.DAT next to the script.py. I started off with using Archolos binaries, but the crashes appear also in base Gothic 2 NotR. The script has additions from the PR to make it work in 1.3.0

Any advice how could I debug it? I was thinking of setting externals to lessen the burden of the default skip behaviour in case of missing externals, as this could perhaps improve the state of the stack, but I don't know which external would be most problematic and I don't know if the randomness is even related to the externals or the stack.

@kamilkrzyskow
Copy link
Author

Next day of investigation about the random crashes. I've added some externals and started getting this error

RecursionError: maximum recursion depth exceeded

I don't really understand where the recursion is, but looking at the Traceback it's related to the DaedalusInstance being constantly converted into other proper NpcInstance ItemInstance for each external, so perhaps adding externals makes use of the cb() callback, and somehow there is a chain created 🤔

Exception ignored on calling ctypes callback function <function DaedalusVm._register_external.<locals>.<lambda> at 0x0000015599AF8540>:
Traceback (most recent call last):
  File "...\zenkit\daedalus_vm.py", line 198, in <lambda>
    fptr = DaedalusVmExternalCallback(lambda _, __: cb())
  File "...\zenkit\daedalus_vm.py", line 177, in _wrapper
    vals = [self.pop(arg) for arg in reversed(args)][::-1]
  File "...\zenkit\daedalus_vm.py", line 109, in pop
    return self.pop_instance()
  File "...\zenkit\daedalus_vm.py", line 133, in pop_instance
    return DaedalusInstance.from_native(handle)
  File "...\zenkit\daedalus\base.py", line 60, in from_native
    typ = DaedalusInstanceType(DLL.ZkDaedalusInstance_getType(handle))
RecursionError: maximum recursion depth exceeded
Traceback (most recent call last):
  File "...\script.py", line 178, in <module>
    main()
  File "...\script.py", line 62, in main
    if can_skip_symbol(vm_sym):
  File "...\script.py", line 50, in can_skip_symbol
    return sym.is_member or sym.type not in (DaedalusDataType.INSTANCE, DaedalusDataType.FUNCTION)
  File "...\zenkit\daedalus_script.py", line 180, in type
    return DaedalusDataType(DLL.ZkDaedalusSymbol_getType(self._handle))
RecursionError: maximum recursion depth exceeded

Worst part is that I still get random results, sometimes there is no RecursionError, but I get this corrupted external name Missing externals {'p┬že┬ę├╣\x7f'}, but the name could also contain non-ASCII characters, and my terminal has some issues with UTF-8 characters and Python, so this could be an issue with the encoding in the Terminal 🤔 Still, I doubt that the randomness is related to encoding.

script_with_externals.zip

@lmichaelis
Copy link
Member

Hm I do see a crash with G2 NotR with your first script, so there's probably a bug here. I'll hopefully have time to investigate a bit on the weekend. In the mean time, the only good way to debug this is to add debug prints in the C/C++ source, then recompile the native library, since the crash happens in C-land, inside ZkDaedalusVm_initInstance.

@lmichaelis
Copy link
Member

Okay @kamilkrzyskow, I did find at least one issue which, if fixed, seems to resolve your problem. Before calling init_instance, you should check that the symbol you're trying to initialize is_const, otherwise you'll be trying to initialize function parameters and variables too.

You could, for example, add that check to this:

if class_sym and vm_sym.is_const:
    ...

There is a bug with externals still though, which causes the program to crash when an incorrect parameter is taken. I've fixed that in GothicKit/ZenKitCAPI@2a1b554 and d9fedd0 :)

@kamilkrzyskow
Copy link
Author

Thanks a lot for the fix 🫶 ! Also sorry for this blunder 😞 over the many iterations of random errors I've seen some NAME.SLF, but I didn't give this much thought, as I had the is_member check, but function variables aren't members... If I didn't take this long break, then perhaps I would figure it out on my own, but the errors don't really help 😅

As for other observations I've had from my shenanigans:

  • vm_sym.value for INSTANCEs, will always instantiate them anew, so if somebody isn't careful they might fill up their memory with constant recreation of instances. This kind of happened in this case, as to check if isinstance(info, InfoInstance) I needed the info = vm_sym.value, and later I instantiate the NPC from info.npc, so it's created twice. The game also kind of allows to insert the same NPC twice into the world, so multiple memory allocations is expected, but I still feel this should be somehow addressed to prevent bad coding🤔
  • I tried to add caching to get_parent_as_symbol to limit the amount of indirection and looking for the same index of C_INFO, but I saw no significant improvement. I profiled the main() using time.perf_counter, and in best case scenario it went down from ~2.4s to ~2.3s, but the results varied so much that I consider this as a false positive.
  • Ctrl+C aka KeyboardInterrupt doesn't interrupt the script, and combined with the default logger, which doesn't cache the messages it can happen that a bunch of error messages will fill up the terminal, and there will be no other choice than to wait for the program to finish writing out all of these messages.

So with this, the dialogue and routine (script above needed a small fix) extraction solution is superior to my previous DecDat -> Regex/Text extraction method, and less buggy 🥳

I was thinking about simplifying vm.get_symbol_by_index(info.npc) into info.get_instance_of(attribute: str), however, given the point above about instantiation and memory allocation, then I guess this abstraction could get quite costly 🤔

So I'm getting closer to being happy with this PR, thanks again ✌️

@lmichaelis
Copy link
Member

vm_sym.value for INSTANCEs, will always instantiate them anew, so if somebody isn't careful they might fill up their memory with constant recreation of instances. This kind of happened in this case, as to check if isinstance(info, InfoInstance) I needed the info = vm_sym.value, and later I instantiate the NPC from info.npc, so it's created twice. The game also kind of allows to insert the same NPC twice into the world, so multiple memory allocations is expected, but I still feel this should be somehow addressed to prevent bad coding🤔

I was at first confused but I think I understand now. The way you implemented .value for instances is not actually the way it should be done. I have a patch locally which adds that functionality in the native library, which I can make available. Essentially, you'll get an get_instance() function, analogous to get_{string, int, float}() which will return an already initialized instance bound to that symbol. This would mean that you initialize the instance once and then you can access it directly from the underlying symbol its attached to.

Generally, you should not use the _keepalive member, because in this case especially, it could also be pointing to a DaedalusScript instance, not necessarily a DaedalusVm. Instance instantiation should only be done externally in client code, i.e. not in the library itself.

Ctrl+C aka KeyboardInterrupt doesn't interrupt the script, and combined with the default logger, which doesn't cache the messages it can happen that a bunch of error messages will fill up the terminal, and there will be no other choice than to wait for the program to finish writing out all of these messages.

You can write your own logger if you like. Just use set_logger and give it a level and a callback function like (LogLevel, str, str) -> None.

@kamilkrzyskow
Copy link
Author

which I can make available. Essentially, you'll get an get_instance() function

I like the sound of that, instantiation-aware instances 🤩 I like how you have solutions already in the making to the issues I face 😆

_keepalive member, because in this case especially, it could also be pointing to a DaedalusScript instance, not necessarily a DaedalusVm

That's why there is the protection with hasattr in the PR, I omitted those in the local scripts, as it was only ported over for testing the Vm. Once your patch is available, then DaedalusInstance.get_instance_of(attribute: str), would be feasible.

You can write your own logger if you like

Thanks, I've written one for debugging the vm_sym during an ERROR log, however due to the randomness before it didn't get much use. It lacks the coloring of sections, but I didn't need it.

class Container:
    logger_messages = dict()

def main():

    # Setup logger
    vm_sym_name = "*"
    
    def logger_callback(lvl: LogLevel, name: str, message: str):
        count = Container.logger_messages.get(message, 0)

        if count == 0:
            nonlocal vm_sym_name
            print(lvl.name, name, vm_sym_name, message, sep=" - ")

        Container.logger_messages[message] = count + 1

    set_logger(LogLevel.DEBUG, logger_callback)

    # ...

    for vm_sym in vm.symbols:

        vm_sym_name = vm_sym.name
        
        # ...

@kamilkrzyskow
Copy link
Author

kamilkrzyskow commented Jan 26, 2025

There is a bug with externals still though, which causes the program to crash when an incorrect parameter is taken.

Small progress with the externals, as I use 1 in my next extraction project. My bug came from reading the parameters from the DecDat extraction header comment.

// func void CreateInvItem(var __class par0, var __class par1);
// func void CreateInvItems(var __class par0, var __class par1, var int par2);

Where it says __class I assumed it's the DaedalusInstance, however I noticed on the example page that the itemInstance was of type int. So after checking the source, I've learned that in those functions above the item instance should be also an int, just like the example.

@lmichaelis
Copy link
Member

I've add DaedalusSymbol.get_instance in 85484af. You need to call DaedalusVm.init_instance on the symbol in order to get a value return from get_instance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants