BSShaderProperties update

[ttl269]

@neomonkeus @skyfox69 @jonwd7 @Ghostwalker71 @throttlekitty @nexustheru @amorilia
Changes made in BSEffectShaderProperty:

1. Changed Texture Clamp Mode storage type from uint to byte, due to Skyrim's BSEffectShaderProperty, where the byte value of Texture Clamp Mode is followed by unknown non-zero byte value - it caused showing of a number in Texture Clamp Mode line instead of its showing of named value. This unknown byte is mostly set to 255 but in many meshes there are other values.
2. "Emmisive Color" splitted to "Base Color" (renamed according to CK) and "Alpha". Now the variables 0 and 5 in EffectShaderControlledVariable list can be clearly named.

Added proposal of abstract parent object for Bethesda Shader properties which must not be shared (can't be "combined") for easier excluding of these properties in Nifskope's Combine Properties spell as requested @jonwd7 #72. It is named NonSharableBSShaderProperty.

[hexabits]

NonSharableBSShaderProperty is a bit of a mouthful, and in actuality all the properties that inherit from BSShaderProperty (WaterShaderProperty, SkyShaderProperty, etc.) are nonshareable as well.

So BSShaderProperty vs NonSharableBSShaderProperty is actually a bit of a misnomer as they are both nonshareable.

So I think the name doesn't make very much sense. Maybe instead of defining these properties as "nonshareable" in the name, we introduce a new attribute.

Something like:

<niobject name="BSShaderProperty" abstract="0" inherit="NiProperty" unique="true">

Unfortunately I still don't know what to name the block, as BSShaderProperty is already taken. I assume BSShaderProperty2 is not the greatest idea. Guess we can ask @neomonkeus what he thinks.

[neomonkeus]

So we are inserting an abstract class in the inheritence tree for type checking. Seems okay to me. Would probably go with BsUniqueShaderProperty or somega permutation of it.

Not sure I follow your suggestion to use an attribute as not really sure how you know its relation to other blocks. Whereas there is explicit uniqueness through subclassing.

[hexabits]

@neomonkeus The attribute says "properties of this type cannot be combined". Put it on the abstract class, that means that all of the properties that inherit it must be unique. Therefore you do not need to name the abstract parent anything with "Unique" or "Nonshareable" as I already have shown that doing so would be a misnomer. (BSShaderProperty and its children must also be unique but the name doesn't indicate that).

The uniqueness is not explicit through subclassing, as programs such as NifSkope would still have to add the check for "inherits BSUniqueShaderProperty". The name of the block is arbitrary and just a hint as to the purpose, so it's implicit. Whereas checking the attribute unique=true is name agnostic and explicit.

Also, imagine that in the future there will be a whole new class of NiBlocks for new games that should not inherit from the existing abstract parent (BSUniqueShaderProperty), but a new type of block. All these properties must also be unique. If you just add unique=true to the parent, then NifSkope would know to skip them and a check for these blocks would not have to be added in manually. Thus, automatic futureproof support.

The only initial work would be adding a check for the new attribute to the parser.

[neomonkeus]

I take your point about using the inheritance tree should they decide to subclass from a unique and make it non-unique. Suppose my brain is not fully engaged, but what happens in the case of a sub-set of the sub-classes need to be unique.

[hexabits]

@72b711d ... Ah, didn't think to do it that way at all. 👍

[ttl269]

@jonwd7 @neomonkeus
I agree with jonwd7, that new attribute should be used for this purpose instead of adding next abstract object.
But looking into nif structure listed by CK preview it seems that BSLightingShaderProperty (and next three properties) should be inherited from BSShaderProperty (same as Fallout 3 specific shader property blocks are shown in Geck and are structured in nif.xml). So I have made necessary change to nif.xml.

[ttl269]

Although I understand advantages of having new attribute in nif xml, I don't know if necessary work to adding it would be worthwile. It depends on how often it would be used in applications. We talked about it only due to Combine Spell bug. @jonwd7 - you know nifsope's code - would it be benefical to have this attribute?

[hexabits]

@ttl269 In the short term, not really. In the long term, it'd lower maintenance cost of the codebase. Right now the only check is nif->inherits( iBlock, "BSShaderProperty" ) ... maybe a new group of blocks will require a change in this condition, meaning having to update the NifSkope codebase for support. With the attribute the syntax for the check could simply become something like nif->isUnique( iBlock ), and after adding such functions and adding support to the parser, the support for new unique properties would be automatic.

However I agree that it's probably not worth it unless the XML format is going to undergo large changes anyway (like the ones in my proposals). So I'm fine with shelving such an attribute until then.

[hexabits]

@ttl269 Related, I was looking at if this BSShaderProperty change would have any implications in the NifSkope codebase and it doesn't seem to, however this commit I tracked down for some code is a bit confusing: niftools/nifskope@ab8bc18

I'm confused by the wording:

Temporary workaround for non-NiProperty properties

Did BSShaderProperty not inherit from NiProperty at some point in nif.xml's history? How are they "non-NiProperty" properties?

Also I'm still figuring out what that code means, but it seems it was never updated for BSEffectShaderProperty and others... though now with BSShaderProperty as parent I should actually be able to remove that first condition.

[neomonkeus]

Did some poking through the decoding forums - http://niftools.sourceforge.net/forum/viewtopic.php?f=10&t=3193

[ttl269]

@jonwd7 Did BSShaderProperty not inherit from NiProperty at some point in nif.xml's history? It seems that no. Just before doing change to nif.xml today I have been searching for when BSShaderProperty was added to nif.xml (because this block was strange to me when I discovered that it is not present in any Oblivion, Fallout 3 or Skyrim nif file. So, isn't it a real node and should it be abstract?). They added it 29. 10. 2008 - but only added it, not used anywhere in nif.xml. I don't understand this step - maybe it was used in nifskope?. Then on 16.11.2008 they made change and BSShaderProperty was used for inheritance in nif.xml.

[hexabits]

Typo in "default" on this line and the next.

[neomonkeus]

While doing some testing on the Blender plugin, I spotted some parcularities, which mainly tie back into this proposed change. Had a poke about the SKSE & FOSE code bases and they also use the BSShaderProperty, but with one slight difference in that it inherits from NiShader.

I think we should investigate a similiar solution as there are some accompanying things, like how there is an attribute check in a base class (NiNode) for an attribute that is part of the sub-class (BSLightingShaderProperty) which will is should just be done as part of that class.