Fix #107633: USD import: duplicate shader nodes #107945

Merged

Michael Kowalski merged 4 commits from makowalski/blender:usd-import-duplicate-shaders into main

2023-05-16 17:46:32 +02:00

Michael Kowalski commented

2023-05-15 18:55:58 +02:00

Member

This pull request addresses bug #107633

Added logic to avoid creating duplicate Blender shader nodes when
converting USD Preview Surface shaders that are connected to more
than one input.

This change adds a NodePlacementContext::node_cache map to record
Blender nodes converted for given USD shaders. The node creation
logic has been updated to query this cache to check if a node was
previously created for a given USD shader, before creating a
new node.

This pull request addresses bug https://projects.blender.org/blender/blender/issues/107633 Added logic to avoid creating duplicate Blender shader nodes when converting USD Preview Surface shaders that are connected to more than one input. This change adds a `NodePlacementContext::node_cache` map to record Blender nodes converted for given USD shaders. The node creation logic has been updated to query this cache to check if a node was previously created for a given USD shader, before creating a new node.

Michael Kowalski added the

Pipeline, Assets & IO

labels 2023-05-15 18:55:58 +02:00

Michael Kowalski added 1 commit 2023-05-15 18:56:08 +02:00

4e1f5cd701 USD import: Fix duplicate shader nodes

Added logic to avoid creating duplicate Blender shader nodes when
converting USD Preview Surface shaders that are connected to more
than one input.

This change adds a `NodePlacementContext::node_cache` map to record
Blender nodes converted for given USD shaders.  The node creation
logic has been updated to query this cache to check if a node was
previously created for a given USD shader, before creating a
new node.

Michael Kowalski added this to the USD project 2023-05-15 18:56:33 +02:00

Michael Kowalski requested review from Matt McLin 2023-05-15 18:56:51 +02:00

Michael Kowalski requested review from Jesse Yurkovich 2023-05-15 18:57:21 +02:00

Michael Kowalski commented

2023-05-15 19:02:34 +02:00

Author

Member

To test, import the attached test_usd_preview_alpha.usdz and verify that the Image Texture and UV Map nodes in the imported shader network are not duplicated.

To test, import the attached `test_usd_preview_alpha.usdz` and verify that the `Image Texture` and `UV Map` nodes in the imported shader network are not duplicated.

test_usd_preview_surface_alpha.usdz

98 KiB

Hans Goudey reviewed 2023-05-15 19:03:19 +02:00

