nifskope icon indicating copy to clipboard operation
nifskope copied to clipboard

Added BSShaderTextureSets merging

Open deedes opened this issue 9 years ago • 12 comments

"Combine Properties" Spell now combines identical BSShaderTextureSets. Checked if BSShaderTextureSets could be merged in Fallout 3, NV and Skyrim and found evidence in vanilla Skyrim .nif files that they are actually meant to be merged.

deedes avatar Sep 18 '14 21:09 deedes

Adding @jonwd7 for input.

neomonkeus avatar Sep 20 '14 15:09 neomonkeus

The blender plugin merges identical texture sets by default. I think the other plugins do as well but not positive.

Ghostwalker71 avatar Nov 03 '14 21:11 Ghostwalker71

@jonwd7 @deedes This spell ("Combine Properties") needs to be corrected to not combine the properites which are not allowed to be shared (they need to be unique for each node, otherwise it leads to Creation Kit crash). These properties have to stay unique: BSLightingShaderProperty, BSEffectShaderProperty, BSSkyShaderProperty, BSWaterShaderProperty.

ttl269 avatar Jan 10 '15 22:01 ttl269

@ttl269 So are two unique BSLightingShaderProperty blocks allowed to point to the same shared BSShaderTextureSet or do all the child properties have to be unique as well? If the child properties have to be unique then this BSShaderTextureSet change to Combine Properties is actually incorrect, because AFAIK BSShaderTextureSet is always a child to the BS*ShaderProperty blocks.

hexabits avatar Jan 10 '15 23:01 hexabits

@jonwd7 Yes - two (or more) unique BSLightingShaderProperty blocks are allowed to link same BSShaderTextureSet, i.e. BSShaderTextureSet can be linked multiple times in one nif file. It is not actually a property, it is some kind of data block - a list of items (textures in this case).

ttl269 avatar Jan 11 '15 16:01 ttl269

@ttl269 I just noticed that BSLightingShaderProperty, BSEffectShaderProperty, BSSkyShaderProperty, BSWaterShaderProperty all have the same two rows in common (Shader Flags 1 and Shader Flags 2) ... Should these not all inherit a block similar to BSShaderProperty?

Maybe I should create a ticket in nifxml to discuss this?

After looking at the old code, it's odd that the BSShaderTextureSet was explicitly disallowed from combining if it's in fact OK to do so. I'll try to track down the source of this addition and see what their reasoning was.

hexabits avatar Jan 11 '15 16:01 hexabits

@deedes @neomonkeus @ttl269 ... I tracked down the introduction of that condition to niftools/nifskope@5c7e253 but unfortunately there is no reasoning given for the change. What about combining BSShaderTextureSet in older games like Fallout 3? Maybe they can't be combined in versions earlier than Skyrim?

hexabits avatar Jan 11 '15 16:01 hexabits

@jonwd7 Yes, all these four Bethesda Shader Property blocks you mentioned use same two ShaderPropertyFlags and also next two items (UV Offset and UV Scale). So, some compound could be created and used by these blocks. Right now I am working on some update of BSEffectShaderProperty in nif.xml, so I can include this new compound into this update.

ttl269 avatar Jan 11 '15 16:01 ttl269

@jonwd7 What about combining BSShaderTextureSet in older games like Fallout 3? Maybe they can't be combined in versions earlier than Skyrim? Tested in Fallout 3 and there is no problem with sharing of BSShaderTextureSet . BTW: Tested also sharing NiTriShapeData block by more than one NiTriShape block and it also works (same as it works in Skyrim).

ttl269 avatar Jan 11 '15 17:01 ttl269

@ttl269 If it used block inheritance (an abstract parent) that would be better because instead of explicitly excluding all of {BSLightingShaderProperty, BSEffectShaderProperty, BSSkyShaderProperty, BSWaterShaderProperty} from the Combine Properties spell, I would just have to test for whether the block inherits their parent.

Note that the exact same thing is done currently for BSShaderLightingProperty, WaterShaderProperty, SkyShaderProperty and so on... they all have a parent BSShaderProperty. The Combine Property spell excludes all of them via

nif->inherits( iBlock, "BSShaderProperty" )

Unfortunately the naming is already super confusing on all these blocks, so I have no idea what you would name the abstract parent.

hexabits avatar Jan 11 '15 18:01 hexabits

@deedes @ttl269 @neomonkeus

Revisiting this, I believe that combining BSShaderTextureSet blocks with a "Combine Properties" spell is misleading and unwanted behavior considering a texture set is not a property. There may be times one wants to combine the properties but leave the texture sets alone.

Since the texture set does not inherit NiProperty in any way, a separate spell called "Combine Texture Sets" should be devised instead.

hexabits avatar Mar 14 '16 21:03 hexabits

@jonwd7, indeed, there should be two separate issues. Not sure if you can merge texture sets. I think the tangent about properties and the inheritance tree hierarchy should be revisited. Not to clear what is involved when it comes to BSShaderTextureSet as those as members of the property blocks.

neomonkeus avatar Mar 14 '16 21:03 neomonkeus