Restructure the GUI - Fixes Issue3603 #3719
Conversation
There was a problem hiding this comment.
If vapor is started with data or a session, or when a session is opened, it should default to the render page.
This could be a separate PR later on, but since we now have an explicit separation between the render stage and annotation stage, should we move per-renderer annotations under the annotate section?
After pressing "Capture Time series", it says "Saved: <file> At: time" but the file name is not correct.
Should camera controls be under Export since they are related to the viewport outside of exporting? Same with moving domain settings.
Minor feature request: remember last selected output folder.
Bug: Under animation enable "loop playback". Press capture time series. Vapor will indefinitely save images.
Bug: Import files does not respect data directory
The text field next to import files is somewhat confusing:
- It is not edible
- Once you "select" a dataset it gets imported and then the text field sits populated but doesn't do anything.
Capture Time series should not be limited by max animation frames per second.
Same for animation play step size.
Bug: Changing the time series range changes the current tilmestep display in the rest of the application but it does not actually change the visualizer timestep. (This is correct, it should not affect the visualizer tilmestep imo).
I believe "Time series" is two words but it is spelled as one word throughout the ui.
Each dataset now has a "Data Projection" menu but this is a global property, not per-dataset.
Bug: open a dataset. Create a new session. Click on the render list. Vapor will crash.
| void DataImported(); | ||
|
|
||
| signals: | ||
| void dataImported(); |
| @@ -5,14 +5,16 @@ | |||
| #include <QWidget> | |||
| #include "Updatable.h" | |||
|
|
|||
| class ViewpointTab : public QWidget, public Updatable { | |||
| for (const auto &e : _updatableElements) | ||
| leftPanel->UpdateImportMenu(); | ||
|
|
||
| for (const auto &e : _updatableElements) |
There was a problem hiding this comment.
Could leftPanel just be updatable?
|
|
||
| #include <QScrollArea> | ||
|
|
||
| LeftPanel::LeftPanel(ControlExec *ce) | ||
| LeftPanel::LeftPanel(ControlExec *ce, MainForm *mf) : _ce(ce) |
There was a problem hiding this comment.
The MainForm should not be passed around. It displays information from the CE. If you need to update the main form, that data should be changed in the CE, then the main form will update to show those changes.
|
|
||
| const std::string TiffStrings::CaptureFileType = "TIFF"; | ||
| const std::string TiffStrings::FileFilter = "TIFF (*.tif)"; | ||
| const std::string TiffStrings::FileSuffix = "tif"; |
There was a problem hiding this comment.
These one-off strings probably don't need to be stored as variables but you are also using hardcoded strings below and in other parts of this PR that contradict these anyway.
| } | ||
|
|
||
| void RenderersPanel::Update() | ||
| { | ||
| setEnabled(true); |
There was a problem hiding this comment.
Again, this was already being handled and will this leads to crash documented in main comment.
| QLabel *_ql; | ||
|
|
||
| public: | ||
| VLabel(const std::string &text = ""); | ||
| void SetText(const std::string &text); | ||
| void MakeSelectable(); | ||
| }; | ||
|
|
||
| class VHyperlink : public VLabel { |
| } | ||
|
|
||
| void VLineItem::SetLabelText(const std::string &text) { | ||
| _qLabel->setText(QString::fromStdString(text)); |
There was a problem hiding this comment.
Line Items are a widget and a label describing that widget. For your use case here, you need to use two labels in a horizontal layout rather than repurposing the line time.
| @@ -42,7 +42,7 @@ COMMON_API std::string ToLower(std::string str); | |||
| COMMON_API std::vector<std::string> Split(std::string str, const std::string &delimeter); | |||
| COMMON_API std::string Join(const std::vector<std::string> &parts, const std::string &delimeter); | |||
| COMMON_API std::string ReplaceAll(std::string source, const std::string &oldSegment, const std::string &newSegment); | |||
|
|
|||
| COMMON_API std::string GetUserTime(); | |||
There was a problem hiding this comment.
Name could be confused for user data time.
| @@ -46,6 +46,8 @@ const string GUIStateParams::_flowDimensionalityTag = "_flowDimensionalityTag"; | |||
| const string GUIStateParams::BookmarksTag = "BookmarksTag"; | |||
| const string GUIStateParams::MovingDomainTrackCameraTag = "MovingDomainTrackCameraTag"; | |||
| const string GUIStateParams::MovingDomainTrackRenderRegionsTag = "MovingDomainTrackRenderRegionsTag"; | |||
| const string GUIStateParams::SelectedImportDataTypeTag = "SelectedImportDataTypeTag"; | |||
| const string GUIStateParams::SelectedImportDataFilesTag = "SelectedImportDataFilesTag"; | |||
There was a problem hiding this comment.
Do these need to be persistent state?
There was a problem hiding this comment.
This is a noticeable improvement Scott.
A few issues and suggestions:
- Bug: In the export menu, changing the format from TIFF to Png int he dropdown still saves the files as TIFF
- Suggestion: Can we increase the size of top level tabs (import, render, animate) to set them apart from lower level tabs?
- Suggestion: Capture timeseries button appears greyed out. Making it consistent might help
- Suggestion: Can the annotate tab under 'render' be renamed to colorbar instead?
Fixes #3603 by changing the GUI to what is shown below.
Two considerations:
PWidget::ShowBasedOnParam(string, string) now can accept strings for comparing parameter values. I am finding that I need to also set the ShowBasedOnParam(string, int) to a non-zero value for things to work. PWidget::_enableBasedOnParamValue is getting a default applied when constructed construction, which it should not to my understanding (shouldn't it be garbage?).
MainForm has several utilities for importing data and capturing imagery. I've made the necessary functions public, but this means that PImportData and PCaptureWidget need to hold pointers to the MainForm.