niftools / pyffi

PyFFI is a Python library for processing block structured files.
http://www.niftools.org/pyffi
Other
48 stars 26 forks source link

Fix ExtraVectorsFlags not being set even though data populated. #25

Closed neomonkeus closed 8 years ago

neomonkeus commented 8 years ago

Scope

As from Pull request #15 , the Num UV Sets flag from the NiGeometryData object switched from a bitflags type to an enum, and changes were made accordingly to the PyFFI file format interface.

This flag handles the Tangents and Bitangents members existence in the object, and could take a value &-able with 16 to show that the members should exist, and any other otherwise.

Issue

A problem was found where an UV Set was added and the tangents/bitangents were automatically recalculated. However, the data was never saved to the file.

Solution

This change handled the case in which UV sets didn't exist by setting the new enum to 0. However, it didn't handle the case when an UV value was added.

The pull request solve this case by setting the flag to 16[Tangents_Bitangents] when an UV_set is added. This way, the tangents and bitangents can be computed and stored in the object, and it will be written in the file down the line.

Extra information

The structure and expected data of the UV Sets member was described here : http://niftools.sourceforge.net/forum/viewtopic.php?f=10&t=321

As PyFFI seems to only use the first UV set, it may be an incomplete feature that might need fixing later.

neomonkeus commented 8 years ago

Did some actual testing. Yep, looks like the original implementation was completely broken. If the update_tangent method had been actually called for either branch, it would have failed with an AttributeError because ExtraVectorsFlags doesn't exist in the model. The reason being that the nif.xml submodule version (0.7.1.0) was never updated to point to the correct updated version (0.7.1.1).

Before this can be merged I need to add an additional commit to update the submodule so that the versioning is correct

Memory dump on object with implementation as is

Checking data
<class 'pyffi.formats.nif.NiTriStripsData'> instance at 0x0A6C1940
* unknown_int : 0
* num_vertices : 6
* keep_flags : 0
* compress_flags : 0
* has_vertices : True
* vertices :
    <class 'pyffi.object_models.xml.array.Array'> instance at 0x0A67FB88
    0: [ -84.730 63.487 220.734 ]
    1: [ -84.730 105.453 220.777 ]
    2: [ -84.730 40.911 220.734 ]
    3: [ -84.730 80.577 220.734 ]
    4: [ -84.730 105.453 220.777 ]
    5: [ -84.730 63.487 220.734 ]
* num_uv_sets : 0
* bs_num_uv_sets : 4096
* skyrim_material : SKY_HAV_MAT_LIGHT_WOOD
* has_normals : True
* normals :
    <class 'pyffi.object_models.xml.array.Array'> instance at 0x0A67F9A8
    0: [  1.000 -0.000  0.000 ]
    1: [  1.000 -0.000  0.000 ]
    2: [  1.000 -0.000  0.000 ]
    3: [  1.000  0.000 -0.000 ]
    4: [  1.000  0.000 -0.000 ]
    5: [  1.000  0.000 -0.000 ]
* tangents :
    <class 'pyffi.object_models.xml.array.Array'> instance at 0x0A67FC28
    0: [ -0.000 -1.000 -0.000 ]
    1: [ -0.000 -1.000 -0.000 ]
    2: [ -0.000 -1.000 -0.000 ]
    3: [  0.000 -1.000 -0.000 ]
    4: [  0.000 -1.000 -0.000 ]
    5: [  0.000 -1.000 -0.000 ]
* bitangents :
    <class 'pyffi.object_models.xml.array.Array'> instance at 0x0A67FC78
    0: [ -0.000  0.000  1.000 ]
    1: [ -0.000  0.000  1.000 ]
    2: [ -0.000  0.000  1.000 ]
    3: [  0.000 -0.000  1.000 ]
    4: [  0.000 -0.000  1.000 ]
    5: [  0.000 -0.000  1.000 ]

The implementation gets away with this because it is just an assignment, but because the attribute isn't a modelled attribute it won't get written to memory. I have been able to run the scenario on the edge mesh and can see it has no affect, even with the current implementation without the updated nif.xml

neomonkeus commented 8 years ago

Updated nif.xml

Verified fix, before & after. fix

In the case where it was actually being called, because the flag was not set, the data would be lost on write.

Dexesttp commented 8 years ago

Ok, that's really good. The part where you point to a newer NifXML version kind of worries me though, as I'm not sure if it would affect other parts of PyFFI. I'll check and come back to it when I have more info.

Dexesttp commented 8 years ago

Update : the actual changes between the two NifXML version (older and newer) span over two years. The following link is the actual comparison : https://github.com/niftools/nifxml/compare/959cb9c3dd59a319e60e819fc8a1402f821f3684...f265c56482c728c6877e45d5b5993d3bff83670a

Therefore, I'm not sure that this should take place in this pull request but rather in a dedicated PR. What do you think ?

Dexesttp commented 8 years ago

Ok, so after discussion there was already a study of the new nif.xml. It adds mostly new enums and sometimes new compounds, but there should be no needed support changes for the API. It was also tested.

I am ok for the changes then, including the nif.xml.

neomonkeus commented 8 years ago

Support was verified up to the changes introduced in 0.7.1.1 - #12 The changes made by 0.7.1.1 are tested by this fix.