-
Notifications
You must be signed in to change notification settings - Fork 9
Feature/UI section refactor #247
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feature/properties
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the naming here is such that core/prty is the name of a library with a (loose) namespace prty that could have many classes in it. Currently prtyProperty is the only one. This is a foundational class that can store a named value for the UI. A future reorg moves core out of renderlib along with many other non-rendering classes.
| // non-copy, just use paren operator | ||
| T& operator()() const { return value; } | ||
|
|
||
| // set up the rule of 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the following several functions are just boilerplate made explicit for how to copy/move these objects in various c++ language scenarios.
| m_viewerWindow->gesture.input.setDoubleClickTime((double)QApplication::doubleClickInterval() / 1000.0); | ||
|
|
||
| // camera is created deep down inside m_viewerWindow. | ||
| m_cameraDataObject = new CameraDataObject(&m_viewerWindow->m_CCamera); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initialization order for cameras is now slightly different so we can introduce the CameraDataObject from which the gui will be constructed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are definitions for basic data needed to set up ui controls. This will expand over time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all the methods in controlFactory are about taking the generic uiInfos and prtyProperties and creating actual Qt user interface that updates the values of the prtyProperties
|
|
||
| #include <QLabel> | ||
|
|
||
| QCameraWidget::QCameraWidget(QWidget* pParent, RenderSettings* rs, CameraDataObject* cdo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now all this class has to do is produce the ui info for each property of the camera, and make calls into the controlFactory to create the ui... as seen below. A sort of ideal world would be where this is just a generic loop over all the properties and the uiinfos.
| prtyProperty<float> FieldOfView{ "FieldOfView", 30.0f }; | ||
| prtyProperty<float> FocalDistance{ "FocalDistance", 0.0f }; | ||
|
|
||
| CCamera* m_camera; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that this class holds on to the actual CCamera which is used for real rendering, so that it can update things properly. Each DataObject class may have different internals in addition to its collection of properties.
| // direct assignment from a value | ||
| prtyProperty& operator=(const T& val) | ||
| { | ||
| value = val; | ||
| return *this; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will anything bad happen if this doesn't call notifyAll? Is this assignment operator only used to sync the property value with internal values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great point here, I'm not actually sure but I think for now I will update it to notify. Or else disable this operator entirely.
| QNumericSlider* | ||
| create(const IntSliderSpinnerUiInfo* info, std::shared_ptr<prtyProperty<int>> prop); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing equivalent declaration for addRow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added...
| slider->setValue(prop->get(), true); | ||
| QObject::connect(slider, &QNumericSlider::valueChanged, [slider, prop](double value) { prop->set(value, true); }); | ||
| // TODO how would this capture the "previous" value, for undo? | ||
| QObject::connect(slider, &QNumericSlider::valueChangeCommit, [slider, prop]() { prop->notifyAll(true); }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this still need to call notifyAll if prtyProperty already does so on set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this piece of code is concerned with the possibility of undo, and that we might not want to undo every single slider move, but still want the slider moves to update the rendering in realtime. It's not worked out well enough yet, and will almost certainly be discarded/reworked.
| QNumericSlider* slider = addRow(CameraUiDescription::m_exposure, &m_cameraDataObject->Exposure); | ||
| m_MainLayout.addRow("Exposure", slider); | ||
| QComboBox* comboBox = addRow(CameraUiDescription::m_exposureIterations, &m_cameraDataObject->ExposureIterations); | ||
| m_MainLayout.addRow("Exposure Time", comboBox); | ||
| QCheckBox* checkBox = addRow(CameraUiDescription::m_noiseReduction, &m_cameraDataObject->NoiseReduction); | ||
| m_MainLayout.addRow("Noise Reduction", checkBox); | ||
| QNumericSlider* slider2 = addRow(CameraUiDescription::m_apertureSize, &m_cameraDataObject->ApertureSize); | ||
| m_MainLayout.addRow("Aperture Size", slider2); | ||
| QNumericSlider* slider3 = addRow(CameraUiDescription::m_fieldOfView, &m_cameraDataObject->FieldOfView); | ||
| m_MainLayout.addRow("Field of view", slider3); | ||
| QNumericSlider* slider4 = addRow(CameraUiDescription::m_focalDistance, &m_cameraDataObject->FocalDistance); | ||
| m_MainLayout.addRow("Focal distance", slider4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very clean!
9a21f5a to
478121d
Compare
…ling Feature/tfeditor styling
# Conflicts: # agave_app/tfeditor/gradients.cpp
Co-authored-by: Peyton Lee <[email protected]>
478121d to
d8a50d1
Compare
Start to make a more generic ui for setting properties on objects. In this way we will establish patterns for having dockable windows with the lists of editable object properties. Currently only the Camera is implemented, but AreaLight, SphereLight, ClipPlane, Volume, and RenderSettings would follow.
Implementation details are added in inline review comments, see below.
before:

after:
