HENDRIX-ZT2 commented 4 years ago

@niftools/pyffi-reviewer

Overview

refactoring of XML parsing: done, now using etree. Could do with more code cleanup, but much easier to tell what's going on now
support for old nif.xml: probably still fine
support for latest nif.xml: WIP

Fixes Known Issues

n/a

Documentation

n/a

Testing

n/a

Manual

Blender plugin seems to work just fine with old nif.xml. New nif.xml has kinks to work out, but the basics are there.

Automated

n/a

Additional Information

n/a

coveralls commented 4 years ago

Coverage decreased (-0.03%) to 57.798% when pulling 488b9ff5f3b97dac43cd62aa5db00884ba33a863 on HENDRIX-ZT2:develop into 3bb308a5797471ee06b96a003599a635b5b943c4 on niftools:develop.

coveralls commented 4 years ago

Coverage decreased (-0.06%) to 57.762% when pulling 596471150c01a1cbba2b1e6aaefd189e1f834271 on HENDRIX-ZT2:develop into 3bb308a5797471ee06b96a003599a635b5b943c4 on niftools:develop.

hexabits commented 4 years ago

The other day @neomonkeus asked me for my expression parser that I use for my nifxml linting and xml/docs/codegen but it's seeming like that isn't necessary anymore since you appear to be both capable and willing to deal with PyFFI.

I was so put off by PyFFI I instead opted to create a flat, statically typed Python lib with my codegen that can read everything NifSkope can (more, weirdly, due to NifSkope bugs) and isn't a complete nightmare to write for and debug since it's not relying 99.9% on dynamic language features. Using Python features like dataclasses and Enum/Flag (or IntEnum/IntFlag for less strictness) also leaves the generated code extremely clean and navigable (though I can optionally generate non-dataclass versions). The enums let me catch dozens of missing things from the XML, like 15 undocumented Havok materials and maybe a dozen other enumeration values spread across random games, since I could configure the Enum class's _missing_.

Creating the entire lib and incrementally fixing codegen to read all the files nifxml supports only took a couple days too i.e. far less than the time to figure out how to force PyFFI to adjust to the new nifxml... which to me says something about how dense and esoteric PyFFI is. I've browsed through your commits and unfortunately PyFFI remains... difficult for me so I can't yet comment on the correctness of anything you've implemented from nifxml.

There are actually some nifxml changes which were meant to go into my PR that neo merged without warning, one which I still haven't settled on. Specifically arg="Vertex Desc #RSH# 44" is apparently not sufficient for all BSVertexData like previously thought. There are two NIFs in FO76 that have a UV position/offset that aren't 8/16 byte aligned and the vertex has 2 more elements meaning it's not a Vector3 or HalfVector3 but instead a ... Vector5. It's a really weird edge case for sure, but it means we should pass both the flags and at least the vertex size / UV offset (same thing) meaning at least two args, meaning some kind of system for an arbitrary amount of args. Such a syntax has been knocked around before but we never implemented it. Passing in the full unshifted Vertex Desc is an option but it also incurs the arithmetic overhead of repeated shifting and masking for each field. BSVertexData parsing in NifSkope is already an order of magnitude slower than NiTriShape meshes and this would just make it even worse.

Speaking of non-NiTriShape meshes, since you're willing to deal with PyFFI you should really try getting NIFs beyond 20.3 fully supported. Starting in 20.5 they deprecated NiTriShape etc. and switched to NiMesh. Though I can't tell what is needed in PyFFI itself vs the Blender plugin. PyFFI seems to split the RTTI args for NiDataStream on \x01 already so I would guess all the necessary work belongs in the Blender plugin. Though considering you have ZT2 in your name, I'm guessing your motivations for working on this stop and end at ZT2. Which is perfectly fine if so.

The second upcoming XML change is a "stop condition" for object/field serialization. This is required in FO76 for shader properties which effectively stop serialization if the Name field on NiObjectNET/NiProperty is not empty. I opted to add it as an attribute to <niobject> and it acts kind of like a cond expression for every field in the object. Only superficially though as applying a cond to every row is extremely inefficient since stopcond is just simple short-circuiting logic.

For this particular stop condition I haven't settled on the exact expression because there are issues with "truthiness" of strings in languages other than Python. Most simply it can be

stopcond="#BSVER# #GTE# 155 #AND# Name"

which results in my codegen as:

    def read(self, stream: BinaryStream, args: Optional[List[int]]=None):
        super().read(stream)
        if stream.bs_version >= 155 and self.name:
            return
        # Rest of function

More rigorously it should be:

stopcond="#BSVER# #GTE# 155 #AND# Name #NEQ# #EMPTY#"

