BSpline weight in information layer

[abdullah]

Hi!

This thread is for PR 4027.



Thanks for the PR. I think it is a nice idea for the current version of FreeCAD. Let's see of we can find a solution for the alignment issue.

There is one thing I do not quite like and that produces bad visualisation in my system: the "alignment" of knot multiplicity and bspline weights using spaces.

Here the screenshot showing that the starting bracket intended for the weight information is not shown:

Screenshot_20201108_084134.png (47.16 KiB) Viewed 655 times

Screenshot_20201108_071911.png (7.43 KiB) Viewed 655 times

If a bpsline knot has a two digit multiplicity (not my type of bspline, but I guess it could happen), this will also result in even more interference between knot multiplicity information and bspline weight information.

On the positive side, I think it is the right decision to provide different SoText2 for the two pieces of information. See for example the periodic bspline that has totally different positions as the endpoints are not coincident with a pole. I think a solution may come from relocating the information at different places so that it does not interfere.

I am not that skillful with coin and generally struggle with these alignment issues. So maybe somebody has a good idea about how to best align these two pieces of information so that they do not interfere.

There are some minor things, mostly coming from copy and paste:
- Do not use itw as a name for an index, as it evokes an interator - it - and is misleading
- When using indices in a 'for loop', the comparison is best done with 'less than' instead of 'not equal to', in order to show intent that values inside the loop over size() are not intended and are undesirable. Yes, this is a different syntax than when using iterators and, yes, I agree that the assembly code may even be optimised to be the same. It is just about showing intent.
- to fix sign/unsign comparison warning with std::vector, use size_t for the index instead of int where possible (when you do not need negative indices for some reason).

You may consider this diff for the minor things (you can apply this as a patch if you wish, I do not need attribution):

Code: Select all

