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
3013: support missing Q2 surface attributes #3872
Conversation
- add test that hasSurfaceAttributes() is checking whether they were parsed, not checking for 0 (failing)
Can I review this commit by commit? |
No, I left an note above :)
|
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 good, just a few minor questions. Sorry for taking so long!
@@ -283,18 +355,26 @@ namespace TrenchBroom { | |||
m_surfaceValueEditor->setRange(min, max); | |||
m_surfaceValueEditor->setIncrements(1.0, 10.0, 100.0); | |||
m_surfaceValueEditor->setDigits(0, 6); | |||
m_surfaceValueUnsetButton = createBitmapButton("Unset.svg", tr("Unset surface 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.
I think that it's more fitting to use the "ResetTexture" icon here, too. Effectively, you are resetting these values to whatever is the default from the texture.
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.
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.
I think of these icons in more general terms like +
is for adding, -
for deleting, X
is for resetting, etc. The reason why I suggested X
for this is that it also resets to defaults, in a sense. That one of them applies to UVs and the other applies to surface flags and such should be apparent from the placement of the buttons. So in that sense, we're not overloading this icon with a new meaning.
We originally used the curved arrow icon for this in the UV editor, but then changed it because we needed an icon for "Reset to world" and couldn't make that work with the curved arrow. In my view, if we now introduce the curved arrow again for something that also means "reset", then that's inconsistent, and I'd rather like to avoid that.
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.
OK, I see what you're saying. There's a subtle difference which is what I was trying to hint at with the different icons:
- the face/contents/value one is "Unset (and inherit the value from the parent [the texture], including future changes if the parent changes)"
- the texture axis one is "Set texture alignment to sensible defaults, based on the current face normal". i.e. there's no concept of inheritance here.
but for the sake of icons maybe these are close enough that we can call them both "Reset"
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.
Thanks! I agree that there is a slight semantic difference, but I believe that from the users' point of view, it doesn't matter as much.
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.
Thanks for addressing my feedback!
Fixes #3013
Overview (best to review the overall diff, not by commits):
std::variant<std::monostate, Q2Data>
member for the above