niftools / nifxml

A repository for the nif.xml file, which contains the nif file format description.
http://www.niftools.org
GNU General Public License v3.0
37 stars 43 forks source link

[Regression] Skyrim collision layer colors in NifSkope #27

Closed hexabits closed 10 years ago

hexabits commented 10 years ago

Resolves #19

NifSkope looks for a game-agnostic value titled "Layer" and displays a different color for each enum value. It cannot grab this enum for Skyrim NIFs because it was renamed and the original "Layer" (OblivionLayer) was disabled for User Version > 11.

neomonkeus commented 10 years ago

@ttl269 can you review & merge if ok.

ttl269 commented 10 years ago

@jonwd7 @neomonkeus Why this? I have suggested similar modification together with other things on April - see this pull request

neomonkeus commented 10 years ago

This is just to change the attribute names rather than the contents of the types.

@jonwd7 - why do we need to change this on the nifxml side, Can NifSkope not just check the string name value? If we want to come back and change the attribute name to be more descriptive, as layer might not be intuitive for a user as opposed to say, eg. Havok Collision Layer, then we have that inverse dependany that the format is decided by the tools? Unless I am misunderstanding the Layer

hexabits commented 10 years ago

@ttl269 Sorry did not see that.

@neomonkeus Because the names were changed for no reason. I can't check the string name value because it was changed, like I showed.

0.7.0.0 was "Layer" and "Layer Copy" for the type OblivionLayer. For both Oblivion and Skyrim NIFs 0.7.1.0 kept "Layer" and "Layer Copy" for the type OblivionLayer, but created a new type, SkyrimLayer and the names are "Skyrim Layer" and "Skyrim Layer Copy".

There is no need to have the word "Skyrim" in the name, or for the names to differ between OblivionLayer and SkyrimLayer. If it's named "Skyrim Layer" then it should also be named "Oblivion Layer", but obviously we don't want or need the game specific names.

I understand your argument about creating dependencies, but in this case it is a lose/lose situation. The name "Layer" was being checked, but if I switch away from the name check that means I need to check the type. However, the type also changed, from OblivionLayer to SkyrimLayer so we'd still be facing the same issue.

With that said, the entirety of NifSkope is one big dependency on the structure of nif.xml and the names and types therein. Almost every "get" function requires the name of the value to get given a block. If I need the array of Vertices on an NiTriShapeData, it's not the only array of Vector3 in that block, so obviously I need to say which block of Vector3 I need. That is done by nif->getArray<Vector3>( iData, "Vertices" ) for the block at index iData.

When a name or type has to change, it needs to be checked in the NifSkope source too, otherwise any code relying on the old name/type is going to break. In this case there was no reason to change the name and it would set a bad precedent going forward, to create a different "Layer" name for each game that comes out. This convention would be bad for maintainability, needing continual updates in NifSkope to the list of names that all are equivalent to a "Layer".

@ttl269 does a good job of removing all the game-specific names. Of course it is a change that will still require changes in NifSkope, but it is a positive change. :)

ttl269 commented 10 years ago

@jonwd7 That adding of word "Skyrim" into names was made by me. I wanted Nifskope users to clearly see that this is Skyrim related item. Now I see that it was mistake:

@neomonkeus Thakns for noticing spelling mistake. I have corrected it.