Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Updates for niflib #62

Merged
merged 11 commits into from
Jun 27, 2017
Merged

Updates for niflib #62

merged 11 commits into from
Jun 27, 2017

Conversation

aperture-it
Copy link
Contributor

Incorrect compile with gen_niflib.py

Incorrect compile with gen_niflib.py
Override of enumerators
Override of enumerators
"Anim Note Arrays" size is "Num Anim Note Arrays" ???
Override Segment
nif.xml Outdated
<add name="Sub Segment Data" type="BSSITSSubSegmentRecord" userver2="130" cond="(Num Segments &lt; Total Segments) &amp;&amp; (Data Size &gt; 0)" />
<add name="Num Segments" type="uint" userver2="100" />
<add name="Segment" type="BSGeometrySegmentData" userver2="100" arr1="Num Segments" />
<add name="SSE_Segment" type="BSGeometrySegmentData" userver2="100" arr1="Num Segments" />
Copy link
Member

@hexabits hexabits Jun 13, 2017

Choose a reason for hiding this comment

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

Niflib needs to be able to handle the same name but with different types when they are exclusive. These are not "overrides" as they are never used at the same time due to version conditions. What is the actual problem with naming them both "Segment"?

Copy link
Member

@hexabits hexabits Jun 13, 2017

Choose a reason for hiding this comment

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

For example, how is this not also a problem then:

<add name="Vertex Data" type="BSVertexData" arr1="Num Vertices" arg="VF" cond="Data Size &gt; 0" userver2="130" />
<add name="Vertex Data" type="BSVertexDataSSE" arr1="Num Vertices" arg="VF" cond="Data Size &gt; 0" userver2="100" />

They are mutually exclusive and also need to be the same name since things have to reference them by name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that gen_niflib.py overwrites the variable with the same name. This applies to the Vertex Data.
https://i.imgur.com/9SXYhK4.png

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

This is a case where gen_niflib.py needs to be fixed then, as this behavior is now required in nif.xml and especially in NifSkope. Simply put the type of an <add> needs to be able to change between versions while maintaining the same name.

Instead gen_niflib should make the names unique itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The material uses a pobny approach. Only need to give different names.
https://i.imgur.com/9rQwlRq.png

Copy link
Member

@hexabits hexabits Jun 13, 2017

Choose a reason for hiding this comment

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

The names need to stay the same... Not necessarily for HavokMaterial, but for the others. Including HavokColFilter: "Layer" needs to remain identical or features of the program will break. For example:

Copy link
Member

Choose a reason for hiding this comment

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

In that example the variable and the field are the same thing between game versions, it's just that the type can change. In C++ this could be handled with std::variant for example.

Copy link
Member

Choose a reason for hiding this comment

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

In the absence of std::variant or other solutions, gen_niflib.py would just have to automatically create unique names for them. In the Layer example they are always functionally a "Layer", there is no "Fallout Layer" or "Oblivion Layer" as the end result is functionally identical. It's just simply the collision layer but with a type that varies per version.

PyFFI already solved this issue, so niflib should have to come up with a solution as well.

You may need to create a niflibname attribute or something which complements the name attribute. When niflibname is present, gen_niflib.py will use that instead.

Copy link
Member

@hexabits hexabits Jun 13, 2017

Choose a reason for hiding this comment

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

Or maybe a more generic uniquename as to not be niflib-specific.

For example:

        <add name="Layer" uniquename="OBLayer" type="OblivionLayer" ... />
        <add name="Layer" uniquename="FOLayer" type="Fallout3Layer" ... />
        <add name="Layer" uniquename="SKLayer" type="SkyrimLayer" ... />

I will probably create an issue about this. Edit: I have created #64 to discuss this.

