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:
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("]");