Where #EMPTY# is a token to represent "" or "". Note that grammatically it's nothing like cond which I previously likened it to, since cond cannot have version tokens (except in the Header struct because the fields are local). And it's not like vercond because it can reference local/inherited fields. So support for stopcond would not work by aliasing / shoehorning it into existing constructs.

At the moment I'm not sure about the state of string literal support in any of the expression parsers, incl. my own and NifSkope's. I would probably have to add grammars for it in mine at least. I would prefer the "truthy string" method but then C++ code generation would require some special casing as std::string etc do not implicitly or explicitly cast to bool.

As for attributes which haven't been implemented in PyFFI yet, I'll try to go over my nifxml commits and pick out the ones that are actually important. Attributes such as range are merely for data validation, not serialization (though it could validate on read/write if one chooses to).

I've run out of wind for now but my next comment will be about PyFFI and field name/type "collisions" which are technically not collisions and if that is finally fully addressed yet. But it will take me a while to come up with the cases and how I deal with them statically.

neomonkeus commented 4 years ago

I merged onto the PR into develop to get a baseline for us to work with. If there if any additional work locally feel free to PR towards develop.

I added you as a reviewer both here and on the nif.xml PR from develop to master in case there was indeed any additional work that you thought should be included, such as the above. Maybe we should talk it over there, but good to have an insight into what is left out.

Having a full working implementation of your parse would still be a great reference.

HENDRIX-ZT2 commented 4 years ago

Hi Jon! I haven't implemented all of your new nif.xml features; more exotic stuff like the Hex to Float comparison are missing for now. The main work I have done so far is making the XML parser more readable and robust. So now it can parse both the old format and the new format. The classes it generates from your new nif.xml are not completely usable.

One strange pyffi bug needs further investigation before I can continue updating to your new xml specs completely: niobjects or compounds with fields of the same name but mutually exclusive verconds cause trouble. Specifically, this instance:

  <niobject name="NiAVObject" abstract="true" inherit="NiObjectNET" module="NiMain">
        Abstract audio-visual base class from which all of Gamebryo's scene graph objects inherit.
        <field name="Flags" type="uint" default="0x8000E" vercond="#BSVER# #GT# 26">snip</field>
        <field name="Flags" type="ushort" ver1="3.0" vercond="#BSVER# #LTE# 26">Basic flags for AV objects.</field>

I haven't been able to tell what is going wrong there. Parsing of tokens and expressions works fine. When either one or the other field is commented out, Nifs of the remaining type parse fine. It really appears to be down to the doubled field name, but the whole pyffi system is so obfuscated that I haven't managed to pinpoint the actual source of the issue.

I would also appreciate your parsing library for reference. It seems like it could be a very helpful resource for upgrading pyffi.

TagnumElite commented 4 years ago

From what I see, there are a few problems with mutiple same-named attributes.

First when you read/write a struct, you go through a filtered list. This filtered list doesn't return same named attributes.

Next in _MetaStructBase when assigning all the attributes it just overrides the old one with the new one.

I believe there was one other problem but it isn't coming to mind right now.

hexabits commented 4 years ago

niobjects or compounds with fields of the same name but mutually exclusive verconds cause trouble

This is almost verbatim how I've referred to the problem case previously so maybe you've already caught some of the previous discussion in tickets and commits, but in case not I'll try to cover all the previous ground. It's the issue I referred to at the end of my last comment that I ran out of wind to cover. :)

I decided to explicitly allow these cases because often fields are inevitably the same but change position (or expand/narrow in type) between versions. I first implemented the convertible attribute to cover what types can "fit" into other types, i.e. ushort is convertible to uint. Ideally, as long as all "duplicate" fields are mutually exclusive (works for mutually exclusive cond too, FYI) and can all be converted to one type, the definition is valid. I'm not currently actually linting for this though, as Python, NifSkope and even modern C++ (through std::variant) can handle type unioning.

When the types are not integral, I implemented a suffix attribute for when a particular parser really needs to deal with the fields as separate attributes. I do this for structs and enums such as the bhkRigidBodyCInfo group of structs and a lot of the enums between Ob-FO3-Skyrim-FO4. Enums could technically be unioned based on their aliased type but both Python and modern C++ have scoped enumerations that are not implicitly assignable/comparable and so I added suffix for when one needs/wants to treat them strictly.

So, in a statically typed codegen situation, dealing with this is actually extremely trivial. You form the member type out of a union of the duplicate types, and for the serialization types you refer to the original field type. The member type union may simplify to one type if all types are convertible, in which case you pick the largest type. Statically, in Python this looks like:

@dataclass
class NiAVObject(NiObjectNET):
    flags: uint = 524302
    # other fields
    def read(self, stream: BinaryStream, args: Optional[List[int]]=None):
        super().read(stream)
        if stream.bs_version > 26:
            self.flags = stream.read_uint()
        if stream.version >= 0x3000000 and stream.bs_version <= 26:
            self.flags = stream.read_ushort()
        # rest of serialization

