Material: Appearance Updates 2 #13792

davesrocketshop · 2024-05-02T14:50:30Z

Improves the use of the ShapeAppearance property for the Part workbench.

removes DiffuseColor property
- adds Python compatibility using custom attributes
- transitions DiffuseColor to ShapeAppearance on open
Improved UI elements for setting object appearance, and appearance per face
Lays the foundation for future texture support

maxwxyz · 2024-05-03T15:10:15Z

with this PR, can we get rid of the default diffuse color, ambient color,... introduced in the settings a few days ago?

davesrocketshop · 2024-05-03T15:43:21Z

with this PR, can we get rid of the default diffuse color, ambient color,... introduced in the settings a few days ago?

No. They're still used. The set the default appearance

maxwxyz · 2024-05-03T15:45:10Z

why not use the default material there instead of hacking it?

davesrocketshop · 2024-05-03T15:47:59Z

why not use the default material there instead of hacking it?

This would make sense for objects in part and part design, but less so for line colors, draft workbench, etc. The appearance can be used where there is no associated shape property

maxwxyz · 2024-05-03T15:51:00Z

yes, I'm talking about the material appearance in the Part / Part Design preferences:

Line/Vertex color could stay. But the rest could be just using the default material or a material chooser or maybe launch the material editor to adjust it. Do these settings currently overwrite the default material or is this just a proxy and no material is applied?

davesrocketshop · 2024-05-03T15:55:16Z

yes, I'm talking about the material appearance in the Part / Part Design preferences:

Line/Vertex color could stay. But the rest could be just using the default material or a material chooser or maybe launch the material editor to adjust it. Do these settings currently overwrite the default material or is this just a proxy and no material is applied?

BTW, I'm defending someone else's code here, but yes. It does override the default material allowing you to customize what that default looks like. This is similar to specifying a custom appearance in the "Set appearance..." dialog only without the pop up.

Could it be done with the MaterialTreeWidget? Yes, but there would still need a separate dialog to specify a custom value. It works fine as shown.

maxwxyz · 2024-05-03T15:57:49Z

I know it was introduced with another PR. But the settings there do not overwrite / save my default material, right? Does a new object then have a material with the settings in the preferences and where is that stored?

davesrocketshop · 2024-05-03T15:58:54Z

I know it was introduced with another PR. But the settings there do not overwrite / save my default material, right? Does a new object then have a material with the settings in the preferences and where is that stored?

Not yet. Work in progress

davesrocketshop · 2024-05-06T15:45:02Z

Hold off on merging this... I'm reviewing a potential issue.

adrianinsaval · 2024-05-06T16:33:21Z

please mark again as ready for review once ready

yorikvanhavre · 2024-05-06T16:34:21Z

FYI @davesrocketshop when you don't want a PR to be merged, a good thing to do is to turn it back into draft

Improves the use of the ShapeAppearance property for the Part workbench. removes DiffuseColor property adds Python compatibility using custom attributes transitions DiffuseColor to ShapeAppearance on open Improved UI elements for setting object appearance, and appearance per face Lays the foundation for future texture support

chennes

Mostly just questions and little stuff.

chennes · 2024-05-16T19:09:18Z

src/App/Material.cpp

+
+long toPercent(float value)
+{
+ return static_cast<long>(100.0 * value + 0.5);


The linter is (almost) right here, this code should use std::roundl().

This is plagiarized code. I'll update.

chennes · 2024-05-16T19:11:12Z

src/App/PropertyStandard.cpp

+ aboutToSetValue();
+ setSize(colors.size(), _lValueList[0]);
+
+ for (int i = 0; i < colors.size(); i++) {


Any reason not to use range-for here?

chennes · 2024-05-16T19:18:20Z

src/Gui/DlgMaterialPropertiesImp.cpp

+ if (updateColors) {
+ App::Color ambient = getColor(ui->ambientColor->color());

- for (std::vector<ViewProvider*>::iterator it= Objects.begin(); it != Objects.end(); ++it) {
- App::Property* prop = (*it)->getPropertyByName(material.c_str());
- if (prop && prop->isDerivedFrom<App::PropertyMaterial>()) {
- auto ShapeMaterial = static_cast<App::PropertyMaterial*>(prop);
- App::Material mat = ShapeMaterial->getValue();
- mat.ambientColor = ambient;
- ShapeMaterial->setValue(mat);
+ for (std::vector<ViewProvider*>::iterator it= Objects.begin(); it != Objects.end(); ++it) {
+ App::Property* prop = (*it)->getPropertyByName(material.c_str());
+ if (prop && prop->isDerivedFrom<App::PropertyMaterialList>()) {
+ auto ShapeAppearance = static_cast<App::PropertyMaterialList*>(prop);
+ auto mat = getMaterial();
+ mat.ambientColor = ambient;
+ ShapeAppearance->setValue(mat);
+ }


This is a lot of boilerplate repeated a half dozen times with only a single line change -- in the future it would be a good idea to refactor to eliminate that code duplication (and to modernize the for loop).

src/Gui/DlgMaterialPropertiesImp.cpp

src/Gui/ViewProviderGeometryObject.cpp

chennes · 2024-05-16T19:40:19Z

src/Mod/Part/Gui/TaskFaceAppearances.cpp

+ // if (current.empty())
+ //  current.push_back(vp->ShapeAppearance.getDiffuseColor());


Just delete?

chennes · 2024-05-16T19:41:53Z

src/Mod/Part/Gui/TaskFaceAppearances.cpp

+ connect(d->ui->buttonCustomAppearance,
+ &QPushButton::clicked,
+ this,
+ &FaceAppearances::onbuttonCustomAppearanceClicked);


I'd capitalize the "B" in onbuttonCustomAppearanceClicked here, it jumps out as an error to a reader otherwise.

chennes · 2024-05-16T19:45:00Z

src/Mod/Part/Gui/ViewProviderBoolean.cpp

- std::vector<App::Color> colBool;
- colBool.resize(boolMap.Extent(), this->ShapeAppearance.getDiffuseColor());
+ std::vector<App::Material> colBool;
+ colBool.resize(boolMap.Extent(), this->ShapeAppearance[0]);


No need to check size first here?

No. The PropertyMaterialList code ensures there's always at least one entry

chennes · 2024-05-16T19:46:28Z

src/Mod/Part/Gui/ViewProviderExt.cpp

@@ -175,7 +177,7 @@ ViewProviderPartExt::ViewProviderPartExt()
 ADD_PROPERTY_TYPE(LineColor, (lmat.diffuseColor), osgroup, App::Prop_None, "Set object line color.");
 ADD_PROPERTY_TYPE(PointColor, (vmat.diffuseColor), osgroup, App::Prop_None, "Set object point color");
 ADD_PROPERTY_TYPE(PointColorArray, (PointColor.getValue()), osgroup, App::Prop_None, "Object point color array.");
- ADD_PROPERTY_TYPE(DiffuseColor,(ShapeAppearance.getDiffuseColors()), osgroup, App::Prop_None, "Object diffuse color.");
+ // ADD_PROPERTY_TYPE(DiffuseColor,(ShapeAppearance.getDiffuseColors()), osgroup, App::Prop_None, "Object diffuse color.");


If you're going to leave this here I'd also leave a comment about why

chennes · 2024-05-16T19:52:09Z

src/Mod/Part/Gui/ViewProviderMirror.cpp

- else if (!colBase.empty() && colBase[0] != this->ShapeAppearance.getDiffuseColor()) {
- colBase.resize(baseMap.Extent(), colBase[0]);
- applyColor(hist[0], colBase, colCham);
+ else if (colBase.getSize() > 0 && colBase[0] != this->ShapeAppearance[0]) {


Why change to getSize() > 0? I typically advise the opposite change (though I wasn't going to bother suggesting it in this PR).

Co-authored-by: Chris Hennes <[email protected]>

github-actions bot added Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD WB FEM Related to the FEM Workbench WB Part Design Related to the Part Design Workbench WB Part Related to the Part Workbench Materials Materials related WB MeshPart labels May 2, 2024

adrianinsaval marked this pull request as draft May 6, 2024 16:33

davesrocketshop force-pushed the material_appearance_pr branch 2 times, most recently from 755edcc to dcc315a Compare May 12, 2024 06:39

davesrocketshop marked this pull request as ready for review May 12, 2024 11:14

chennes self-requested a review May 13, 2024 16:08

davesrocketshop force-pushed the material_appearance_pr branch from dcc315a to 542cd63 Compare May 14, 2024 15:04

chennes reviewed May 16, 2024

View reviewed changes

Update src/Gui/DlgMaterialPropertiesImp.cpp

49e3e66

Co-authored-by: Chris Hennes <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Material: Appearance Updates 2 #13792

Material: Appearance Updates 2 #13792

davesrocketshop commented May 2, 2024 •

edited

maxwxyz commented May 3, 2024

davesrocketshop commented May 3, 2024

maxwxyz commented May 3, 2024

davesrocketshop commented May 3, 2024

maxwxyz commented May 3, 2024

davesrocketshop commented May 3, 2024

maxwxyz commented May 3, 2024

davesrocketshop commented May 3, 2024

davesrocketshop commented May 6, 2024

adrianinsaval commented May 6, 2024

yorikvanhavre commented May 6, 2024

chennes left a comment

chennes May 16, 2024

davesrocketshop May 17, 2024

chennes May 16, 2024

chennes May 16, 2024

chennes May 16, 2024

chennes May 16, 2024

chennes May 16, 2024

davesrocketshop May 17, 2024

chennes May 16, 2024

chennes May 16, 2024

		// if (current.empty())
		// current.push_back(vp->ShapeAppearance.getDiffuseColor());

Material: Appearance Updates 2 #13792

Are you sure you want to change the base?

Material: Appearance Updates 2 #13792

Conversation

davesrocketshop commented May 2, 2024 • edited

maxwxyz commented May 3, 2024

davesrocketshop commented May 3, 2024

maxwxyz commented May 3, 2024

davesrocketshop commented May 3, 2024

maxwxyz commented May 3, 2024

davesrocketshop commented May 3, 2024

maxwxyz commented May 3, 2024

davesrocketshop commented May 3, 2024

davesrocketshop commented May 6, 2024

adrianinsaval commented May 6, 2024

yorikvanhavre commented May 6, 2024

chennes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davesrocketshop commented May 2, 2024 •

edited