source/blender/io/usd/intern/usd_reader_material.h Outdated

						
				@ -15,6 +15,8 @@ struct bNodeTree;

				namespace blender::io::usd {

				typedef std::map<pxr::SdfPath, bNode *> ShaderToNodeMap;

Hans Goudey commented

2023-05-15 19:01:50 +02:00

Member

For Blender code it's preferred to use our own container types, Map in this case (https://wiki.blender.org/wiki/Style_Guide/C_Cpp#C.2B.2B_Containers). Also, using is preferred over typedef

For Blender code it's preferred to use our own container types, `Map` in this case (https://wiki.blender.org/wiki/Style_Guide/C_Cpp#C.2B.2B_Containers). Also, `using` is preferred over `typedef`

Michael Kowalski commented

2023-05-15 21:02:39 +02:00

Author

Member

Thanks for the helpful suggestions, @HooglyBoogly. I made the requested changes. As part of the change to use blender::Map, I'm now using std::string, rather than pxr::SdfPath, for the map key, to avoid a compiler error because pxr::SdfPath doesn't have a hash function defined.

Thanks for the helpful suggestions, @HooglyBoogly. I made the requested changes. As part of the change to use `blender::Map`, I'm now using `std::string`, rather than `pxr::SdfPath`, for the map key, to avoid a compiler error because `pxr::SdfPath` doesn't have a hash function defined.

Hans Goudey commented

2023-05-15 21:13:38 +02:00

Member

Just so you know, the key type doesn't have to have a .hash() member function, it can also be provided manually outside of the class. This is documented in BLI_hash.hh, in the section after There are three main ways to provide a hash table implementation with a custom hash function

Just so you know, the key type doesn't have to have a `.hash()` member function, it can also be provided manually outside of the class. This is documented in `BLI_hash.hh`, in the section after `There are three main ways to provide a hash table implementation with a custom hash function`

HooglyBoogly marked this conversation as resolved

Iliya Katushenock added the

Nodes & Physics

label 2023-05-15 19:04:57 +02:00

Michael Kowalski added 1 commit 2023-05-15 20:57:06 +02:00

9135003c96 USD material import: Use blender::Map.

Changed code to use blender::Map instead of
std::map, to conform with current coding guidelines.

Michael Kowalski commented

2023-05-16 15:51:50 +02:00

Author

Member

@HooglyBoogly I'd like to merge this for 3.6 by the deadline today, if possible. Would you be available to approve this pull request, by any chance? No worries if you can't.

@HooglyBoogly I'd like to merge this for 3.6 by the deadline today, if possible. Would you be available to approve this pull request, by any chance? No worries if you can't.

Hans Goudey approved these changes 2023-05-16 15:56:17 +02:00

Hans Goudey left a comment

Member

The code looks reasonable to me. I haven't tested it though.

The code looks reasonable to me. I haven't tested it though.

source/blender/io/usd/intern/usd_reader_material.cc Outdated

						
				@ -87,0 +93,4 @@

				                              const pxr::UsdShadeShader &usd_shader)

				{

				  const std::string path_str = usd_shader.GetPath().GetAsString();

				  if (node_cache.contains(path_str)) {

Hans Goudey commented

2023-05-16 15:53:07 +02:00

Member

Better to use lookup_ptr here, to avoid the double lookup

Better to use `lookup_ptr` here, to avoid the double lookup

Michael Kowalski commented

2023-05-16 17:14:09 +02:00

Author

Member

I just updated the code to use lookup_ptr. Thanks for pointing this out.

I just updated the code to use `lookup_ptr`. Thanks for pointing this out.

makowalski marked this conversation as resolved

Michael Kowalski added 2 commits 2023-05-16 17:13:00 +02:00

e0c5c2b976 Merge branch 'main' into usd-import-duplicate-shaders

buildbot/vexp-code-patch-coordinator Build done. Details

6d8f7ce7fe USD import: call Map::lookup_ptr().

Now calling Map::lookup_ptr() to avoid double
lookup.

Michael Kowalski commented

2023-05-16 17:14:58 +02:00

Author

Member

@blender-bot build

@blender-bot build

Michael Kowalski merged commit 74b5e62d2a into main

2023-05-16 17:46:32 +02:00

Michael Kowalski deleted branch usd-import-duplicate-shaders

2023-05-16 17:46:35 +02:00

Michael Kowalski referenced this issue from a commit

2023-05-16 17:46:41 +02:00

Fix #107633: USD import: duplicate shader nodes

Matt McLin commented

2023-05-16 18:06:34 +02:00

Member

Sorry I was late in my review, but fyi, I did test this yesterday with a more complicated shader graph and it worked well.

Sorry I was late in my review, but fyi, I did test this yesterday with a more complicated shader graph and it worked well.

Michael Kowalski commented

2023-05-16 18:09:08 +02:00

Author

Member

Sorry I was late in my review, but fyi, I did test this yesterday with a more complicated shader graph and it worked well.

No worries at all about any delay, and thanks for testing!

> Sorry I was late in my review, but fyi, I did test this yesterday with a more complicated shader graph and it worked well. No worries at all about any delay, and thanks for testing!

Falk David referenced this issue from a commit

2023-05-23 14:35:33 +02:00

Fix #107633: USD import: duplicate shader nodes

Howard Trickey referenced this issue from a commit

2023-05-29 02:51:39 +02:00

Merge in orign/main:

Bastien Montagne removed this from the USD project 2023-08-25 23:55:07 +02:00

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

3 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#107945

No description provided.