New numbering scheme in Sketcher?

[openBrain]

If you are linux based, and you are going to do this, install ccache.

I already have it. But poor CPU, and any upstream rebasing is almost a full compilation.

https://doc.qt.io/qt-5/qcolor.html#QColor-3

Good. I'll give a try to static fromRgba(). Will amend my commit if it's OK so if you're the one that merge, please wait another compilation time before proceeding.

But, now you are going to use it in more than one place. On code reusability and code maintainability, such a method is better. Code reusability and maintainability wins over readability.

That's it.

Please: QIcon getIcon(bool construction, bool external) const

Commit amended.

[openBrain]

@community : are you happy with new proposed behavior ?

iconColor.gif (48.56 KiB) Viewed 651 times

[abdullah]

Good. I'll give a try to static fromRgba(). Will amend my commit if it's OK so if you're the one that merge, please wait another compilation time before proceeding.

I am not sure that is necessary, as we are not using alpha channel...

This code:

Code: Select all

QIcon TaskSketcherElements::MultIcon::getIcon(bool construction, bool external) const
{
    if (construction == external) return Normal;
    if (construction) return Construction;
    if (external) return External;
    return Normal;
}

Saying that when the value of construction equals the value of external, then the normal icon should be used, has IMO code readability problems. It is unclear why having a same value should return Normal...unless you assume that they are never going to be both true at the same time. This impacts code maintenability.

Another argument is these cases is as follows: if the sketcher is modified to allow for external geometry that is construction and external geometry that is not construction, it would be unexpected that the neither the construction, nor the external icons are returned, but the normal icon. However, I do agree that in such a case this function would need to be changed anyway to match the new situation, so this argument is not very relevant to this specific case.

In cases like this, if you want to cover all the bases, it is relevant to think: what if that pesky user is really putting to true both parameters? what is the best behaviour for the user:
a) we make FC crash.
b) we raise an exception.
c) we just prioritise the results and give the one that we think is a better match.

What about something in the lines of:

Code: Select all

QIcon TaskSketcherElements::MultIcon::getIcon(bool construction, bool external) const
{
    if (construction) 
    	return Construction;
    
    if (external) 
    	return External;
    
    return Normal;
}

[openBrain]

Here we are. PR is ready for review.

: X-posting. I agree with post above. Let's go for a change.

[openBrain]

I am not sure that is necessary, as we are not using alpha channel...

I'll let it as it is in case transparency will be used in the future.
I made another proposal to solve the problem you raised on your above post.
Now PR should be final (I hope). Maybe you can comment on GH directly if you have further remarks. Thanks for your support on this.

[abdullah]

I am not sure that is necessary, as we are not using alpha channel...

I'll let it as it is in case transparency will be used in the future.
I made another proposal to solve the problem you raised on your above post.
Now PR should be final (I hope). Maybe you can comment on GH directly if you have further remarks. Thanks for your support on this.

setPixelColor is also Qt 5.6

Code: Select all

/home/travis/build/FreeCAD/FreeCAD/src/Mod/Sketcher/Gui/TaskSketcherElements.cpp:1126:31: error: ‘class QImage’ has no member named ‘setPixelColor’; did you mean ‘setColor’?

                     imgConstr.setPixelColor(ix, iy, clr);

                               ^~~~~~~~~~~~~

                               setColor

/home/travis/build/FreeCAD/FreeCAD/src/Mod/Sketcher/Gui/TaskSketcherElements.cpp:1129:28: error: ‘class QImage’ has no member named ‘setPixelColor’; did you mean ‘setColor’?

                     imgExt.setPixelColor(ix, iy, clr);

                            ^~~~~~~~~~~~~

                            setColor

You have:

void QImage::setPixel(const QPoint &position, uint index_or_rgb)

Sets the pixel index or color at the given position to index_or_rgb.

If the image's format is either monochrome or paletted, the given index_or_rgb value must be an index in the image's color table, otherwise the parameter must be a QRgb value.

If position is not a valid coordinate pair in the image, or if index_or_rgb >= colorCount() in the case of monochrome and paletted images, the result is undefined.

Warning: This function is expensive due to the call of the internal detach() function called within; if performance is a concern, we recommend the use of scanLine() or bits() to access pixel data directly.

See also pixel() and Pixel Manipulation.
void QImage::setPixel(int x, int y, uint index_or_rgb)

This is an overloaded function.

Sets the pixel index or color at (x, y) to index_or_rgb.

[openBrain]

setPixelColor is also Qt 5.6

I was silly to not check that... Fixed now in the PR.

When testing, I found something that we can probably improve later. Circle icon in Midpoint mode has no other color than green and white, so it is the same whatever the circle being a normal, construction or external one.

[abdullah]

I was silly to not check that... Fixed now in the PR.

Do not worry.

When testing, I found something that we can probably improve later. Circle icon in Midpoint mode has no other color than green and white, so it is the same whatever the circle being a normal, construction or external one.

You are right. We cannot provide appropriate construction/external icons using the algorithm we have now.

I wonder if could find an alternative way to modify the icons to mark construction/external that is applicable to all cases

[C_h_o_p_i_n]

I wonder if could find an alternative way to modify the icons to mark construction/external that is applicable to all cases

How about displaying a small "C" in blue or "E" in orange just beside the center point - like usually the type of a constraint ist displayed.

Kind regards,
Stefan

[openBrain]

How about displaying a small "C" in blue or "E" in orange just beside the center point - like usually the type of a constraint ist displayed.

Honestly I doesn't like it much. I think color tampering is more user friendly.

You are right. We cannot provide appropriate construction/external icons using the algorithm we have now.

I wonder if could find an alternative way to modify the icons to mark construction/external that is applicable to all cases

I pushed a new commit in the PR. It basically colors the edge part of the icon when a mode other than 'Edge' is selected. What do you think about that ?
Finally managing transparency was good.
Below how it looks now.

iconColor.gif (38.78 KiB) Viewed 518 times