From 73509d1bc1cbdbd5365f545e880e15959ae947eb Mon Sep 17 00:00:00 2001 From: Michael B Johnson Date: Sat, 18 Mar 2023 00:05:39 +0100 Subject: [PATCH 1/2] Fix #105327: USD import fails to recognize UV map varname Follow connections when reading the varname attribute of a primvar reader, and support both string and TfToken types for the varname. A unit test is also provided. Authored by Apple: Matt McLin Pull Request: https://projects.blender.org/blender/blender/pulls/105508 --- .../io/usd/intern/usd_reader_material.cc | 20 +++- tests/python/bl_usd_import_test.py | 111 ++++++++++++++++++ 2 files changed, 129 insertions(+), 2 deletions(-) diff --git a/source/blender/io/usd/intern/usd_reader_material.cc b/source/blender/io/usd/intern/usd_reader_material.cc index 2c50f852a48..3b98a99b42a 100644 --- a/source/blender/io/usd/intern/usd_reader_material.cc +++ b/source/blender/io/usd/intern/usd_reader_material.cc @@ -786,10 +786,26 @@ void USDMaterialReader::convert_usd_primvar_reader_float2( /* Set the texmap name. */ pxr::UsdShadeInput varname_input = usd_shader.GetInput(usdtokens::varname); + + /* First check if the shader's "varname" input is connected to another source, + * and use that instead if so. */ + if (varname_input) { + for (const pxr::UsdShadeConnectionSourceInfo& source_info : varname_input.GetConnectedSources()) { + pxr::UsdShadeShader shader = pxr::UsdShadeShader(source_info.source.GetPrim()); + pxr::UsdShadeInput secondary_varname_input = shader.GetInput(source_info.sourceName); + if (secondary_varname_input) { + varname_input = secondary_varname_input; + break; + } + } + } + if (varname_input) { pxr::VtValue varname_val; - if (varname_input.Get(&varname_val) && varname_val.IsHolding()) { - std::string varname = varname_val.Get().GetString(); + /* The varname input may be a string or TfToken, so just cast it to a string. + * The Cast function is defined to provide an empty result if it fails. */ + if (varname_input.Get(&varname_val) && varname_val.CanCastToTypeid(typeid(std::string))) { + std::string varname = varname_val.Cast().Get(); if (!varname.empty()) { NodeShaderUVMap *storage = (NodeShaderUVMap *)uv_map->storage; BLI_strncpy(storage->uv_map, varname.c_str(), sizeof(storage->uv_map)); diff --git a/tests/python/bl_usd_import_test.py b/tests/python/bl_usd_import_test.py index e05b32cf0e8..3c7d4e58c1c 100644 --- a/tests/python/bl_usd_import_test.py +++ b/tests/python/bl_usd_import_test.py @@ -3,6 +3,11 @@ import pathlib import sys import unittest +import tempfile +from pxr import Usd +from pxr import UsdShade +from pxr import UsdGeom +from pxr import Sdf import bpy @@ -13,6 +18,8 @@ class AbstractUSDTest(unittest.TestCase): @classmethod def setUpClass(cls): cls.testdir = args.testdir + cls._tempdir = tempfile.TemporaryDirectory() + cls.tempdir = pathlib.Path(cls._tempdir.name) def setUp(self): self.assertTrue(self.testdir.exists(), @@ -21,6 +28,9 @@ class AbstractUSDTest(unittest.TestCase): # Make sure we always start with a known-empty file. bpy.ops.wm.open_mainfile(filepath=str(self.testdir / "empty.blend")) + def tearDown(self): + self._tempdir.cleanup() + class USDImportTest(AbstractUSDTest): @@ -189,6 +199,107 @@ class USDImportTest(AbstractUSDTest): self.assertAlmostEqual(1.234, test_cam.shift_x, 3) self.assertAlmostEqual(5.678, test_cam.shift_y, 3) + def test_import_shader_varname_with_connection(self): + """Test importing USD shader where uv primvar is a connection""" + + varname = "testmap" + texfile = str(self.testdir / "textures/test_grid_1001.png") + + # Create the test USD file. + temp_usd_file = str(self.tempdir / "usd_varname_test.usda") + stage = Usd.Stage.CreateNew(temp_usd_file) + mesh1 = stage.DefinePrim("/mesh1", "Mesh") + mesh2 = stage.DefinePrim("/mesh2", "Mesh") + + # Create two USD preview surface shaders in two materials. + m1 = UsdShade.Material.Define(stage, "/mat1") + s1 = UsdShade.Shader.Define(stage, "/mat1/previewshader") + s1.CreateIdAttr("UsdPreviewSurface") + m1.CreateSurfaceOutput().ConnectToSource(s1.ConnectableAPI(), "surface") + t1 = UsdShade.Shader.Define(stage, "/mat1/diffuseTexture") + t1.CreateIdAttr("UsdUVTexture") + t1.CreateInput('file', Sdf.ValueTypeNames.Asset).Set(texfile) + t1.CreateOutput("rgb", Sdf.ValueTypeNames.Float3) + s1.CreateInput("diffuseColor", Sdf.ValueTypeNames.Color3f).ConnectToSource(t1.ConnectableAPI(), "rgb") + t2 = UsdShade.Shader.Define(stage, "/mat1/roughnessTexture") + t2.CreateIdAttr("UsdUVTexture") + t2.CreateInput('file', Sdf.ValueTypeNames.Asset).Set(texfile) + t2.CreateOutput("rgb", Sdf.ValueTypeNames.Float3) + s1.CreateInput("roughness", Sdf.ValueTypeNames.Color3f).ConnectToSource(t2.ConnectableAPI(), "rgb") + + m2 = UsdShade.Material.Define(stage, "/mat2") + s2 = UsdShade.Shader.Define(stage, "/mat2/previewshader") + s2.CreateIdAttr("UsdPreviewSurface") + m2.CreateSurfaceOutput().ConnectToSource(s2.ConnectableAPI(), "surface") + t3 = UsdShade.Shader.Define(stage, "/mat2/diffuseTexture") + t3.CreateIdAttr("UsdUVTexture") + t3.CreateInput('file', Sdf.ValueTypeNames.Asset).Set(texfile) + t3.CreateOutput("rgb", Sdf.ValueTypeNames.Float3) + s2.CreateInput("diffuseColor", Sdf.ValueTypeNames.Color3f).ConnectToSource(t3.ConnectableAPI(), "rgb") + t4 = UsdShade.Shader.Define(stage, "/mat2/roughnessTexture") + t4.CreateIdAttr("UsdUVTexture") + t4.CreateInput('file', Sdf.ValueTypeNames.Asset).Set(texfile) + t4.CreateOutput("rgb", Sdf.ValueTypeNames.Float3) + s2.CreateInput("roughness", Sdf.ValueTypeNames.Color3f).ConnectToSource(t4.ConnectableAPI(), "rgb") + + # Bind mat1 to mesh1, mat2 to mesh2. + bindingAPI = UsdShade.MaterialBindingAPI.Apply(mesh1) + bindingAPI.Bind(m1) + bindingAPI = UsdShade.MaterialBindingAPI.Apply(mesh2) + bindingAPI.Bind(m2) + + # Create varname defined as a token. + s3 = UsdShade.Shader.Define(stage, "/mat1/primvar_reader1") + s3.CreateIdAttr('UsdPrimvarReader_float2') + s3input = s3.CreateInput("varname", Sdf.ValueTypeNames.Token) + s3input.Set(varname) + t1.CreateInput("st", Sdf.ValueTypeNames.TexCoord2f).ConnectToSource(s3.ConnectableAPI(), "result") + + # Create varname defined as a connection to a token. + varname1 = m1.CreateInput("varname", Sdf.ValueTypeNames.Token) + varname1.Set(varname) + s4 = UsdShade.Shader.Define(stage, "/mat1/primvar_reader2") + s4.CreateIdAttr('UsdPrimvarReader_float2') + s4input = s4.CreateInput("varname", Sdf.ValueTypeNames.Token) + UsdShade.ConnectableAPI.ConnectToSource(s4input, varname1) + t2.CreateInput("st", Sdf.ValueTypeNames.TexCoord2f).ConnectToSource(s4.ConnectableAPI(), "result") + + # Create varname defined as a string. + s5 = UsdShade.Shader.Define(stage, "/mat2/primvar_reader1") + s5.CreateIdAttr('UsdPrimvarReader_float2') + s5input = s5.CreateInput("varname", Sdf.ValueTypeNames.String) + s5input.Set(varname) + t3.CreateInput("st", Sdf.ValueTypeNames.TexCoord2f).ConnectToSource(s5.ConnectableAPI(), "result") + + # Create varname defined as a connection to a string. + varname2 = m2.CreateInput("varname", Sdf.ValueTypeNames.String) + varname2.Set(varname) + s6 = UsdShade.Shader.Define(stage, "/mat2/primvar_reader2") + s6.CreateIdAttr('UsdPrimvarReader_float2') + s6input = s6.CreateInput("varname", Sdf.ValueTypeNames.String) + UsdShade.ConnectableAPI.ConnectToSource(s6input, varname2) + t4.CreateInput("st", Sdf.ValueTypeNames.TexCoord2f).ConnectToSource(s6.ConnectableAPI(), "result") + + stage.Save() + + # Now import the USD file. + res = bpy.ops.wm.usd_import(filepath=temp_usd_file, import_all_materials=True) + self.assertEqual({'FINISHED'}, res) + + # Ensure that we find the correct varname for all four primvar readers. + num_uvmaps_found = 0 + mats_to_test = [] + mats_to_test.append(bpy.data.materials["mat1"]) + mats_to_test.append(bpy.data.materials["mat2"]) + for mat in mats_to_test: + self.assertIsNotNone(mat.node_tree, "Material node tree is empty") + for node in mat.node_tree.nodes: + if node.type == "UVMAP": + self.assertEqual(varname, node.uv_map, "Unexpected value for varname") + num_uvmaps_found += 1 + + self.assertEqual(4, num_uvmaps_found, "One or more test materials failed to import") + def main(): global args From ea019cf01ee91ae1c09e88baffcfbca10b3be9ee Mon Sep 17 00:00:00 2001 From: Jorijn de Graaf Date: Sat, 18 Mar 2023 19:57:22 -0400 Subject: [PATCH 2/2] Fix #105152: Removing color attribute doesn't update active It now gets the index before deleting the attribute, then sets the attribute at that index as active. Pull Request: https://projects.blender.org/blender/blender/pulls/105871 --- source/blender/blenkernel/intern/attribute.cc | 63 +++++++++++++++---- .../editors/geometry/geometry_attributes.cc | 34 ---------- 2 files changed, 52 insertions(+), 45 deletions(-) diff --git a/source/blender/blenkernel/intern/attribute.cc b/source/blender/blenkernel/intern/attribute.cc index 32d12e1af17..fa39d1c6168 100644 --- a/source/blender/blenkernel/intern/attribute.cc +++ b/source/blender/blenkernel/intern/attribute.cc @@ -370,6 +370,26 @@ CustomDataLayer *BKE_id_attribute_duplicate(ID *id, const char *name, ReportList return BKE_id_attribute_search(id, uniquename, CD_MASK_PROP_ALL, ATTR_DOMAIN_MASK_ALL); } +static int color_name_to_index(ID *id, const char *name) +{ + const CustomDataLayer *layer = BKE_id_attribute_search( + id, name, CD_MASK_COLOR_ALL, ATTR_DOMAIN_MASK_COLOR); + return BKE_id_attribute_to_index(id, layer, ATTR_DOMAIN_MASK_COLOR, CD_MASK_COLOR_ALL); +} + +static int color_clamp_index(ID *id, int index) +{ + const int length = BKE_id_attributes_length(id, ATTR_DOMAIN_MASK_COLOR, CD_MASK_COLOR_ALL); + return min_ii(index, length - 1); +} + +static const char *color_name_from_index(ID *id, int index) +{ + const CustomDataLayer *layer = BKE_id_attribute_from_index( + id, index, ATTR_DOMAIN_MASK_COLOR, CD_MASK_COLOR_ALL); + return layer ? layer->name : nullptr; +} + bool BKE_id_attribute_remove(ID *id, const char *name, ReportList *reports) { using namespace blender; @@ -403,19 +423,23 @@ bool BKE_id_attribute_remove(ID *id, const char *name, ReportList *reports) BM_data_layer_free_named(em->bm, data, BKE_uv_map_pin_name_get(name, buffer_src)); } } - /* Because it's possible that name is owned by the layer and will be freed - * when freeing the layer, do these checks before freeing. */ + /* Update active and default color attributes. */ const bool is_active_color_attribute = name == StringRef(mesh->active_color_attribute); const bool is_default_color_attribute = name == StringRef(mesh->default_color_attribute); - if (BM_data_layer_free_named(em->bm, data, name)) { - if (is_active_color_attribute) { - MEM_SAFE_FREE(mesh->active_color_attribute); - } - else if (is_default_color_attribute) { - MEM_SAFE_FREE(mesh->default_color_attribute); - } - return true; + const int active_index = color_name_to_index(id, mesh->active_color_attribute); + const int default_index = color_name_to_index(id, mesh->default_color_attribute); + if (!BM_data_layer_free_named(em->bm, data, name)) { + return false; } + if (is_active_color_attribute) { + BKE_id_attributes_active_color_set( + id, color_name_from_index(id, color_clamp_index(id, active_index))); + } + if (is_default_color_attribute) { + BKE_id_attributes_default_color_set( + id, color_name_from_index(id, color_clamp_index(id, default_index))); + } + return true; } } return false; @@ -429,7 +453,6 @@ bool BKE_id_attribute_remove(ID *id, const char *name, ReportList *reports) } if (GS(id->name) == ID_ME) { - std::optional metadata = attributes->lookup_meta_data(name); if (metadata->data_type == CD_PROP_FLOAT2) { /* remove UV sub-attributes. */ @@ -438,6 +461,24 @@ bool BKE_id_attribute_remove(ID *id, const char *name, ReportList *reports) BKE_id_attribute_remove(id, BKE_uv_map_edge_select_name_get(name, buffer_src), reports); BKE_id_attribute_remove(id, BKE_uv_map_pin_name_get(name, buffer_src), reports); } + /* Update active and default color attributes. */ + Mesh *mesh = reinterpret_cast(id); + const bool is_active_color_attribute = name == StringRef(mesh->active_color_attribute); + const bool is_default_color_attribute = name == StringRef(mesh->default_color_attribute); + const int active_index = color_name_to_index(id, mesh->active_color_attribute); + const int default_index = color_name_to_index(id, mesh->default_color_attribute); + if (!attributes->remove(name)) { + return false; + } + if (is_active_color_attribute) { + BKE_id_attributes_active_color_set( + id, color_name_from_index(id, color_clamp_index(id, active_index))); + } + if (is_default_color_attribute) { + BKE_id_attributes_default_color_set( + id, color_name_from_index(id, color_clamp_index(id, default_index))); + } + return true; } return attributes->remove(name); diff --git a/source/blender/editors/geometry/geometry_attributes.cc b/source/blender/editors/geometry/geometry_attributes.cc index 516d67220cb..da88b5483d8 100644 --- a/source/blender/editors/geometry/geometry_attributes.cc +++ b/source/blender/editors/geometry/geometry_attributes.cc @@ -107,36 +107,6 @@ static int geometry_attribute_add_exec(bContext *C, wmOperator *op) return OPERATOR_FINISHED; } -static void next_color_attribute(ID *id, const StringRefNull name, bool is_render) -{ - const CustomDataLayer *layer = BKE_id_attributes_color_find(id, name.c_str()); - int index = BKE_id_attribute_to_index(id, layer, ATTR_DOMAIN_MASK_COLOR, CD_MASK_COLOR_ALL); - - index++; - - layer = BKE_id_attribute_from_index(id, index, ATTR_DOMAIN_MASK_COLOR, CD_MASK_COLOR_ALL); - - if (!layer) { - index = 0; - layer = BKE_id_attribute_from_index(id, index, ATTR_DOMAIN_MASK_COLOR, CD_MASK_COLOR_ALL); - } - - if (layer) { - if (is_render) { - BKE_id_attributes_active_color_set(id, layer->name); - } - else { - BKE_id_attributes_default_color_set(id, layer->name); - } - } -} - -static void next_color_attributes(ID *id, const StringRefNull name) -{ - next_color_attribute(id, name, false); /* active */ - next_color_attribute(id, name, true); /* render */ -} - void GEOMETRY_OT_attribute_add(wmOperatorType *ot) { /* identifiers */ @@ -182,8 +152,6 @@ static int geometry_attribute_remove_exec(bContext *C, wmOperator *op) ID *id = static_cast(ob->data); CustomDataLayer *layer = BKE_id_attributes_active_get(id); - next_color_attributes(id, layer->name); - if (!BKE_id_attribute_remove(id, layer->name, op->reports)) { return OPERATOR_CANCELLED; } @@ -436,8 +404,6 @@ static int geometry_color_attribute_remove_exec(bContext *C, wmOperator *op) return OPERATOR_CANCELLED; } - next_color_attributes(id, active_name); - if (!BKE_id_attribute_remove(id, active_name.c_str(), op->reports)) { return OPERATOR_CANCELLED; }