Name uniqueness issues

[hexabits]

Recent nif.xml changes have required a type to vary while the name remains the same. This is typically because the type changes between versions yet the function is identical. Examples include:

HavokColFilter Layer HavokMaterial BSVertexData/BSVertexDataSSE

There are more but these are the most obvious examples. Here is a visual example:

The "Layer" name needs to remain the same name for several reasons:

1. The function does not change.
2. The version/game is irrelevant to the name.
3. The name remaining consistent is better for users, i.e. you do not need to specify which "Layer" specifically as it is always just "Layer".
4. Functionality in programs such as NifSkope rely on the name and changing the name will break the functionality.

The issue is that C++ is strongly typed and so niflib has a problem with its codegen. Refer to #62. Relevant discussion: https://github.com/niftools/nifxml/pull/62#pullrequestreview-43833411

So far I have suggested that gen_niflib.py will need to change in some way to accommodate this requirement. Either it would fix it internally by creating unique names automatically, or we could change nif.xml to accommodate niflib's codegen. So far I have suggested something like so:

Proposal

An attribute which, when present, gen_niflib.py will use instead of name.

        <add name="Layer" uniquename="Layer_OB" type="OblivionLayer" ... />
        <add name="Layer" uniquename="Layer_FO" type="Fallout3Layer" ... />
        <add name="Layer" uniquename="Layer_SK" type="SkyrimLayer" ... />

The exact string used for the attribute is up for discussion, though I think it is likely the best choice. uniqueid would imply UUID, and niflibname is accurate but project-specific. A generic uniquename thus seems like the best option.

Updated Proposal

        <add name="Layer" suffix="OB" type="OblivionLayer" ... />
        <add name="Layer" suffix="FO" type="Fallout3Layer" ... />
        <add name="Layer" suffix="SK" type="SkyrimLayer" ... />

In niflib this would become:

OblivionLayer layer_ob;
Fallout3Layer layer_fo;
SkyrimLayer layer_sk;

[hexabits]

Tagging @aperture-it @neomonkeus for input. Not only do we need to agree on the changes to nif.xml but somebody needs to be willing to update nifdocsys (gen_niflib.py).

[aperture-it]

Yes, the version with a uniquename is good. I already fixed gen_niflib.py and compiled the project as a test.

[aperture-it]

I added a print operator to gen_niflib.py to check for duplicates with different types, and that's what it gave out

Name block >> name dublicate element

BSVertexData >> Vertex
BSVertexData >> Bitangent X
HavokColFilter >> Layer
HavokColFilter >> Layer
HavokMaterial >> Material
HavokMaterial >> Material
bhkRigidBody >> Body Flags
NiAVObject >> Flags
NiGeometry >> Data
NiGeometry >> Skin Instance
BSLightingShaderProperty >> Shader Flags 1
BSLightingShaderProperty >> Shader Flags 2
BSEffectShaderProperty >> Shader Flags 1
BSEffectShaderProperty >> Shader Flags 2
BSTriShape >> Num Triangles
BSTriShape >> Vertex Data
BSSubIndexTriShape >> Segment

[hexabits]

Hmm. Well Vertex and Bitangent X being different names are going to be annoying for niflib, though I guess figment somehow already did it... Does he just treat BSVertexData as a completely internal type (i.e. niflibtype attribute in the <compound>)? You may want to treat BSVertexData completely internally like Vector3, Vector4, etc.

[aperture-it]

In my niflib there is only one BSVertexData. And He contains two types of vertices.

/*! Unknown. */
HalfVector3 vertex;
/*! Unknown. */
Vector3 vertex2;

When is set or get data, only Vector3 is used. A HalfVector3 is used only when writing or reading a nif file.

void BSVertexData::SetVertex(const Vector3& v)
{
    vertex.x = ConvertFloatToHFloat(v.x);
    vertex.y = ConvertFloatToHFloat(v.y);
    vertex.z = ConvertFloatToHFloat(v.z);

    vertex2.x = v.x;
    vertex2.y = v.y;
    vertex2.z = v.z;
}

Vector3 BSVertexData::GetVertex() const
{
    return vertex2;
}

I think that you can move away from using BSVertexDataSSE

[hexabits]

It will be no different though. Before BSVertexDataSSE split there were three vertex rows: https://github.com/niftools/nifxml/pull/60/commits/6a5c86654f18de22164083055e85def940b7ac2b#diff-b40bc0336ce54aa4b9bc9ffd65a03ea3R1490

Due to the cond differences (SSE is always full precision) there had to be three vertex types.

[aperture-it]

Why three? Two types, Vector3 and HalfVector3. In the case of a SSE, a vector will be written into the nif, in the case of a FO4 - halfvector. However, for internal calculations of the plugin, a vector will always be used.

[hexabits]

Anyway here is the full correct unified BSVertexData:

<compound name="BSVertexData">
    <add name="Vertex" type="Vector3" userver2="100" cond="((ARG &amp; 16) != 0)" />
    <add name="Bitangent X" type="float" userver2="100" cond="((ARG &amp; 256) != 0)" />
    <add name="Unknown Int" type="int" userver2="100" cond="(ARG &amp; 256) == 0" />

    <add name="Vertex" type="HalfVector3" userver2="130" cond="((ARG &amp; 16) != 0) &amp;&amp; ((ARG &amp; 16384) == 0)" />
    <add name="Bitangent X" type="hfloat" userver2="130" cond="((ARG &amp; 256) != 0) &amp;&amp; ((ARG &amp; 16384) == 0)" />
    <add name="Unknown Short" type="ushort" userver2="130" cond="((ARG &amp; 256) == 0) &amp;&amp; ((ARG &amp; 16384) == 0)" />

    <add name="Vertex" type="Vector3" userver2="130" cond="((ARG &amp; 16) != 0) &amp;&amp; ((ARG &amp; 16384) != 0)" />
    <add name="Bitangent X" type="float" userver2="130" cond="((ARG &amp; 256) != 0) &amp;&amp; ((ARG &amp; 16384) != 0)" />
    <add name="Unknown Int" type="uint" userver2="130" cond="((ARG &amp; 256) == 0) &amp;&amp; ((ARG &amp; 16384) != 0)" />

    <add name="UV" type="HalfTexCoord" cond="(((ARG &amp; 16) != 0) &amp;&amp; ((ARG &amp; 32) != 0))" />
    <add name="Normal" type="ByteVector3" cond="(ARG &amp; 128) != 0" />
    <add name="Bitangent Y" type="byte" cond="(ARG &amp; 128) != 0" />
    <add name="Tangent" type="ByteVector3" cond="((ARG &amp; 128) != 0) &amp;&amp; ((ARG &amp; 256) != 0)" />
    <add name="Bitangent Z" type="byte" cond="((ARG &amp; 128) != 0) &amp;&amp; ((ARG &amp; 256) != 0)" />
    <add name="Vertex Colors" type="ByteColor4" cond="(ARG &amp; 512) != 0" />
    <add name="Bone Weights" type="hfloat" arr1="4" cond="(ARG &amp; 1024) != 0" />
    <add name="Bone Indices" type="byte" arr1="4" cond="(ARG &amp; 1024) != 0" />
    <add name="Unknown Int 2" type="uint" cond="(ARG &amp; 4096) != 0" />
</compound>

You're still going to 3 vertex variables and 3 bitangent X variables. If that is sufficient I suppose I will go back to the unified version. There is no way to only do it with two as I've already illustrated.

[hexabits]

I guess I will clarify that there is a way it's just really complicated and ugly. I could make extremely complicated cond to use just two rows, but then this becomes slower and more difficult to parse for other programs.

[hexabits]

Here is if I make the extremely complicated and unreadable condition to combine the three vertex blocks into two:

<compound name="BSVertexData">
    <add name="Vertex" type="HalfVector3" userver2="130" cond="((ARG &amp; 16) != 0) &amp;&amp; ((ARG &amp; 16384) == 0)" />
    <add name="Bitangent X" type="hfloat" userver2="130" cond="((ARG &amp; 256) != 0) &amp;&amp; ((ARG &amp; 16384) == 0)" />
    <add name="Unknown Short" type="ushort" userver2="130" cond="((ARG &amp; 256) == 0) &amp;&amp; ((ARG &amp; 16384) == 0)" />

    <add name="Vertex" type="Vector3" cond="((User Version 2 == 100) &amp;&amp; ((ARG &amp; 16) != 0)) || ((User Version 2 == 130) &amp;&amp; ((ARG &amp; 16) != 0) &amp;&amp; ((ARG &amp; 16384) != 0))" />
    <add name="Bitangent X" type="float" cond="((User Version 2 == 100) &amp;&amp; ((ARG &amp; 256) != 0)) || ((User Version 2 == 130) &amp;&amp; ((ARG &amp; 256) != 0) &amp;&amp; ((ARG &amp; 16384) != 0))" />
    <add name="Unknown Int" type="uint" cond="((User Version 2 == 100) &amp;&amp; ((ARG &amp; 256) == 0)) || ((User Version 2 == 130) &amp;&amp; ((ARG &amp; 256) == 0) &amp;&amp; ((ARG &amp; 16384) != 0))" />\

    <add name="UV" type="HalfTexCoord" cond="(((ARG &amp; 16) != 0) &amp;&amp; ((ARG &amp; 32) != 0))" />
    <add name="Normal" type="ByteVector3" cond="(ARG &amp; 128) != 0" />
    <add name="Bitangent Y" type="byte" cond="(ARG &amp; 128) != 0" />
    <add name="Tangent" type="ByteVector3" cond="((ARG &amp; 128) != 0) &amp;&amp; ((ARG &amp; 256) != 0)" />
    <add name="Bitangent Z" type="byte" cond="((ARG &amp; 128) != 0) &amp;&amp; ((ARG &amp; 256) != 0)" />
    <add name="Vertex Colors" type="ByteColor4" cond="(ARG &amp; 512) != 0" />
    <add name="Bone Weights" type="hfloat" arr1="4" cond="(ARG &amp; 1024) != 0" />
    <add name="Bone Indices" type="byte" arr1="4" cond="(ARG &amp; 1024) != 0" />
    <add name="Unknown Int 2" type="uint" cond="(ARG &amp; 4096) != 0" />
</compound>

This is what I was trying to avoid as no one can read this and it's more difficult to parse. I'm also only 90% sure I converted it correctly.

((User Version 2 == 100) && ((ARG & 16) != 0)) || ((User Version 2 == 130) && ((ARG & 16) != 0) && ((ARG & 16384) != 0))

Is just ridiculous, which I think everyone can agree on. I suppose that it could be simplified somewhat to be more readable, but this is the most correct, expanded version of the expression.

Edit: It turns out that can't even use version conditions inside of cond so it's impossible and has to be three separate rows.

[aperture-it]

Let there be three types in nifxml. Niflib has no conditions, and will contain only two.

[aperture-it]

One of the Vector3 will not have a uniquename, and will be skipped during building.

[hexabits]

Well you need to assure that somehow for SSE that it never writes a HalfVector3 to file for BSTriShape regardless of what the VF_Full_Precision or Float Size say, since SSE does not do half precision. Don't the conditions in the XML change the read/write functions?

[aperture-it]

At the moment, the plugin does not write half precision in the SSE

[aperture-it]

if (IsFallout4(info))
{
    if (!(vertexFlags[6] & 64))
    {
        NifStream( vertexData[i2].vertex.x, in, info );
        NifStream( vertexData[i2].vertex.y, in, info );
        NifStream( vertexData[i2].vertex.z, in, info );

        if ((vertexFlags[5] & 128) && (vertexFlags[2] & 64))
        {
            NifStream( vertexData[i2].bitangentX, in, info );
        }
        if (!(vertexFlags[5] & 128) && (vertexFlags[2] & 64))
        {
            NifStream( vertexData[i2].unknownShort1, in, info );
        }
    }
    else
    {
        NifStream( vertexData[i2].vertex2, in, info );
        NifStream( vertexData[i2].bitangentX2, in, info );
    }
}
else if (IsSSE(info) && (vertexFlags[5] & 16))
{
    NifStream( vertexData[i2].vertex2, in, info );
    if (vertexFlags[5] & 128)
    {
        NifStream( vertexData[i2].bitangentX2, in, info );
    }
    else
    {
        NifStream( vertexData[i2].unknownInt1, in, info );
    }
}

[aperture-it]

Yes, the conditions in the XML change the read/write functions. But the resulting code is not at all optimal, so in this case it makes sense to write the custom code.

[aperture-it]

We can use a non unique name, but a suffix that will be added to the original name. As a result, the in niflib will more clearly understand that these are branch variables.