@hexabits hexabits changed the title Correction EffectShaderControlledVariable Updates for niflib Jun 13, 2017
This was referenced Jun 13, 2017
hexabits added 5 commits June 14, 2017 12:25
Also updated definitions for Eye Data (missed during decoding from niftools#58)
Adopted proposal from niftools#63.  Only the enums with collisions will be updated for this PR.
Implementation of proposal in niftools#64.
@hexabits
Copy link
Member

@aperture-it OK I believe I've implemented prefix/suffix fully, and recombined BSVertexData.

The crap with three Data/Skin Instance in NiGeometry is actually due to inheritance changes in their engine. They don't actually use NiGeometry anymore. As of SSE and FO4 they use "BSGeometry" with a completely different layout and currently nif.xml is just entirely wrong for NiGeometry and NiParticleSystem. We don't know how to handle these inheritance changes in nif.xml so I haven't ported over any of my decoding for it from my 010 templates.

But anyway I think all the issues you've brought up have been addressed so far, as long as you make the updates to nifdocsys. Can you open up a nifdocsys PR with your changes?

@hexabits
Copy link
Member

hexabits commented Jun 14, 2017

@aperture-it Why does name="Data" type="Ref" need a suffix? They are the "base" / default. How have you implemented it then?

<add name="Data" type="Ref" ... />
<add name="Data" suffix="BS" type="Ref" ... />
<add name="Data" suffix="BSPS" type="uint" ...  />

should result in

Ref data;
Ref data_bs;
uint data_bsps;

If you've implemented it any other way I think that's a design problem.

@aperture-it
Copy link
Contributor Author

Sorry, I'm wrong. The first type can be without a suffix.

@aperture-it
Copy link
Contributor Author

aperture-it commented Jun 14, 2017

Can you open up a nifdocsys PR with your changes?

niftools/nifdocsys#6

@aperture-it
Copy link
Contributor Author

What is 010 templates?

@hexabits
Copy link
Member

Where all the decoding work from #58 came from. https://github.com/jonwd7/0x6e6966746f6f6c73/tree/master/nif

We've ported most of it to nif.xml except for NiGeometry/NiParticleSystem. Stuff like this https://github.com/jonwd7/0x6e6966746f6f6c73/blob/master/nif/Particles.bt#L15 currently isn't handled in nif.xml. (Inheritance changes between NIF versions)

@hexabits
Copy link
Member

I have just realized that the re-unification for BSVertexData in 9eb4c2a has broken SSE NIFs with BSDynamicTriShape. I will need to address this somehow.

BSVertexData/BSVertexDataSSE

BSVertexData unified

@hexabits
Copy link
Member

What? No. That's exactly what it's supposed to be.

@hexabits
Copy link
Member

hexabits commented Jun 17, 2017

If there were too many flags, no skinned SSE file would parse. The Data Size is the size in bytes and the Vertex Size is (VF1 & 0x3F) * 4.

@aperture-it
Copy link
Contributor Author

I confused the Vertex Size with the flag :)

Maybe

<add name="UV" type="HalfTexCoord" cond="(((ARG &amp; 16) != 0) &amp;&amp; ((ARG &amp; 32) != 0))" />

Remove ARG &amp; 16) != 0

@hexabits
Copy link
Member

hexabits commented Jun 17, 2017

Yes I know what to do already, except I've now run into some kind of issue with NifSkope and the externalcond attribute I add to the compound for condition caching (It's as of yet uncommitted to nifxml).

I already committed once and then did a hard reset and push force when I found out that it broke things.

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

<add name="Vertex" suffix="Half" type="HalfVector3" userver2="130" cond="((ARG &amp; 16) != 0) &amp;&amp; ((ARG &amp; 16384) == 0)" />
<add name="Bitangent X" suffix="Half" type="hfloat" userver2="130" cond="((ARG &amp; 16) != 0) &amp;&amp; ((ARG &amp; 256) != 0) &amp;&amp; ((ARG &amp; 16384) == 0)" />
<add name="Unknown Short" type="ushort" userver2="130" cond="((ARG &amp; 16) != 0) &amp;&amp; ((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; 16) != 0) &amp;&amp; ((ARG &amp; 256) != 0) &amp;&amp; ((ARG &amp; 16384) != 0)" />
<add name="Unknown Int" type="uint" userver2="130" cond="((ARG &amp; 16) != 0) &amp;&amp; ((ARG &amp; 256) == 0) &amp;&amp; ((ARG &amp; 16384) != 0)" />

