Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Added BSShaderTextureSets merging #72

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Added BSShaderTextureSets merging #72

wants to merge 1 commit into from

Conversation

deedes
Copy link

@deedes deedes commented Sep 18, 2014

"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.

"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.
@neomonkeus
Copy link
Member

Adding @jonwd7 for input.

@Ghostwalker71
Copy link

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

@ttl269
Copy link
Member

ttl269 commented Jan 10, 2015

@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.

@hexabits
Copy link
Member

@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.

@ttl269
Copy link
Member

ttl269 commented Jan 11, 2015

@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).

@hexabits
Copy link
Member

@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
Copy link
Member

@deedes @neomonkeus @ttl269 ... I tracked down the introduction of that condition to 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?

@ttl269
Copy link
Member

ttl269 commented Jan 11, 2015

@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
Copy link
Member

ttl269 commented Jan 11, 2015

@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).

@hexabits
Copy link
Member

@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
Copy link
Member

@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.

@neomonkeus
Copy link
Member

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants