-
Notifications
You must be signed in to change notification settings - Fork 10
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
Feature/flip volume #184
Feature/flip volume #184
Conversation
299f872
to
fe215d0
Compare
renderlib/json/json.hpp
Outdated
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.
can disregard diff in this file as it is a third party include which I updated
lights, | ||
capture, | ||
showScaleBar) | ||
NLOHMANN_DEFINE_TYPE_INTRUSIVE_WITH_DEFAULT(ViewerState, |
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.
comment why this macro is here and why it usees WITH_DEFAULT
renderlib/ImageXYZC.cpp
Outdated
} | ||
|
||
void | ||
ImageXYZC::setVolumeAxesFlipped(float x, float y, float z) |
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 really should be storing ints or booleans
@@ -369,6 +384,34 @@ QAppearanceSettingsWidget::createSkyLightingControls() | |||
return section; | |||
} | |||
|
|||
void | |||
QAppearanceSettingsWidget::OnFlipX(bool value) |
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.
just making a note of my suggestion to change these 3 functions to one that takes in an axis
value to reduce the copied code
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.
Some small suggestions came up in our review meeting, but overall seems good
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.
Looks great!
Estimated time to review: ~15 min ? A lot of code changes but all small.
Allows users to flip (mirror / invert) each of the three x,y,z axes for the volume.
The internal storage uses -1 for a flipped axis, and 1 for an unflipped one.
The volume sampling shader has to switch its sampling wrap mode to "REPEAT" to make this work.
When adding a new feature like this, we need to make sure the feature is implemented in json save files (serialization), python script output, as well as client/server communication (aka Commands).
In order to make it easier to add new serialized variables, without having to bump versions, I had to upgrade the json library I am using. Disregard changes to json.hpp as it is a third party file.