Skip to content

Commit

Permalink
+ issue #2525: Line color of shapes are always black
Browse files Browse the repository at this point in the history
  • Loading branch information
@wwmayer
wwmayer committed May 6, 2016
1 parent 4eada9a commit 7e01d01
Showing 1 changed file with 3 additions and 1 deletion.
4 changes: 3 additions & 1 deletion src/Mod/Part/Gui/ViewProviderExt.cpp
View file
Expand Up @@ -350,17 +350,19 @@ void ViewProviderPartExt::onChanged(const App::Property* prop)
pcLineMaterial->diffuseColor.setValue(c.r,c.g,c.b);
if (c != LineMaterial.getValue().diffuseColor)
LineMaterial.setDiffuseColor(c);
LineColorArray.setValue(LineColor.getValue());
}
else if (prop == &PointColor) {
const App::Color& c = PointColor.getValue();
pcPointMaterial->diffuseColor.setValue(c.r,c.g,c.b);
if (c != PointMaterial.getValue().diffuseColor)
PointMaterial.setDiffuseColor(c);
PointColorArray.setValue(PointColor.getValue());
}
else if (prop == &LineMaterial) {
const App::Material& Mat = LineMaterial.getValue();
if (LineColor.getValue() != Mat.diffuseColor)
LineColor.setValue(Mat.diffuseColor);
LineColor.setValue(Mat.diffuseColor);
pcLineMaterial->ambientColor.setValue(Mat.ambientColor.r,Mat.ambientColor.g,Mat.ambientColor.b);
pcLineMaterial->diffuseColor.setValue(Mat.diffuseColor.r,Mat.diffuseColor.g,Mat.diffuseColor.b);
pcLineMaterial->specularColor.setValue(Mat.specularColor.r,Mat.specularColor.g,Mat.specularColor.b);
Expand Down

4 comments on commit 7e01d01

@ickby
Copy link
Contributor

@ickby ickby commented on 7e01d01 May 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LineColorArray is used to color individual edges. It was added for the temporary highlithing of edges for PartDesign features like fillet, to show the user which edges are selected. This functionality does not need any persistence, it is only shown temporary, hence a Property is IMHO the wrong way to achieve this. I think life becomes easier if we remove this property and implement the highlithing with some kind of temporary mode in the object with normal member variable as storage.

@ickby
Copy link
Contributor

@ickby ickby commented on 7e01d01 May 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the same temporary highlighting is needed for faces and Points, same issue as described for lines hold for those Properties too.

@wwmayer
Copy link
Contributor Author

@wwmayer wwmayer commented on 7e01d01 May 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This functionality does not need any persistence, it is only shown temporary, hence a Property is IMHO the wrong way to achieve this.

This can be easily done by declaring the property as "Transient". This attribute causes the property container (and sub-classes) to ignore the property when saving a document. On load time nothing happens because the LineColorArray won't be triggered.

Note that the same temporary highlighting is needed for faces and Points

Might be correct for points but it's definitely wrong for faces. There we already have the possibility to set individual colors for faces using the DiffuseColor property. For faces there is no second color property used for highlighting so that I guess DiffuseColor is used for this, too.

My actual goal is to reduce the (historically grown) different color properties. For a standard view provider we at the moment have: ShapeColor, ShapeMaterial and DiffuseColor to set color(s) on faces. For edges we have LineColor, LineMaterial and LineColorArray and for points we have PointColor, PointMaterial and PointColorArray.

At least the pairs (ShapeColor and ShapeMaterial), (LineColor and LineMaterial) and (PointColor and PointMaterial) can be replaced by one property for each. I therefore suggest the new property PropertyMaterialList because this gives us the full flexibility of the SoMaterial node.

We can still keep DiffuseColors, LineColorArray and PointColorArray but should rename them to FaceHightlight, LineHighlight and PointHighlight to make clear their purposes.

@wwmayer
Copy link
Contributor Author

@wwmayer wwmayer commented on 7e01d01 May 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a look at the code now to see how LineColorArray is used at the moment. It's always done like this:

  1. Save current stage of LineColorArray
  2. Change LineColorArray to highlight edges
  3. Restore saved stage of LineColorArray

OK, there is indeed no reason to have this implemented with properties. To highlight any sub-elements I would add a method e.g. hightlightFaces() which gets a list of colors or materials and a method unhighlightFaces() which internally triggers the property to restore the old colors.

So, this also means for faces, line and point we can have only one property for each.

Please sign in to comment.