Here are currently all the instances in the XML for integral unions:

Class	Field Name(s)
BSVertexData	Unused W
InterpBlendItem	Priority
ExtraMeshDataEpicMickey	Num Mapped Primitives
bhkRigidBody	Body Flags
NiBlendInterpolator	Array Size, Interp Count, Single Index, High Priority, Next High Priority
NiAVObject	Flags
NiParticlesData	Num Subtexture Offsets
NiPixelFormat	Bits Per Pixel
BSTriShape	Num Triangles

For some of these names there are more than two fields. And for non-integral unions there is:

BSVertexData

Vertex Union['HalfVector3', 'Vector3']
Bitangent X Union['hfloat', 'float'] NiStencilProperty
Flags Union['StencilFlags', 'ushort'] NiTexturingProperty
Flags Union['ushort', 'TexturingFlags'] NiPhysXSceneDesc
Broad Phase Type Union['NxBroadPhaseType', 'NiSceneDescNxBroadPhaseType'] BSLightingShaderProperty
Skin Tint Color Union['Color3', 'Color4']

...And also Vertex Data on BSTriShape, but in mypy, List[] is invariant so currently I'm not generating valid static types for lists of unioned types. :(

Also note that for Vertex and Bitangent X there should be no explicit Union as I have defined hfloat to be convertible to float and the same for HalfVector3 to Vector3 as when you read these data types you are converting immediately to float and then storing the float (and if you're not currently in PyFFI you should be!). I simply haven't integrated convertible well into my xml parsing and linting yet as I previously mentioned.

The rest of the explicit Union[] I'm actually quite unhappy with. For the flags bitfields StencilFlags and TexturingFlags their underlying types are still ushort but there are certain, mostly legacy NIF versions that specifically do not have the same bitfield definitions and it would be incorrect to use a bitfield for those values too.

SO... If I were to guess what the issue is in PyFFI, I would say that you're missing the distinction between member type and serialization type in some way. Either the first or the last field that gets generated dynamically overwrites what the serialization type should be, and you end up reading the wrong size. Of course you're also not using static typing, so I'm not sure "member type" is exactly a thing in your case. You may be only storing one read type, etc.

In my static lib example this would look like:

@dataclass
class NiAVObject(NiObjectNET):
    flags: uint = 524302
    # other fields
    def read(self, stream: BinaryStream, args: Optional[List[int]]=None):
        super().read(stream)
        if stream.bs_version > 26:
            self.flags = stream.read_uint()
        if stream.version >= 0x3000000 and stream.bs_version <= 26:
            self.flags = stream.read_uint()  # INCORRECT, SHOULD BE read_ushort
        # rest of serialization

There is definitely no going back from this type unioning, because pushing minor type/version/condition differences all the way to the fully qualified name that you use at runtime (and in API and other libs..) is rather severe and unmaintainable.

However, whether we class this as a PyFFI bug or possibly needing more specification in nifxml I guess is now up for discussion. At the moment though I can't clearly define any scenarios where more explicit attributes would actually help PyFFI parse these semantics aside from mere convenience.

Even in my parsing I have to go over the members once, collect the types by unique name, and then run functions on the types to see which type the member ends up being. During serialize, I pretend the fields were never unioned into the same member. I suppose this data could be baked into the XML but I don't see it helping PyFFI's issue.

(P.S. I wrote all this before Tagnum replied so I haven't checked out his comment yet)

HENDRIX-ZT2 commented 4 years ago

Thanks!

First when you read/write a struct, you go through a filtered list. This filtered list doesn't return same named attributes.

At least this one I had suspected too and changed, but then rolled it back because something somewhere was breaking tests (not necessarily this change). Looking at it now I'm wondering if it's even a problem: Same name is the last condition attr has to go through and by then a bad attr should have been filtered out by the vercond checks.

Next in _MetaStructBase when assigning all the attributes it just overrides the old one with the new one.

This seems to be the core problem. Not sure how we can fix that. Maybe we should go over the filtered list instead of _attrs, but is that available at that point?

HENDRIX-ZT2 commented 4 years ago

Another issue here in StructBase, it also avoids dupes and if it didn't it would override previous attributes:

# initialize attributes
for attr in self._attribute_list:
     ...
     # assign attribute value
     setattr(self, "_%s_value_" % attr.name, attr_instance)

https://github.com/niftools/pyffi/blob/develop/pyffi/object_models/xml/struct_.py#L294

The problem is, we need the data parameter to get the filtered attribute list and we don't have that on init. We could create just the attr_instance in init and do setattr(self, "_%s_value_" % attr.name, attr_instance) only in read or write methods where we have the data parameter, so we know which attributes we need for that particular version or conditions in general.

HENDRIX-ZT2 commented 4 years ago

niobjects or compounds with fields of the same name but mutually exclusive verconds cause trouble

This is almost verbatim how I've referred to the problem case previously so maybe you've already caught some of the previous discussion in tickets and commits, but in case not I'll try to cover all the previous ground. It's the issue I referred to at the end of my last comment that I ran out of wind to cover. :)

In fact I did not find that discussion haha, I just read your issues for the formalization and comments on your nifxml commits.

I decided to explicitly allow these cases because often fields are inevitably the same but change position (or expand/narrow in type) between versions.

I completely agree.

So, in a statically typed codegen situation, dealing with this is actually extremely trivial.

Very elegant, makes me wonder why pyffi has historically evolved as a dynamic system rather than a simple static codegen...

SO... If I were to guess what the issue is in PyFFI, I would say that you're missing the distinction between member type and serialization type in some way. Either the first or the last field that gets generated dynamically overwrites what the serialization type should be, and you end up reading the wrong size.

That seems to be pretty much what's happening. Init overwrites attributes and read / write don't have access to the ones they need.

However, whether we class this as a PyFFI bug or possibly needing more specification in nifxml I guess is now up for discussion.

It is most definitely a pyffi bug.

Even in my parsing I have to go over the members once, collect the types by unique name, and then run functions on the types to see which type the member ends up being. During serialize, I pretend the fields were never unioned into the same member. I suppose this data could be baked into the XML but I don't see it helping PyFFI's issue.

I don't think we need any additional data in the nifxml. Pyffi should be able to deal with these duped fields with a slight recode.

TagnumElite commented 4 years ago

I have a few ideas, whether or not they will work is another thing. In relation to the blender_nif_plugin. I wouldn't mind getting rid of PyFFI altogether because of all the ~bloat~ extra features we don't need. Like qskope and the other formats. A statically typed nifxml lib could work quite nicely though.

Now for pyffi, non-duplicate struct attributes just we just written but any duplicate structs could be processed at read/write.

HENDRIX-ZT2 commented 4 years ago

I have a few ideas, whether or not they will work is another thing. In relation to the blender_nif_plugin. I wouldn't mind getting rid of PyFFI altogether because of all the ~bloat~ extra features we don't need. Like qskope and the other formats. A statically typed nifxml lib could work quite nicely though.

I wouldn't be opposed to using a static codegen instead of pyffi for the blender plugin, but I'm sure that would entail a lot of recoding work to make up for functions that had been delegated to pyffi. That is, of course, if Jon is willing to share his codegen.

In the long run the whole nif project would surely benefit from using just one nif XML parsing solution.

hexabits commented 4 years ago

BTW at least as of right now I have done this for the strict float comparison (as hex bytes):

Added to the tokens for cond:

<condexpr token="#FLT_MAX#" string="0x7F7FFFFF" />

...and in the actual cond:

cond="Rimlight Power == #FLT_MAX#"

Python and floats are... weird, and not really my area of expertise, but what I found works for my static lib prototype is replacing the #FLT_MAX# token with

float.fromhex('0x1.fffffe0000000p+127')

Which this feels... weird as I said before. I am unsure if this value is platform specific or even binary specific. I am using 32-bit 3.7.4 in this example. This is what FLT_MAX read from the NIF files reported as when turned into hex, at least on said Python version. I have the sneaking suspicion the value could be different on 64-bit Python so it doesn't seem like a very portable method. In any case, 0x7F7FFFFF was definitely NOT the correct float value for Python.

At least as a token now you can more cleanly deal with it.

HENDRIX-ZT2 commented 4 years ago

@jonwd7 I think the struct module could be better suited for this. Not sure how fromhex handles endianness and stuff. Makes sense to put it as a token, I like that!

hexabits commented 4 years ago

Endianness doesn't really factor in here as the endianness varies only on the left side of the expression X == #FLT_MAX#. At this stage, X would already be read in as either big or little endian and it's being compared to a static constant--in this case float.fromhex('0x1.fffffe0000000p+127'). So it would only be incorrect if X were not read correctly before the comparison.

I went ahead and looked into it and the Python docs were actually helpful:

aaaaaaaaaaa

And as I assumed the reason the hexadecimal value does not match true flt_max (7f7fffff) is that Python float is actually a double.

HENDRIX-ZT2 commented 4 years ago

Alright, I think I solved the union / serialization issue, at least for reading. Taking baby steps here 🤣

niftools / pyffi

XML parser refactoring in preparation for latest nif.xml #62

Overview

Fixes Known Issues

Documentation

Testing

Manual

Automated

Additional Information