Models using the same NiMaterialProperty names, but different NiSourceTexture blocks cause erroneous behavior

[SK83RJOSH]

@niftools/blender-nif-plugin-reviewer -

Issue Overview

I've come across a handful of models that appear to use the same material names in their NiMaterialProperty block. This is pretty average and not an issue in and of itself. The issue is that these models also use different NiSourceTexture blocks for each instance, which leads to the importer wiping out the old material instance on import. The game appears to support having both of these materials loaded at once, and properly binds different textures to both objects.

Version Information

Blender Nif Plugin Version Info

2.6.0

Blender Version Info

2.82

Platform information

Windows 10 Pro

Context

I suspect the game in question (Bully: Scholarship Edition) isn't unique here, and that most other titles support swapping out the source textures for previously defined materials. This error is also somewhat hard to catch unless you're very familiar with what the imported model should look like, and can lead to users getting incorrect imports / exports. That said, I think the plugin should be able to gracefully handle this without messing up the existing model.

Steps to Reproduce

1. Download the attached zip file
2. Extract it at your leisure
3. Open the blend file, and take note of the existing scene geometry, as it is incorrect
4. Delete the geometry and reimport SC1b_fence04.nif, if it's the same, it's broken

Expected Result

You should see a scene with at least 3-4 sets of uniquely textured fences. None of them should have bent bars.

Actual Result

Most of the fences are using an identical texture that tiles very obviously. It has a single bar that's bent vertically and is dark green. This should actually be a black fence with straight bars and / or a chain link fence depending on the fence in question.

Possible Fix

I would suggest that materials using different source textures, but the same material name, be duplicated on import if an existing material isn't a match. This way the original material name + source textures could be stored as metadata on the material object in a similar fashion to Niftools Material Flags. This would ensure that material names and source textures are user editable and not destroyed on import.

Screenshot

Attachments

Files.zip

[SK83RJOSH]

@HENDRIX-ZT2 / @neomonkeus I wouldn't mind tackling this issue if one of you two can decide on what the best approach would be. I don't want to start making changes to material configurations without consulting one of you two on what approach would be the least invasive for existing workflows. 🙂

[HENDRIX-ZT2]

This is where you would hook it in:

https://github.com/niftools/blender_nif_plugin/blob/fea79910e12d52cb4bfe3bbedf130caf886464ae/io_scene_nif/modules/nif_import/property/material/__init__.py#L109

The get_material_hash function has been removed during the port to 2.8, you'll either want to retrieve it from older commits or rewrite something similar from scratch. Not sure if using a dict lookup is better than checking the blender materials and their textures each time. The dicts caused a lot of issues during the rewrite, that's why they were removed in the first place.

[HENDRIX-ZT2]

Yeah, storing the original name makes sense.

[HENDRIX-ZT2]

Ah sorry, this is where you really have to look and adjust the lookup conditions for existing materials (only re-use if mat has been imported and uses the same textures):

https://github.com/niftools/blender_nif_plugin/blob/fea79910e12d52cb4bfe3bbedf130caf886464ae/io_scene_nif/modules/nif_import/property/geometry/mesh.py#L76

@neomonkeus This forces us to re-think our processor approach.

[SK83RJOSH]

So I've been going over this a little bit, and I'm struggling with how this should be implemented. I would think that a "correct" implementation would check if the existing material would be modified by any of the incoming processors. If so, we'd want to create a new material for correctness' sake. Since in reality this problem isn't limited to source textures, but rather all properties that can differ between materials. Unfortunately it's ultimately a bit code heavy to do this depending on the approach taken.

So far these are the potential approaches I could see working:

1. Create a new material and apply all processors, check the material hash against the existing material (expensive, but we already do the expensive part, just without creating the new material)
2. Iterate over all the incoming properties and check their settings against the existing material settings (cheaper, but then we need to know what values a property produces, and which fields it modifies)
3. Store all of the properties as block data on the material, compare them against incoming properties (not sure if it's viable to register pyffi types directly, and manual translation to property groups would be a nightmare)

Of course, the first option would be the least code heavy to implement. However, it comes with the drawback that materials created with different plugin versions would likely get re-imported and old instances would stick around and be tied to old meshes. Maybe this is ideal, maybe it's not? Do we want the plugin to have the ability to update existing materials?

I'm also not sure how the current processor approach works with exporting. Is the intention to allow users to make modifications to materials directly and have the plugin work out what properties would need to be exported? Or do you see there being a lot more property groups being added in the future for the various material options?

--

Here's the branch I'll be using to tackle this issue, it includes some miscellaneous fixes for now: https://github.com/niftools/blender_nif_plugin/compare/develop...SK83RJOSH:material-import-fixes

[neomonkeus]

So I've been going over this a little bit, and I'm struggling with how this should be implemented. I would think that a "correct" implementation would check if the existing material would be modified by any of the incoming processors. If so, we'd want to create a new material for correctness' sake. Since in reality this problem isn't limited to source textures, but rather all properties that can differ between materials. Unfortunately it's ultimately a bit code heavy to do this depending on the approach taken.

Yes, and we were hoping that the processor approach would reduce this complexity somewhat as this is what the original implementation looked like, both in terms of passing in properties, more were tacked on over time especially the bsShaderProperty nodes...

def get_material_hash(self, n_mat_prop, n_texture_prop,
n_alpha_prop, n_specular_prop,
textureEffect, n_wire_prop,
bsShaderProperty, extra_datas):
"""Helper function for import_material. Returns a key that
uniquely identifies a material from its properties. The key
ignores the material name as that does not affect the
rendering.
"""
return (n_mat_prop.get_hash()[1:]   if n_mat_prop else None, # skip first element, which is name
n_texture_prop.get_hash()   if n_texture_prop  else None,
n_alpha_prop.get_hash()     if n_alpha_prop else None,
n_specular_prop.get_hash()  if n_specular_prop  else None,
textureEffect.get_hash()    if textureEffect else None,
n_wire_prop.get_hash()      if n_wire_prop  else None,
bsShaderProperty.get_hash() if bsShaderProperty else None,
tuple(extra.get_hash()      for extra in extra_datas))

def import_material(self, n_mat_prop, n_texture_prop,
                    n_alpha_prop, n_specular_prop,
                    textureEffect, n_wire_prop,
                    bsShaderProperty, extra_datas):

https://github.com/niftools/blender_nif_plugin/blob/master/scripts/addons/io_scene_nif/materialsys/material.py#L55

The main focus of the implementation was to simply the process adding of property nodes and create a common interface for mapping common functionality to underlying Blender node types, eg NiAlphaProperty > Transparency node (not yet implemented), this is the third element of reusing nodes.

> So far these are the potential approaches I could see working:
> 
>     1. Create a new material and apply all processors, check the material hash against the existing material (expensive, but we already do the expensive part, just without creating the new material)
> 
With the move 2.8, we need to migrate more of the common logic to use specific node types, eg glossy, emission, transparant - https://docs.blender.org/manual/en/latest/render/shader_nodes/shader/index.html
This can reduce the complexity if we lookup by node type before performing setting checking.

>     2. Iterate over all the incoming properties and check their settings against the existing material settings (cheaper, but then we need to know what values a property produces, and which fields it modifies)
> 
This should be available as we do field checking in the common logic for block export, so could be reuse there if split out to essentially become the hash.
>     3. Store all of the properties as block data on the material, compare them against incoming properties (not sure if it's viable to register pyffi types directly, and manual translation to property groups would be a nightmare)
> 
> Of course, the first option would be the least code heavy to implement. However, it comes with the drawback that materials created with different plugin versions would likely get re-imported and old instances would stick around and be tied to old meshes. Maybe this is ideal, maybe it's not? Do we want the plugin to have the ability to update existing materials?
> 
I think this is fine for a development version for now, and in general backward incompatibility is never any fun

> I'm also not sure how the current processor approach works with exporting. Is the intention to allow users to make modifications to materials directly and have the plugin work out what properties would need to be exported? Or do you see there being a lot more property groups being added in the future for the various material options?
> 
Ideally we would like to have a similar system which would read over nodes, but it might not be ideal.