cmlibs / zinc

Source code repository for OpenCMISS-Zinc
Mozilla Public License 2.0
15 stars 18 forks source link

Material module read description functionality #203

Closed hsorby closed 2 years ago

hsorby commented 2 years ago

The material module read description does not appear to merge materials. From what I can see it adds further materials of the same name. I think the desired behaviour would be to merge materials of the same name updating existing materials with the contents of the material described.

rchristie commented 2 years ago

I can't see how Zinc could be doing this. It's not possible to have 2 materials with the same name in the materialmodule's indexed list (implemented over std::set, using the name as the key). Furthermore, here is the merge function which shows it finds a material by name first:

int MaterialmoduleJsonImport::importMaterial(Json::Value &materialSettings)
{
    const char *materialName = materialSettings["Name"].asCString();
    Material material = this->materialmodule.findMaterialByName(materialName);
    if (!material.isValid())
    {
        material = this->materialmodule.createMaterial();
        material.setName(materialName);
    }
    Region rootRegion = this->materialmodule.getContext().getDefaultRegion();
    return readMaterialFromJson(material, materialSettings, rootRegion);
}

Finally, the test code already merges over the default and default selected materials without creating new ones -- the test confirms the correct number of materials exist afterwards.

hsorby commented 2 years ago

Okay, I'll have a look elsewhere. I didn't look too deeply into this I will admit.