Fix opacity updates (#237)

ThomasKroes · web-flow · commit 05df9573dd93 · 2026-02-26T17:38:09.000+01:00
* Cache current dataset and handle dataChanged

Add a _currentDataset member to ScalarAction and wire up its dataChanged signal when a new source is selected. Previously connections could accumulate; the code now disconnects any existing dataset signal before connecting the new one and forwards updates via a lambda that calls sourceDataChanged(getCurrentDataset()).

Also update several parameter names in comments for clarity (variantMap, sourceSelectionIndex, offset).

* Embed source dataset picker in ScalarAction

Centralize dataset picking for scalar-driven properties by adding a DatasetPickerAction to ScalarAction and removing the separate point-size/opacity pickers from DatasetsAction. DatasetsAction no longer owns point size/opacity picker members; it instead reuses the ScalarAction pickers from the point plot settings and adjusts reset/connect logic accordingly (old filter code was commented out). ScalarAction now manages current dataset selection, connects dataset changes to emit sourceDataChanged and GUI name updates, and includes serialization for the new picker. ScatterplotPlugin was updated to listen to ScalarAction signals (sourceSelectionChanged/sourceDataChanged) for HUD updates and to refresh the scatter plot widget when scalar source data changes. Headers and getters were updated to reflect the new picker location.

* Use LabelProxyAction for Size/Opacity pickers

Replace direct DatasetPickerAction additions for point size and opacity with LabelProxyAction wrappers (labels "Size" and "Opacity") and include actions/LabelProxyAction.h instead of QMenu. Remove the now-unused setupPointSizeDatasetPickerAction and setupPointOpacityDatasetPickerAction declarations and implementations, and clean up related filter/invalidation code. This simplifies the dataset picker UI wiring and removes dead/commented code in DatasetsAction.cpp/.h.
diff --git a/src/DatasetsAction.cpp b/src/DatasetsAction.cpp
@@ -5,7 +5,7 @@
 #include <ClusterData/ClusterData.h>
 #include <ColorData/ColorData.h>
 
-#include <QMenu>
+#include <actions/LabelProxyAction.h>
 
 using namespace mv;
 using namespace mv::gui;
@@ -14,48 +14,47 @@ DatasetsAction::DatasetsAction(QObject* parent, const QString& title) :
     GroupAction(parent, title),
     _scatterplotPlugin(dynamic_cast<ScatterplotPlugin*>(parent->parent())),
     _positionDatasetPickerAction(this, "Position"),
-    _colorDatasetPickerAction(this, "Color"),
-    _pointSizeDatasetPickerAction(this, "Point size"),
-    _pointOpacityDatasetPickerAction(this, "Point opacity")
+    _colorDatasetPickerAction(this, "Color")
 {
     setIconByName("database");
     setToolTip("Manage loaded datasets for position and color");
     setConfigurationFlag(WidgetAction::ConfigurationFlag::ForceCollapsedInGroup);
     setLabelSizingType(LabelSizingType::Auto);
 
-    addAction(&_positionDatasetPickerAction);
-    addAction(&_colorDatasetPickerAction);
-    addAction(&_pointSizeDatasetPickerAction);
-    addAction(&_pointOpacityDatasetPickerAction);
-
-    _positionDatasetPickerAction.setDefaultWidgetFlag(OptionAction::Clearable);
-    _colorDatasetPickerAction.setDefaultWidgetFlag(OptionAction::Clearable);
-    _pointSizeDatasetPickerAction.setDefaultWidgetFlag(OptionAction::Clearable);
-    _pointOpacityDatasetPickerAction.setDefaultWidgetFlag(OptionAction::Clearable);
-
     auto scatterplotPlugin = dynamic_cast<ScatterplotPlugin*>(parent->parent());
 
     Q_ASSERT(scatterplotPlugin);
 
     if (scatterplotPlugin == nullptr)
         return;
 
+    auto& settingsAction    = scatterplotPlugin->getSettingsAction();
+    auto& plotAction        = settingsAction.getPlotAction();
+    auto& pointPlotAction   = plotAction.getPointPlotAction();
+
+	addAction(&_positionDatasetPickerAction);
+    addAction(&_colorDatasetPickerAction);
+    addAction(new LabelProxyAction(this, "Size", const_cast<DatasetPickerAction*>(&pointPlotAction.getSizeAction().getSourceDatasetPickerAction())));
+    addAction(new LabelProxyAction(this, "Opacity", const_cast<DatasetPickerAction*>(&pointPlotAction.getOpacityAction().getSourceDatasetPickerAction())));
+
+    _positionDatasetPickerAction.setDefaultWidgetFlag(OptionAction::Clearable);
+    _colorDatasetPickerAction.setDefaultWidgetFlag(OptionAction::Clearable);
+    
     setupDatasetPickerActions(scatterplotPlugin);
 
     const auto invalidateFilters = [this, scatterplotPlugin]() -> void {
         _colorDatasetPickerAction.invalidateFilter();
-        _pointSizeDatasetPickerAction.invalidateFilter();
-        _pointOpacityDatasetPickerAction.invalidateFilter();
     };
 
     connect(&_positionDatasetPickerAction, &DatasetPickerAction::datasetPicked, [this, scatterplotPlugin, invalidateFilters](Dataset<DatasetImpl> pickedDataset) -> void {
         invalidateFilters();
     });
 
-    const auto resetAuxilliaryDatasets = [this]() -> void {
+    const auto resetAuxilliaryDatasets = [this, &pointPlotAction]() -> void {
         _colorDatasetPickerAction.setCurrentIndex(-1);
-		_pointSizeDatasetPickerAction.setCurrentIndex(-1);
-		_pointOpacityDatasetPickerAction.setCurrentIndex(-1);
+
+        pointPlotAction.getSizeAction().setCurrentSourceIndex(ScalarSourceModel::DefaultRow::Constant);
+		pointPlotAction.getOpacityAction().setCurrentSourceIndex(ScalarSourceModel::DefaultRow::Constant);
 	};
 
     connect(&scatterplotPlugin->getPositionDataset(), &Dataset<Points>::changed, this, resetAuxilliaryDatasets);
@@ -74,8 +73,6 @@ void DatasetsAction::connectToPublicAction(WidgetAction* publicAction, bool recu
     if (recursive) {
         actions().connectPrivateActionToPublicAction(&_positionDatasetPickerAction, &publicDatasetsAction->getPositionDatasetPickerAction(), recursive);
         actions().connectPrivateActionToPublicAction(&_colorDatasetPickerAction, &publicDatasetsAction->getColorDatasetPickerAction(), recursive);
-        actions().connectPrivateActionToPublicAction(&_pointSizeDatasetPickerAction, &publicDatasetsAction->getPointSizeDatasetPickerAction(), recursive);
-        actions().connectPrivateActionToPublicAction(&_pointOpacityDatasetPickerAction, &publicDatasetsAction->getPointOpacityDatasetPickerAction(), recursive);
     }
 
     GroupAction::connectToPublicAction(publicAction, recursive);
@@ -89,8 +86,6 @@ void DatasetsAction::disconnectFromPublicAction(bool recursive)
     if (recursive) {
         actions().disconnectPrivateActionFromPublicAction(&_positionDatasetPickerAction, recursive);
         actions().disconnectPrivateActionFromPublicAction(&_colorDatasetPickerAction, recursive);
-        actions().disconnectPrivateActionFromPublicAction(&_pointSizeDatasetPickerAction, recursive);
-        actions().disconnectPrivateActionFromPublicAction(&_pointOpacityDatasetPickerAction, recursive);
     }
 
     GroupAction::disconnectFromPublicAction(recursive);
@@ -118,8 +113,6 @@ void DatasetsAction::setupDatasetPickerActions(ScatterplotPlugin* scatterplotPlu
 {
     setupPositionDatasetPickerAction(scatterplotPlugin);
     setupColorDatasetPickerAction(scatterplotPlugin);
-    setupPointSizeDatasetPickerAction(scatterplotPlugin);
-    setupPointOpacityDatasetPickerAction(scatterplotPlugin);
 }
 
 void DatasetsAction::setupPositionDatasetPickerAction(ScatterplotPlugin* scatterplotPlugin)
@@ -174,90 +167,8 @@ void DatasetsAction::setupColorDatasetPickerAction(ScatterplotPlugin* scatterplo
     });
 }
 
-void DatasetsAction::setupPointSizeDatasetPickerAction(ScatterplotPlugin* scatterplotPlugin)
-{
-    auto& settingsAction        = scatterplotPlugin->getSettingsAction();
-    auto& pointPlotAction       = settingsAction.getPlotAction().getPointPlotAction();
-    auto& pointSizeAction       = pointPlotAction.getSizeAction();
-
-    _pointSizeDatasetPickerAction.setFilterFunction([this, scatterplotPlugin](mv::Dataset<DatasetImpl> dataset) -> bool {
-        if (!scatterplotPlugin->getPositionDataset().isValid())
-            return false;
-
-    	if (dataset->getDataType() != PointType)
-            return false;
-
-        return true;
-    });
-
-    connect(&_pointSizeDatasetPickerAction, &DatasetPickerAction::currentIndexChanged, this, [this, &pointPlotAction, &pointSizeAction, scatterplotPlugin](const int32_t& currentIndex) -> void {
-        const auto& pointSizeDataset = _pointSizeDatasetPickerAction.getCurrentDataset();
-
-        if (pointSizeDataset.isValid())
-            disconnect(&_pointSizeDataset, &Dataset<>::guiNameChanged, this, nullptr);
-
-        _pointSizeDataset = pointSizeDataset;
-
-        connect(&_pointSizeDataset, &Dataset<>::guiNameChanged, scatterplotPlugin, &ScatterplotPlugin::updateHeadsUpDisplay);
-
-        pointPlotAction.setCurrentPointSizeDataset(_pointSizeDataset);
-
-        if (!_pointSizeDataset.isValid())
-            pointSizeAction.setCurrentSourceIndex(ScalarSourceModel::DefaultRow::Constant);
-    });
-
-    connect(&pointSizeAction, &ScalarAction::sourceSelectionChanged, this, [this, &pointSizeAction](const uint32_t& sourceSelectionIndex) -> void {
-        _pointSizeDatasetPickerAction.setCurrentDataset(pointSizeAction.isSourceDataset() ? pointSizeAction.getCurrentDataset() : nullptr);
-
-        if (!pointSizeAction.isSourceDataset())
-            _pointSizeDatasetPickerAction.setCurrentIndex(-1);
-    });
-}
-
-void DatasetsAction::setupPointOpacityDatasetPickerAction(ScatterplotPlugin* scatterplotPlugin)
-{
-    auto& settingsAction        = scatterplotPlugin->getSettingsAction();
-    auto& pointPlotAction       = settingsAction.getPlotAction().getPointPlotAction();
-    auto& pointOpacityAction    = pointPlotAction.getOpacityAction();
-
-    _pointOpacityDatasetPickerAction.setFilterFunction([this, scatterplotPlugin](mv::Dataset<DatasetImpl> dataset) -> bool {
-        if (!scatterplotPlugin->getPositionDataset().isValid())
-            return false;
-
-    	if (dataset->getDataType() != PointType)
-            return false;
-        
-        return true;
-    });
-
-    connect(&_pointOpacityDatasetPickerAction, &DatasetPickerAction::currentIndexChanged, this, [this, &pointPlotAction, &pointOpacityAction, scatterplotPlugin](const int32_t& currentIndex) -> void {
-        const auto& pointOpacityDataset = _pointOpacityDatasetPickerAction.getCurrentDataset();
-
-        if (pointOpacityDataset.isValid())
-            disconnect(&_pointOpacityDataset, &Dataset<>::guiNameChanged, this, nullptr);
-
-        _pointOpacityDataset = pointOpacityDataset;
-
-        connect(&_pointOpacityDataset, &Dataset<>::guiNameChanged, scatterplotPlugin, &ScatterplotPlugin::updateHeadsUpDisplay);
-
-        pointPlotAction.setCurrentPointOpacityDataset(_pointOpacityDataset);
-
-        if (!_pointOpacityDataset.isValid())
-            pointOpacityAction.setCurrentSourceIndex(ScalarSourceModel::DefaultRow::Constant);
-    });
-
-    connect(&pointOpacityAction, &ScalarAction::sourceSelectionChanged, this, [this, &pointOpacityAction](const uint32_t& sourceSelectionIndex) -> void {
-        _pointOpacityDatasetPickerAction.setCurrentDataset(pointOpacityAction.isSourceDataset() ? pointOpacityAction.getCurrentDataset() : nullptr);
-
-    	if (!pointOpacityAction.isSourceDataset())
-            _pointOpacityDatasetPickerAction.setCurrentIndex(-1);
-    });
-}
-
 void DatasetsAction::invalidateDatasetPickerActionFilters()
 {
     _positionDatasetPickerAction.invalidateFilter();
     _colorDatasetPickerAction.invalidateFilter();
-    _pointSizeDatasetPickerAction.invalidateFilter();
-    _pointOpacityDatasetPickerAction.invalidateFilter();
 }
diff --git a/src/DatasetsAction.h b/src/DatasetsAction.h
@@ -21,8 +21,6 @@ class DatasetsAction : public GroupAction
     Q_INVOKABLE DatasetsAction(QObject* parent, const QString& title);
 
     mv::Dataset<mv::DatasetImpl> getColorDataset() { return _colorDataset; }
-    mv::Dataset<mv::DatasetImpl> getPointSizeDataset() { return _pointSizeDataset; }
-    mv::Dataset<mv::DatasetImpl> getPointOpacityDataset() { return _pointOpacityDataset; }
 
 protected: // Linking
 
@@ -73,37 +71,19 @@ class DatasetsAction : public GroupAction
      */
     void setupColorDatasetPickerAction(ScatterplotPlugin* scatterplotPlugin);
 
-    /**
-     * Set up the point size dataset picker action with the point size datasets from the scatter plot plugin
-     * @param scatterplotPlugin Pointer to scatter plot plugin whose point size datasets are used to populate the dataset picker action
-     */
-    void setupPointSizeDatasetPickerAction(ScatterplotPlugin* scatterplotPlugin);
-
-    /**
-     * Set up the point opacity dataset picker action with the point opacity datasets from the scatter plot plugin
-     * @param scatterplotPlugin Pointer to scatter plot plugin whose point opacity datasets are used to populate the dataset picker action
-     */
-    void setupPointOpacityDatasetPickerAction(ScatterplotPlugin* scatterplotPlugin);
-
     /** Update the filters of the dataset picker actions based on the current datasets in the scatter plot plugin */
     void invalidateDatasetPickerActionFilters();
 
 public: // Action getters
 
     DatasetPickerAction& getPositionDatasetPickerAction() { return _positionDatasetPickerAction; }
     DatasetPickerAction& getColorDatasetPickerAction() { return _colorDatasetPickerAction; }
-    DatasetPickerAction& getPointSizeDatasetPickerAction() { return _pointSizeDatasetPickerAction; }
-    DatasetPickerAction& getPointOpacityDatasetPickerAction() { return _pointOpacityDatasetPickerAction; }
 
 private:
     ScatterplotPlugin*              _scatterplotPlugin;               /** Pointer to scatter plot plugin */
     DatasetPickerAction             _positionDatasetPickerAction;     /** Dataset picker action for position dataset */
     DatasetPickerAction             _colorDatasetPickerAction;        /** Dataset picker action for color dataset */
-    DatasetPickerAction             _pointSizeDatasetPickerAction;    /** Dataset picker action for point size */
-    DatasetPickerAction             _pointOpacityDatasetPickerAction; /** Dataset picker action for point opacity */
     mv::Dataset<mv::DatasetImpl>    _colorDataset;                    /** Smart pointer to dataset used for coloring (if any) */
-    mv::Dataset<mv::DatasetImpl>    _pointSizeDataset;                /** Smart pointer to dataset for driving point size (if any) */
-    mv::Dataset<mv::DatasetImpl>    _pointOpacityDataset;             /** Smart pointer to dataset for driving point opacity (if any) */
 
     friend class mv::AbstractActionsManager;
     friend class ScatterplotPlugin;
diff --git a/src/ScalarAction.cpp b/src/ScalarAction.cpp
@@ -9,18 +9,23 @@ using namespace mv::gui;
 ScalarAction::ScalarAction(QObject* parent, const QString& title, const float& minimum /*= 0.0f*/, const float& maximum /*= 100.0f*/, const float& value /*= 0.0f*/) :
     GroupAction(parent, title),
     _magnitudeAction(this, title, minimum, maximum, value),
-    _sourceAction(this, QString("%1 source").arg(title))
+    _sourceAction(this, QString("%1 source").arg(title)),
+    _sourceDatasetPickerAction(this, "Source dataset")
 {
     setDefaultWidgetFlags(GroupAction::Horizontal);
     setShowLabels(false);
 
     addAction(&_magnitudeAction);
     addAction(&_sourceAction);
 
+	_sourceDatasetPickerAction.setDefaultWidgetFlag(OptionAction::Clearable);
+
     connect(&_sourceAction.getPickerAction(), &OptionAction::currentIndexChanged, this, [this](const std::uint32_t& currentIndex) {
         bool emitSourceSelectionChanged = true;
 
         if (currentIndex >= ScalarSourceModel::DefaultRow::DatasetStart) {
+            _sourceDatasetPickerAction.setCurrentDataset(_sourceAction.getModel().getDataset(currentIndex));
+
             if (auto scatterplotPlugin = dynamic_cast<ScatterplotPlugin*>(findPluginAncestor())) {
                 auto positionDataset            = scatterplotPlugin->getPositionDataset();
                 auto scalarSourcePointsDataset  = Dataset<Points>(getCurrentDataset());
@@ -33,12 +38,39 @@ ScalarAction::ScalarAction(QObject* parent, const QString& title, const float& m
                     scatterplotPlugin->addNotification(QString("The number of points in the scalar source dataset does not match the number of points in the position dataset. (numPositions=%1, numScalars:%2)").arg(QString::number(numPositions), QString::number(numScalars)));
                 }
             }
+        } else {
+            _sourceDatasetPickerAction.setCurrentIndex(-1);
         }
 
         if (emitSourceSelectionChanged)
             emit sourceSelectionChanged(currentIndex);
     });
 
+    connect(&_sourceDatasetPickerAction, &DatasetPickerAction::datasetPicked, this, [this](const Dataset<>& dataset) {
+        if (_currentDataset.isValid()) {
+            disconnect(&_currentDataset, &Dataset<>::dataChanged, this, nullptr);
+            disconnect(&_currentDataset, &Dataset<>::guiNameChanged, this, nullptr);
+        }
+            
+        _currentDataset = dataset;
+
+        const auto datasetRowIndex = _sourceAction.getModel().getRowIndex(_currentDataset);
+
+        _sourceAction.getPickerAction().setCurrentIndex(_currentDataset.isValid() ? datasetRowIndex : ScalarSourceModel::DefaultRow::Constant);
+
+        if (!_currentDataset.isValid())
+            return;
+
+        connect(&_currentDataset, &Dataset<>::dataChanged, this, [this]() -> void {
+            emit sourceDataChanged(getCurrentDataset());
+        });
+
+        connect(&_currentDataset, &Dataset<>::guiNameChanged, this, [this]() -> void {
+            if (auto scatterplotPlugin = dynamic_cast<ScatterplotPlugin*>(findPluginAncestor()))
+                scatterplotPlugin->updateHeadsUpDisplay();
+        });
+    });
+
     connect(&_magnitudeAction, &DecimalAction::valueChanged, this, [this](const float& value) {
         emit magnitudeChanged(value);
     });
@@ -88,22 +120,12 @@ void ScalarAction::removeAllDatasets()
 
 Dataset<DatasetImpl> ScalarAction::getCurrentDataset()
 {
-    auto& scalarSourceModel = _sourceAction.getModel();
-
-    const auto currentSourceIndex = _sourceAction.getPickerAction().getCurrentIndex();
-
-    if (currentSourceIndex < ScalarSourceModel::DefaultRow::DatasetStart)
-        return {};
-
-    return scalarSourceModel.getDataset(currentSourceIndex);
+    return _sourceDatasetPickerAction.getCurrentDataset();
 }
 
 void ScalarAction::setCurrentDataset(const Dataset<DatasetImpl>& dataset)
 {
-    const auto datasetRowIndex = _sourceAction.getModel().getRowIndex(dataset);
-
-    if (datasetRowIndex >= 0)
-        _sourceAction.getPickerAction().setCurrentIndex(datasetRowIndex);
+    _sourceDatasetPickerAction.setCurrentDataset(dataset);
 }
 
 void ScalarAction::setCurrentSourceIndex(std::int32_t sourceIndex)
@@ -162,6 +184,7 @@ void ScalarAction::fromVariantMap(const QVariantMap& variantMap)
 
     _magnitudeAction.fromParentVariantMap(variantMap);
     _sourceAction.fromParentVariantMap(variantMap);
+    _sourceDatasetPickerAction.fromParentVariantMap(variantMap);
 }
 
 QVariantMap ScalarAction::toVariantMap() const
@@ -170,6 +193,7 @@ QVariantMap ScalarAction::toVariantMap() const
 
     _magnitudeAction.insertIntoVariantMap(variantMap);
     _sourceAction.insertIntoVariantMap(variantMap);
+    _sourceDatasetPickerAction.insertIntoVariantMap(variantMap);
 
     return variantMap;
-}
+}
diff --git a/src/ScalarAction.h b/src/ScalarAction.h
@@ -1,6 +1,7 @@
 #pragma once
 
 #include <actions/GroupAction.h>
+#include <actions/DatasetPickerAction.h>
 
 #include "ScalarSourceAction.h"
 
@@ -81,7 +82,7 @@ class ScalarAction : public GroupAction
 
     /**
      * Load widget action from variant map
-     * @param Variant map representation of the widget action
+     * @param variantMap Variant map representation of the widget action
      */
     void fromVariantMap(const QVariantMap& variantMap) override;
 
@@ -96,11 +97,13 @@ class ScalarAction : public GroupAction
     DecimalAction& getMagnitudeAction() { return _magnitudeAction; }
     ScalarSourceAction& getSourceAction() { return _sourceAction; }
 
+	const DatasetPickerAction& getSourceDatasetPickerAction() { return _sourceDatasetPickerAction; }
+
 signals:
 
     /**
      * Signals that the source selection changed
-     * @param sourceIndex Index of the selected source (0 is constant, 1 is selection, above is a dataset)
+     * @param sourceSelectionIndex Index of the selected source (0 is constant, 1 is selection, above is a dataset)
      */
     void sourceSelectionChanged(const std::uint32_t& sourceSelectionIndex);
 
@@ -125,13 +128,15 @@ class ScalarAction : public GroupAction
 
     /**
      * Signals that the scalar offset changed
-     * @param magnitude Scalar magnitude
+     * @param offset Scalar offset
      */
     void offsetChanged(const float& offset);
 
 private:
-    DecimalAction           _magnitudeAction;   /** Scalar magnitude action */
-    ScalarSourceAction      _sourceAction;      /** Scalar source action */
+    DatasetPickerAction     _sourceDatasetPickerAction;     /** Dataset picker action for source dataset selection */
+    DecimalAction           _magnitudeAction;               /** Scalar magnitude action */
+    ScalarSourceAction      _sourceAction;                  /** Scalar source action */
+    Dataset<>               _currentDataset;                /** Cached current dataset (if any) */
 
     friend class mv::AbstractActionsManager;
 };
diff --git a/src/ScatterplotPlugin.cpp b/src/ScatterplotPlugin.cpp