diff --git a/src/Mod/Sketcher/Gui/ViewProviderSketch.cpp b/src/Mod/Sketcher/Gui/ViewProviderSketch.cpp
index 000e4cc5f1..e0bb0e44af 100644
--- a/src/Mod/Sketcher/Gui/ViewProviderSketch.cpp
+++ b/src/Mod/Sketcher/Gui/ViewProviderSketch.cpp
@@ -4246,7 +4246,7 @@ void ViewProviderSketch::draw(bool temp /*=false*/, bool rebuildinformationlayer
         // pole weights --------------------------------------------------------
         std::vector<double> weights = spline->getWeights();
         QString WeightString;
-        int itw;
+        size_t index;^M
         // since the first and last control point of a spline is also treated as knot and thus
         // can also have a displayed multiplicity, we must assure the multiplicity is not visibly overwritten
         // we add spaces to show the weight behind the multiplicity
@@ -4254,7 +4254,7 @@ void ViewProviderSketch::draw(bool temp /*=false*/, bool rebuildinformationlayer
 
         if (rebuildinformationlayer) {
 
-            for (itw = 0; itw != weights.size(); ++itw) {
+            for (index = 0; index < weights.size(); ++index) {^M
 
                 SoSwitch* sw = new SoSwitch();
 
@@ -4272,7 +4272,7 @@ void ViewProviderSketch::draw(bool temp /*=false*/, bool rebuildinformationlayer
 
                 SoTranslation* translate = new SoTranslation;
 
-                Base::Vector3d poleposition = poles[itw];
+                Base::Vector3d poleposition = poles[index];^M
 
                 SoFont* font = new SoFont;
                 font->name.setValue("Helvetica");
@@ -4281,7 +4281,7 @@ void ViewProviderSketch::draw(bool temp /*=false*/, bool rebuildinformationlayer
                 translate->translation.setValue(poleposition.x, poleposition.y, zInfo);
 
                 // set up string with weight value and the user-defined number of decimals
-                WeightString  = QString::fromLatin1("%1").arg(weights[itw], 0, 'f', Base::UnitsApi::getDecimals());
+                WeightString  = QString::fromLatin1("%1").arg(weights[index], 0, 'f', Base::UnitsApi::getDecimals());^M
 
                 SoText2* WeightText = new SoText2;
                 WeightText->string = SbString(TextBegin) + SbString(WeightString.toStdString().c_str()) + SbString("]");
@@ -4301,7 +4301,7 @@ void ViewProviderSketch::draw(bool temp /*=false*/, bool rebuildinformationlayer
             }
         }
         else {
-            for (itw = 0; itw != weights.size(); ++itw) {
+            for (index = 0; index < weights.size(); ++index) {^M
                 SoSwitch* sw = static_cast<SoSwitch*>(edit->infoGroup->getChild(currentInfoNode));
 
                 if (visibleInformationChanged)
@@ -4309,12 +4309,12 @@ void ViewProviderSketch::draw(bool temp /*=false*/, bool rebuildinformationlayer
 
                 SoSeparator* sep = static_cast<SoSeparator*>(sw->getChild(0));
 
-                Base::Vector3d poleposition = poles[itw];
+                Base::Vector3d poleposition = poles[index];^M
 
                 static_cast<SoTranslation*>(sep->getChild(GEOINFO_BSPLINE_DEGREE_POS))
                     ->translation.setValue(poleposition.x, poleposition.y, zInfo);
 
-                WeightString = QString::fromLatin1("%1").arg(weights[itw], 0, 'f', Base::UnitsApi::getDecimals());
+                WeightString = QString::fromLatin1("%1").arg(weights[index], 0, 'f', Base::UnitsApi::getDecimals());^M
 
                 static_cast<SoText2*>(sep->getChild(GEOINFO_BSPLINE_DEGREE_TEXT))
                                         ->string = SbString(TextBegin) + SbString(WeightString.toStdString().c_str()) + SbString("]");

The rest looks ok. Let's see if we can find a better way to align that information.

[wmayer]

Another minor thing: move the declaration of itw (or better index) into the for loop and do not declare it outside.

Code: Select all

for (size_t index = 0; index < weights.size(); ++index)

[Chris_G]

I am not that skillful with coin and generally struggle with these alignment issues. So maybe somebody has a good idea about how to best align these two pieces of information so that they do not interfere.

A simple solution could be to put this text at the same location as the multiplicity, but on the second line :

Code: Select all

SoMultText.string.setValues(0,1,["Multiplicity"])
SoWeightText.string.setValues(0,2,["","Weight"])

[abdullah]

Another minor thing: move the declaration of itw (or better index) into the for loop and do not declare it outside.

Code: Select all

for (size_t index = 0; index < weights.size(); ++index)

Indeed. I am a little bit rusty...

[abdullah]

A simple solution could be to put this text at the same location as the multiplicity, but on the second line :

Code: Select all

SoMultText.string.setValues(0,1,["Multiplicity"])
SoWeightText.string.setValues(0,2,["","Weight"])

I thought of this solution, but Multiplicity and Weight are apart in a periodic b-spline.

Screenshot_20201108_151519.png (12.07 KiB) Viewed 550 times

[Chris_G]

The idea was to use the same location computation for the Weight-text node, relative to the circle center, that is used for the Mult-text node, relative to the knot-point.
And putting the weight text on the second line, puts it in a safe location, in case of a knot / pole coincidence.
But it will still work otherwise, when there is no text to display above weight.

[abdullah]

The idea was to use the same location computation for the Weight-text node, relative to the circle center, that is used for the Mult-text node, relative to the knot-point.
And putting the weight text on the second line, puts it in a safe location, in case of a knot / pole coincidence.
But it will still work otherwise, when there is no text to display above weight.

Indeed. After replying it was not possible, I coded it

Screenshot_20201108_154804.png (17.53 KiB) Viewed 533 times

[abdullah]

Ok. I merged the PR and then push the changes. Thank you.

[uwestoehr]

And putting the weight text on the second line

This what I was trying to achieve but a simple newline did not work. Now Abdullah implemented it using the second line of a label and this output was my initial plan. Thanks.

Here is an additional small cleanup PR: https://github.com/FreeCAD/FreeCAD/pull/4030

[abdullah]

And putting the weight text on the second line

This what I was trying to achieve but a simple newline did not work. Now Abdullah implemented it using the second line of a label and this output was my initial plan. Thanks.

Here is an additional small cleanup PR: https://github.com/FreeCAD/FreeCAD/pull/4030

Merged!