Draft for Issue1123 - Expandable Transfer Function #3698
Conversation
sgpearse
changed the title
There was a problem hiding this comment.
Crash:
- Create/enable a volume renderer
- Expand TF editor
- Create/enable a 2d renderer
- Delete the first renderer
- Edit the first renderers TF in the expanded view
- Vapor will crash
The resolution of the histogram needs to be limited/performance improved as it becomes noticeably slow at fullscreen retina.
| { | ||
| _maps = new TFMapGroupWidget; | ||
| _histogram = new TFHistogramMap(tag); | ||
| _opacityMap = new TFOpacityMap(tag); | ||
| _colorMap = new TFColorMap(tag); | ||
| _isoMap = new TFIsoValueMap(tag); | ||
| _range = new TFMappingRangeSelector(tag); | ||
| _elements = elements; | ||
| _label = label; |
There was a problem hiding this comment.
The label parameter should be held by the VSection rather than in the subclass. Also, it is optional which is not being handled.
| } | ||
|
|
||
| void PTFEditor::closeEvent(QCloseEvent* event) { | ||
| emit closed(); |
There was a problem hiding this comment.
Why not simply call closeExpandedPTFEditor
There was a problem hiding this comment.
We need to specifically capture the expanded PTFEditor's closeEvent so the containing PTFEditor can then set the expanded to nullptr.
If we don't, then the checks for _expandedPTFEditor==nullptr will fail, particularly in PTFEditor::showExpandedPTFEditor().
| @@ -55,6 +54,8 @@ class PWidget : public UWidget { | |||
| void setToolTip(const QString &) = delete; | |||
|
|
|||
| protected: | |||
| const std::string _tag; | |||
There was a problem hiding this comment.
This should remain private.
| @@ -199,6 +201,11 @@ TFMapWidget::TFMapWidget(TFMap *map) | |||
| AddMap(map); | |||
| setContextMenuPolicy(Qt::CustomContextMenu); | |||
| connect(this, SIGNAL(customContextMenuRequested(const QPoint &)), this, SLOT(_showContextMenu(const QPoint &))); | |||
|
|
|||
| // Is there a better way or place to set the sizePolicy of this object? | |||
There was a problem hiding this comment.
You could do it in the subclass constructor
There was a problem hiding this comment.
The subclass isn't a QWidget though. It needs to be done in the TFMapWidget, not the TFMap :\
| option.features = QStyleOptionToolButton::None; | ||
| p.drawComplexControl(QStyle::CC_ToolButton, option); | ||
| } | ||
|
|
There was a problem hiding this comment.
ExpandSectionButton duplicates the code of SettingsMenuButton apart from changing the icon path.
| @@ -1312,7 +1312,8 @@ void ParamsMgr::PMgrStateSave::EndGroup() | |||
|
|
|||
| if (!GetEnabled()) return; | |||
|
|
|||
| assert(_groups.size()); | |||
| // If we are going to return in the next statement, why also assert for the same condition? | |||
| //assert(_groups.size()); | |||
There was a problem hiding this comment.
This assert needs to remain here. I think the intention was to have it be a VAssert so that it would only crash in debug builds and silently fail on release builds.
|
|
||
| public: | ||
| VSection(const std::string &title); | ||
| QVBoxLayout *layout() const; | ||
| void setMenu(QMenu *menu); | ||
| void setExpandedSection(); |
There was a problem hiding this comment.
Method is called set... but nothing is being set.
|
Thanks @StasJ. I'll address/reply to your comments shortly. |
Draft to fix #1123 by creating new PTFEditors with an expand-window button, shown in the top left corner of the editor.
I have two questions that I'm uncertain on:
ParamsMgr.cpp:1315 asserts that _groups.size() > 0, but then returns in the following statement under the same condition. Why is it throwing an assert here? Does the return statement suffice?
I need to set the sizePolicy for TFMapWidgets that contain TFColorMaps and TFIsoValueMaps. I'm doing this in the TFMapWidget constructor with dynamic casts to catch the TFColorMap/TFIsoValueMap instances. Is there a better way to do this?
I'm currently reaching out to leigh for usability feedback.
Note that the Volume Renderer's colormapped variable transfer function is currently not expandable. I plan on coming back to it hopefully once the currently reported regressions have been fixed.