From 8abb9f0128795413c8686d169e6d98c77f6b73f0 Mon Sep 17 00:00:00 2001 From: David Carter <38090157+davesrocketshop@users.noreply.github.com> Date: Mon, 5 May 2025 16:31:25 +0000 Subject: [PATCH] Materials: Default units (#20909) Improves handling and assignment of default units When creating a material without specifying units, no units are currently assigned. This commit will assign the default units when none are given, or throw an error when incompatible units are given. In the latter case, the units are set to the property defaults. This commit also incidentally fixed an issue when saving the material that resulted in accessing an uninitialized pointer. --- src/Mod/Material/App/MaterialLoader.cpp | 20 +++++++++++-- src/Mod/Material/App/Materials.cpp | 29 ++++++++++++++++++- src/Mod/Material/Gui/MaterialDelegate.cpp | 28 +++++++++++++++++- src/Mod/Material/Gui/MaterialSave.cpp | 11 +++---- src/Mod/Material/Gui/ModelSelect.cpp | 6 +++- .../materialtests/TestMaterialCreation.py | 8 +++++ 6 files changed, 90 insertions(+), 12 deletions(-) diff --git a/src/Mod/Material/App/MaterialLoader.cpp b/src/Mod/Material/App/MaterialLoader.cpp index 7bdc4a881f..19f30c5a00 100644 --- a/src/Mod/Material/App/MaterialLoader.cpp +++ b/src/Mod/Material/App/MaterialLoader.cpp @@ -246,8 +246,24 @@ void MaterialYamlEntry::addToTree( propertyValue = propertyValue.remove( QRegularExpression(QStringLiteral("[\r\n]"))); } - finalModel->setPhysicalValue(QString::fromStdString(propertyName), - propertyValue); + try { + finalModel->setPhysicalValue(QString::fromStdString(propertyName), + propertyValue); + } + catch (const Base::ValueError& e) { + // Units mismatch + Base::Console().Log("Units mismatch in material '%s':'%s' = '%s', " + "setting to default property units '%s'\n", + name.toStdString().c_str(), + propertyName, + propertyValue.toStdString().c_str(), + prop->getUnits().toStdString().c_str()); + auto quantity = Base::Quantity::parse(propertyValue.toStdString()); + finalModel->setPhysicalValue( + QString::fromStdString(propertyName), + Base::Quantity(quantity.getValue(), + prop->getUnits().toStdString())); + } } } catch (const YAML::BadConversion& e) { diff --git a/src/Mod/Material/App/Materials.cpp b/src/Mod/Material/App/Materials.cpp index 90ad18b073..bc5ee18a9d 100644 --- a/src/Mod/Material/App/Materials.cpp +++ b/src/Mod/Material/App/Materials.cpp @@ -283,7 +283,20 @@ QVariant MaterialProperty::getColumnNull(int column) const void MaterialProperty::setValue(const QVariant& value) { - _valuePtr->setValue(value); + if (_valuePtr->getType() == MaterialValue::Quantity && value.canConvert()) { + // Ensure the units are set correctly + auto quantity = value.value(); + if (quantity.isValid()) { + setQuantity(quantity); + } + else { + // Set a default value with default units + setValue(QStringLiteral("0")); + } + } + else { + _valuePtr->setValue(value); + } } void MaterialProperty::setValue(const QString& value) @@ -386,6 +399,20 @@ void MaterialProperty::setFloat(const QString& value) void MaterialProperty::setQuantity(const Base::Quantity& value) { auto quantity = value; + if (quantity.isDimensionless()) { + // Assign the default units when none are provided. + // + // This needs to be parsed rather than just setting units. Otherwise we get mm->m conversion + // errors, etc + quantity = Base::Quantity::parse(quantity.getUserString() + getUnits().toStdString()); + } + else { + auto propertyUnit = Base::Quantity::parse(getUnits().toStdString()).getUnit(); + auto units = quantity.getUnit(); + if (propertyUnit != units) { + throw Base::ValueError("Incompatible material units"); + } + } quantity.setFormat(MaterialValue::getQuantityFormat()); _valuePtr->setValue(QVariant(QVariant::fromValue(quantity))); } diff --git a/src/Mod/Material/Gui/MaterialDelegate.cpp b/src/Mod/Material/Gui/MaterialDelegate.cpp index ad36b74462..e3d32c5997 100644 --- a/src/Mod/Material/Gui/MaterialDelegate.cpp +++ b/src/Mod/Material/Gui/MaterialDelegate.cpp @@ -25,6 +25,7 @@ #include #include #include +#include #include #include #include @@ -143,7 +144,32 @@ void MaterialDelegate::setValue(QAbstractItemModel* model, auto propertyName = group->child(row, 0)->data().toString(); std::string _name = propertyName.toStdString(); auto property = material->getProperty(propertyName); - property->setValue(value); + + try { + property->setValue(value); + } + catch (const Base::ValueError& e) { + // Units mismatch + auto quantity = value.value(); + Base::Console().Log("Units mismatch '%s' = '%s', " + "setting to default property units '%s'\n", + propertyName.toStdString().c_str(), + quantity.getUserString().c_str(), + property->getUnits().toStdString().c_str()); + + QMessageBox msgBox; + msgBox.setWindowTitle(QStringLiteral("Property units mismatch")); + msgBox.setText(QStringLiteral("Units mismatch '%1' = '%2', " + "setting to default property units '%3'\n") + .arg(propertyName) + .arg(QString::fromStdString(quantity.getUserString())) + .arg(property->getUnits())); + msgBox.exec(); + + property->setQuantity( + Base::Quantity(quantity.getValue(), property->getUnits().toStdString())); + } + group->child(row, 1)->setText(property->getString()); } diff --git a/src/Mod/Material/Gui/MaterialSave.cpp b/src/Mod/Material/Gui/MaterialSave.cpp index 6aca99c381..6674baeae7 100644 --- a/src/Mod/Material/Gui/MaterialSave.cpp +++ b/src/Mod/Material/Gui/MaterialSave.cpp @@ -291,12 +291,10 @@ void MaterialSave::setLibraries() auto libraries = Materials::MaterialManager::getManager().getLibraries(); for (auto& library : *libraries) { if (library->isLocal()) { - auto materialLibrary = - reinterpret_cast&>(library); - if (!materialLibrary->isReadOnly()) { + if (!library->isReadOnly()) { QVariant libraryVariant; - libraryVariant.setValue(materialLibrary); - ui->comboLibrary->addItem(materialLibrary->getName(), libraryVariant); + libraryVariant.setValue(library); + ui->comboLibrary->addItem(library->getName(), libraryVariant); } } } @@ -333,8 +331,7 @@ void MaterialSave::addMaterials( for (auto& mat : *modelTree) { std::shared_ptr nodePtr = mat.second; if (nodePtr->getType() == Materials::MaterialTreeNode::NodeType::DataNode) { - std::shared_ptr material = nodePtr->getData(); - QString uuid = material->getUUID(); + QString uuid = nodePtr->getUUID(); auto card = new QStandardItem(icon, mat.first); card->setFlags(Qt::ItemIsSelectable | Qt::ItemIsEnabled | Qt::ItemIsDragEnabled diff --git a/src/Mod/Material/Gui/ModelSelect.cpp b/src/Mod/Material/Gui/ModelSelect.cpp index 4f43325223..d2d7723824 100644 --- a/src/Mod/Material/Gui/ModelSelect.cpp +++ b/src/Mod/Material/Gui/ModelSelect.cpp @@ -240,8 +240,12 @@ void ModelSelect::addModels( for (auto& mod : *modelTree) { std::shared_ptr nodePtr = mod.second; if (nodePtr->getType() == Materials::ModelTreeNode::NodeType::DataNode) { + QString uuid = nodePtr->getUUID(); auto model = nodePtr->getData(); - QString uuid = model->getUUID(); + if (!model) { + model = Materials::ModelManager::getManager().getModel(uuid); + nodePtr->setData(model); + } auto card = new QStandardItem(icon, model->getName()); card->setFlags(Qt::ItemIsSelectable | Qt::ItemIsEnabled | Qt::ItemIsDragEnabled diff --git a/src/Mod/Material/materialtests/TestMaterialCreation.py b/src/Mod/Material/materialtests/TestMaterialCreation.py index 296f772f21..9febd43c94 100644 --- a/src/Mod/Material/materialtests/TestMaterialCreation.py +++ b/src/Mod/Material/materialtests/TestMaterialCreation.py @@ -130,6 +130,14 @@ class MaterialCreationTestCases(unittest.TestCase): self.assertTrue(material.hasPhysicalModel(self.uuids.Density)) # Quantity properties require units + with self.assertRaises(ValueError): + # Units of mass not density + material.setPhysicalValue("Density", "99.9 kg") + + material.setPhysicalValue("Density", "99.9") + self.assertEqual(material.getPhysicalValue("Density").Format["NumberFormat"], "g") + self.assertEqual(material.getPhysicalValue("Density").UserString, self.getQuantity("99.90 kg/m^3").UserString) + material.setPhysicalValue("Density", "99.9 kg/m^3") self.assertEqual(material.getPhysicalValue("Density").Format["NumberFormat"], "g") self.assertEqual(material.getPhysicalValue("Density").UserString, self.getQuantity("99.90 kg/m^3").UserString)