<add name="Vertex Data" suffix="FO4" type="BSVertexData" arr1="Num Vertices" ...
<add name="Vertex Data" suffix="SSE" type="BSVertexDataSSE" arr1="Num Vertices" ...

/*! Unknown. */
vector<BSVertexData > vertexData_FO4;
/*! Unknown. */
vector<BSVertexDataSSE > vertexData_SSE;

[hexabits]

It is less repetition I suppose. Though for BSVertexData/BSVertexDataSSE I guess I will be re-unifying the two compounds if it is easier for niflib. But other examples would include:

<compound name="HavokMaterial">
    <add name="Material" suffix="OB" type="OblivionHavokMaterial" ver1="20.0.0.4" ver2="20.0.0.5">The material of the shape.</add>
    <add name="Material" suffix="FO" type="Fallout3HavokMaterial" vercond="(Version == 20.2.0.7) &amp;&amp; (User Version 2 &lt;= 34)">The material of the shape.</add>
    <add name="Material" suffix="SK" type="SkyrimHavokMaterial" vercond="(Version == 20.2.0.7) &amp;&amp; (User Version 2 &gt; 34)">The material of the shape.</add>
</compound>

vs

<compound name="HavokMaterial">
    <add name="Material" uniquename="Material_OB" type="OblivionHavokMaterial" ver1="20.0.0.4" ver2="20.0.0.5">The material of the shape.</add>
    <add name="Material" uniquename="Material_FO" type="Fallout3HavokMaterial" vercond="(Version == 20.2.0.7) &amp;&amp; (User Version 2 &lt;= 34)">The material of the shape.</add>
    <add name="Material" uniquename="Material_SK" type="SkyrimHavokMaterial" vercond="(Version == 20.2.0.7) &amp;&amp; (User Version 2 &gt; 34)">The material of the shape.</add>
</compound>

[aperture-it]

There is another problem. In nifxml.py there is no parsing by userver2, because of this the conditions are incorrectly constructed.

[hexabits]

Unfortunately I was correct and there is a massive slowdown when merging BSVertexDataSSE back into BSVertexData. Scanning all SSE NIFs went from 112s to 180s, or 1.6x slower. However combining them will prevent a lot of code duplication in niflib so I have no choice.

Also.. update on modifying the conditions to change the three vertex rows to two... It's not possible whatsoever as User Version 2 cannot be used inside cond attribute. So:

        <add name="Vertex" type="Vector3" userver2="100" cond="((ARG &amp; 16) != 0)" />
        <add name="Bitangent X" type="float" userver2="100" cond="((ARG &amp; 256) != 0)" />
        <add name="Unknown Int" type="uint" userver2="100" cond="(ARG &amp; 256) == 0" />

        <add name="Vertex" type="HalfVector3" userver2="130" cond="((ARG &amp; 16) != 0) &amp;&amp; ((ARG &amp; 16384) == 0)" />
        <add name="Bitangent X" type="hfloat" userver2="130" cond="((ARG &amp; 256) != 0) &amp;&amp; ((ARG &amp; 16384) == 0)" />
        <add name="Unknown Short" type="ushort" userver2="130" cond="((ARG &amp; 256) == 0) &amp;&amp; ((ARG &amp; 16384) == 0)" />

        <add name="Vertex" type="Vector3" userver2="130" cond="((ARG &amp; 16) != 0) &amp;&amp; ((ARG &amp; 16384) != 0)" />
        <add name="Bitangent X" type="float" userver2="130" cond="((ARG &amp; 256) != 0) &amp;&amp; ((ARG &amp; 16384) != 0)" />
        <add name="Unknown Int" type="uint" userver2="130" cond="((ARG &amp; 256) == 0) &amp;&amp; ((ARG &amp; 16384) != 0)" />

Is the best that can be done inside of one BSVertexData compound while still being correct.

@aperture-it userver2="N" is just expanded to vercond="User Version 2 == N"

[aperture-it]

The userver2 attribute does not appear in the python scripts. We need to add it for use, because we have the wrong function construction. I'm not talking about the attribute cond.

[hexabits]

I didn't say anything about cond to you. I said that userver2 is a shortcut for vercond. vercond != cond. I realize that userver2 does not appear. I was telling you exactly how to implement it, by just expanding it to a string for the vercond.

The stuff above my @ mention was not directed at you.