<add name="UV" type="HalfTexCoord" cond="(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="Eye Data" type="float" cond="(ARG &amp; 4096) != 0" />

I had to remove & 16 from UV and add it to everywhere else. Suddenly that is breaking NifSkope (when using externalcond). :(

Basically NifSkope needs BSVertexData and BSVertexDataSSE to remain separate. Guess I will just have to keep my own nif.xml with this compound split.

@hexabits
Copy link
Member

@aperture-it What if I do this:

    <compound name="BSVertexData">
        ...
    </compound>

    <compound name="BSVertexDataSSE" niflibtype="BSVertexData">
        ....
    </compound>

So that niflib can treat them the same? Will this work? Just everything in NifSkope I've done to accelerate parsing wants me to keep BSVertexData and BSVertexDataSSE separate...

@aperture-it
Copy link
Contributor Author

& 16 do not add. The rest of the data does not depend on the vertices.

@hexabits
Copy link
Member

I already know this. Please look at the XML I've pasted above: #62 (comment)

As I keep trying to tell you, I cannot do this because of externalcond. For some reason shifting the & 16 around BREAKS NIFSKOPE.

Because what you are not seeing is that

<compound name="BSVertexData">

is actually for me

<compound name="BSVertexData" externalcond="1">

Which is a condition caching mechanism employed by NifSkope to greatly accelerate the parsing speed.

@aperture-it
Copy link
Contributor Author

If the merge BSVertexDataSSE and BSVertexData causes a lot of problems, then you should separate them as before.

@hexabits
Copy link
Member

Yes but will

    <compound name="BSVertexDataSSE" niflibtype="BSVertexData">
        ....
    </compound>

Work for niflib? Redirecting BSVertexDataSSE -> BSVertexData.

@aperture-it
Copy link
Contributor Author

Yes, it will work niflib.

This reverts commit 9eb4c2a.

Split BSVertexData/BSVertexDataSSE back up because of issues with NifSkope's condition caching.  Use niflibtype to redirect BSVertexDataSSE to BSVertexData.
@hexabits
Copy link
Member

OK @aperture-it will 61b3115 work for you with the niflibtype="BSVertexData" and there is no need for suffix anywhere correct? Because I alias BSVertexDataSSE -> BSVertexData?

@aperture-it
Copy link
Contributor Author

Yes, it will work. But the functions of writing and reading need to be rewritten manually.
To gen_niflib.py not overwritt them, you need to add an attribute is_manual_update="3" to Vertex Data in BSTriShape and NiSkinPartition

@hexabits
Copy link
Member

Why 3? From here https://github.com/niftools/nifdocsys/blob/0ffd95a50c277e761ab664b68a827502d163c083/gen_niflib.py#L94

It just looks true/false. Also should that be handled manually in gen_niflib then? I don't think "is_manual_update" is a valid attribute for nif.xml. At least it's not in our spec in the wiki.

@aperture-it
Copy link
Contributor Author

I use the code figment https://github.com/figment/nifdocsys/blob/master/nifxml.py#L1294

In the wiki this attribute is not documented, but it is used in the figment branch to skip overriding the functions.

@aperture-it
Copy link
Contributor Author

The crap with three Data/Skin Instance in NiGeometry is actually due to inheritance changes in their engine. They don't actually use NiGeometry anymore. As of SSE and FO4 they use "BSGeometry" with a completely different layout and currently nif.xml is just entirely wrong for NiGeometry and NiParticleSystem.

Maybe it makes sense to create BSGeometry? And inherit from it new block BSParticleSystem.

@hexabits hexabits merged commit 4dfa0de into niftools:develop Jun 27, 2017
hexabits added a commit to hexabits/nifdocsys that referenced this pull request Jun 28, 2017
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants