OpenNaja / cobra-tools

A suite of GUI tools for extracting and modifying OVL and OVS archives, as well as editing the associated in-house file formats. Also includes a model plugin for Blender. For downloads, guides, and FAQs visit:
https://opennaja.github.io/cobra-tools/
GNU General Public License v3.0
98 stars 26 forks source link

Recursive fix #313

Closed Candoran2 closed 1 year ago

Candoran2 commented 1 year ago

Implemented possibility for recursive fields in the codegen. This is to allow something like https://github.com/niftools/nifskope/issues/185 .Some things had to be reworked for this.

Done

  1. When a field is recursive, its type is no longer simply imported (from module x import y), instead, it is accessed through the import map stored on the type.
  2. The attribute list is no longer directly stored, instead it can be returned as an iterator from the _get_attribute_list class function. For compatibility and to refrain from calling a function every time you need the attribute list, it is immediately stored there, though, resulting in seemingly unchanged functionality. It is also a tuple rather than a list, to reflect the immutability.
  3. The dynamically generated functions are now added not through the StructMetaClass, but through a call to the init_attributes class function. This includes the aforementioned storing of the attribute list.

These were all necessary to prevent circular imports and trying to access classes before they existed in their module namespace when encountering recursive fields.

Points of discussion

How to determine which fields are recursive. Currently, it uses the recursive="true" indicator on the field. However, in my view there are three possibilities:

  1. Keep it as-is.
  2. Use the order of the types in the xml - any field type not defined before it is used is assumed to be recursive. Nif.xml needs no changes for this, but many of the existing ones in cobra-tools do.
  3. A combination of both: it is only allowed to use types in field before it is defined in the xml if that field has recursive="true".

While I don't like introducing new attributes, this one if probably preferable over keeping a purely order-based, since it's more obvious about what's going on to the casual reader of the xml. On the other hand, I do like the idea of enforcing a certain order. Even though it's not explicitly mentioned in the description of the nif xml, I could imagine it might make parsing it easier for whatever generator uses it if that is an established, reliable standard.