[Helper] A static alternative to OptionsGroup: SelectableItem

[alxbilger]

SelectableItem is a new type that can replace OptionsGroup in some cases. The idea is the same: a list of keys + a selection of a key among the list. In OptionsGroup, everything is dynamic, whereas SelectableItem is designed to be static.

Disadvantages of OptionsGroup:

OptionsGroup foo{{"optionA", "optionB"}};
foo.setSelectedItem(12); //out of bounds
foo.setSelectedItem("optionC"); //this key does not exist

if (foo.getSelectedId() == 0) // 0 has no meaning here. We don't know what 0
    // refers to. In addition, the order of the options can change
{
    
}

In SelectableItem:

• The list of items is included in the type and cannot change:

struct TestSelectableItem final : sofa::helper::SelectableItem<TestSelectableItem>
{
    static constexpr std::array s_items {
        sofa::helper::Item{"small", "small displacements"},
        sofa::helper::Item{"large", "large displacements"},
    };
};

The consequences:

• Constant number of items:

static constexpr auto size = TestSelectableItem::numberOfItems();

• List of keys is static:

static const auto& allKeys = TestSelectableItem::allKeysAsString();

• Constructor can be constexpr:

static constexpr TestSelectableItem foo("large");

It allows to use the type in a constexpr context. For example:

  switch ( d_resolutionMethod.getValue() )
  {
      case ResolutionMethod("ProjectedGaussSeidel"):
      case ResolutionMethod("NonsmoothNonlinearConjugateGradient"):
      {
          buildSystem_matrixAssembly(cParams);
          break;
      }
      case ResolutionMethod("UnbuiltGaussSeidel"):
      {
          buildSystem_matrixFree(numConstraints);
          break;
      }
      default:
          msg_error() << "Wrong \"resolutionMethod\" given";
  }

There is compile-time check that any of the ResolutionMethod("ProjectedGaussSeidel"), ResolutionMethod("NonsmoothNonlinearConjugateGradient"), ResolutionMethod("UnbuiltGaussSeidel") exist. If it does not exist, it does not compile.

It is preferable to force the constexpr context:

static constexpr ResolutionMethod NonsmoothNonlinearConjugateGradient("NonsmoothNonlinearConjugateGradient");
if (d_resolutionMethod.getValue() != NonsmoothNonlinearConjugateGradient)
{

The compiler may not choose to use constexpr in:

if (d_resolutionMethod.getValue() != ResolutionMethod("NonsmoothNonlinearConjugateGradient"))
{

Therefore, no constexpr check if NonsmoothNonlinearConjugateGradient is not in the list. There is a runtime check though.

The type allows to deprecate a key if desired (I am thinking about the Data method in the FEM force fields).

There is also a description of each key. And the whole list (key + description) can be added easily in the description of the Data, hence in the documentation.

The major problem is about the GUI. More particularly, when a Data is read to be displayed in the GUI. Here, each of the SelectableItem is a new strong type. So there is no DataTypeName, DataTypeInfo etc for the new type. I tried to factorize with a common base class (BaseSelectableItem) but without success. I tried to define DataTypeName, DataTypeInfo etc for SelectedItem but without success. The consequence is that I cannot specialize a widget for this type of Data.

---

By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).

---

Reviewers will merge this pull-request only if

• it builds with SUCCESS for all platforms on the CI.
• it does not generate new warnings.
• it does not generate new unit test failures.
• it does not generate new scene test failures.
• it does not break API compatibility.
• it is more than 1 week old (or has fast-merge label).

[alxbilger]

[ci-build][with-all-tests]

[bakpaul]

Nice ! I like it. The only down side is the need to use a macro to have something that doesn't look heavy to declare a item list... But I guess there is no other solution.

[alxbilger]

@damienmarchal I managed to return a common string for all types deriving from SelectableItem when calling Data::getValueTypeString. Now, the problem is with the dynamic_cast:

struct TestSelectableItem final : sofa::helper::SelectableItem<TestSelectableItem> {};
TestSelectableItem item;
sofa::Data<TestSelectableItem> data;
BaseData* baseData = &data;

Obviously dynamic_cast<BaseSelectableItem*>(&item) != nullptr, but dynamic_cast<Data<BaseSelectableItem>*>(&baseData)==nullptr.

The widget mechanism relies on dynamic_cast to go from a BaseData to a Data<T> (

sofa/Sofa/GUI/Qt/src/sofa/gui/qt/DataWidget.h

Line 69 in cda8e23

typename T::MyData* data = dynamic_cast<typename T::MyData*>(arg.data);

). Any idea?

[damienmarchal]

@alxbilger

I was trying to find a solution but looking at the code in DataWidget.cpp makes me notice:

DataWidget *DataWidget::CreateDataWidget(const DataWidget::CreatorArgument &dwarg)
{
    DataWidget *datawidget_ = nullptr;
    const std::string &widgetName=dwarg.data->getWidget();
    if (widgetName.empty())
        datawidget_ = DataWidgetFactory::CreateAnyObject(dwarg);
    else
        datawidget_ = DataWidgetFactory::CreateObject(widgetName, dwarg);

    return datawidget_;
}

And wonder if you try the getWidget() to short cut the dynamic_cast ?

[alxbilger]

@damienmarchal I believe that the dynamic_cast still happens in https://github.com/sofa-framework/sofa/blob/master/Sofa/GUI/Qt/src/sofa/gui/qt/DataWidget.h#L180