MAYA-126483 Fix a bug where materials got created in the wrong scope. #2783
Conversation
When using "Assign New Material", a new material gets created in the default materials scope. This scope is usually called "mtl". In the previous implementation, there was a bug where materials got created in *any* scope that starts with "mtl". If, for example, there was a scope called "mtlFoo", and no scope called "mtl", the new material got created in "mtlFoo". However, the expected behaviour in this example is to create a scope called "mtl" and to create the new material in there. This is the expected behaviour in a more general sense: If the default materials scope name (usually "mtl") is available, create the new material in a scope with this name. If the default materials scope name is not available i.e., there is already something that is not of type "Scope" that is using that name (e.g. an Xform called "mtl"), append a number suffix to the default materials scope name (e.g. "mtl1") and try to use that. If this name is not available either, increment the number suffix until an available name is found.
| // | ||
| // 5. Bind the material to all selected primitives in the stage: | ||
| // | ||
| for (const auto& parentPath : selectedPaths) { |
There was a problem hiding this comment.
I moved steps 1 through 4 out of the inner loop. These steps were only executed in the first iteration of the inner loop anyway, so moving them out reduces unnecessary indentation and should improve readability.
| Ufe::SceneItem::Ptr materialsScope = nullptr; | ||
| std::string materialsScopeName = materialsScopeNamePrefix; | ||
| Ufe::SceneItemList children = stageHierarchy->children(); | ||
| for (size_t i = 1;; ++i) { |
There was a problem hiding this comment.
My only concern is that somehow this loop will go beyond then end of the children list. Is that possible?
There was a problem hiding this comment.
It is not used to index the child list. Only for adding a digit to the end of the search string for the std::find_if.
The worry is a scene with 5000 XForms named mtl1 to mtl4999 as the search at line 341 will make this quadratic. Building a std::unordered_map of the children, keyed on the name would keep this linear, but the worst case is so unlikely I am not sure I would mandate this.
frohnej-adsk
added
the
ready-for-merge
When using "Assign New Material", a new material gets created in the default materials scope. This scope is usually called "mtl". In the previous implementation, there was a bug where materials got created in any scope that starts with "mtl". If, for example, there was a scope called "mtlFoo", and no scope called "mtl", the new material got created in "mtlFoo". However, the expected behaviour in this example is to create a scope called "mtl" and to create the new material in there.
This is the expected behaviour in a more general sense: If the default materials scope name (usually "mtl") is available, create the new material in a scope with this name. If the default materials scope name is not available i.e., there is already something that is not of type "Scope" that is using that name (e.g. an Xform called "mtl"), append a number suffix to the default materials scope name (e.g. "mtl1") and try to use that. If this name is not available either, increment the number suffix until an available name is found.
This PDF contains a list of examples to illustrate the expected behaviour: materialScopesAcceptanceCriteria.pdf