Skip to content
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

Add ability to pass HSV data to all Color widgets #2384

Closed
wants to merge 1 commit into from

Conversation

haldean
Copy link
Contributor

@haldean haldean commented Feb 26, 2019

This allows for lossless data round-trips through the color picker and color edit systems when an application stores colors as HSV. It lets you do things like track hue in applications even when saturation and/or value are zero:

ezgif-1-c9e21a21cb3a

I chose to pass the _Input flags through the entire stack, letting each widget decide what to show based on the input data. This means that some paths are a little more expensive because HSV-to-RGB conversions are done more than once, but it also makes it much easier to make sure the whole Color family of functions is lossless.

All drag-and-dropped color data is still RGB; HSV colors are converted to RGB when you drag them, and RGB data is converted to HSV when you drop a color onto a widget with _InputHSV specified.

I didn't add anything to the demo, because it wasn't super clear to me that this was worth surfacing in the demo; it's more about enabling different backend representations than a frontend change. The only thing I could think of is showing both the RGB and HSV interpretations of the same ImVec4 with two different ColorEdits; is that worth including, or just confusing?

Known backwards-incompatible changes

  • ImGuiColorEditFlags__InputMask was renamed to ImGuiColorEditFlags__ShowMask, and ImGuiColorEditFlags__InputMask is now used for a different mask. Since this is internal this breakage should be fine.
  • ColorEdit4 used to let you specify more than one of ImGuiColorEditFlags_RGB, ImGuiColorEditFlags_HSV, and ImGuiColorEditFlags_HEX, but the behavior of the function was a little odd. It now asserts that only one of those flags is provided.

Fixes #2383

@ocornut
Copy link
Owner

ocornut commented Feb 26, 2019

Thank you very much Haldean .

Will have to review more in details later but it seems good!

I chose to pass the _Input flags through the entire stack, letting each widget decide what to show based on the input data. This means that some paths are a little more expensive because HSV-to-RGB conversions are done more than once,

The important path to optimize for is the path where the widget is not active. Did this path got any worse/better when inputting RGB? Other paths should be fine as there's only one active widget.

All drag-and-dropped color data is still RGB; HSV colors are converted to RGB when you drag them, and RGB data is converted to HSV when you drop a color onto a widget with _InputHSV specified.

This sound reasonable.

I didn't add anything to the demo, because it wasn't super clear to me that this was worth surfacing in the demo; it's more about enabling different backend representations than a frontend change. The only thing I could think of is showing both the RGB and HSV interpretations of the same ImVec4 with two different ColorEdits; is that worth including, or just confusing?

I think it would be worth adding something to the demo, maybe working on another variable than the color one used everywhere, e.g. add color_stored_as_hsv ? It's ok if this variable only gets used once or twice.

Looking at the demo i contact, I think we should rename ImGuiColorEditFlags_RGB ImGuiColorEditFlags_HSV ImGuiColorEditFlags_HEX right now, adding a Show prefix to them and #ifndef IMGUI_DISABLE_OBSOLETE_FUNCTIONS block with the old name, and making sure none of the demo/internal code use the old name.

If you do this, would you mind splitting the commit in two and force-pushing?

  • One with just the _RGB->_ShowRGB, __InputsMask->__ShowMask renaming (no other functional changes)
  • Another with the rest.

This is so I can merge the first one right away. This is assuming it is practical for you to select individual hunks (I use SourceTree and I can easily stage certain lines). If your git tooling makes this non-trivial don't worry about it I should be able to do the split separately.

Thanks again, and quite happy receiving non-trivial well formed PR :)

@haldean haldean force-pushed the color-edit-input-hsv branch 3 times, most recently from f6f7699 to 1e24dc4 Compare February 26, 2019 18:37
@haldean
Copy link
Contributor Author

haldean commented Feb 26, 2019

The important path to optimize for is the path where the widget is not active. Did this path got any worse/better when inputting RGB? Other paths should be fine as there's only one active widget.

The new data flow with _InputRGB should be identical to the data flow previously: each widget that wants to display the color as HSV does its own RGB-to-HSV conversion. What I was referring to was _ShowRGB used to always be cheap, but is now slightly more expensive when used with _InputHSV because I chose to pass HSV all the way down the stack. I mentioned it only because there's a second option, which is to convert from HSV to RGB the moment the color enters the stack and then _InputHSV has exactly the same performance as _InputRGB plus a single ColorConvertHSVtoRGB.

I'm realizing now that that would completely negate the whole purpose of my PR, so there isn't really a choice here, but I don't think there's cause for concern. The codepath that behaves the same as the old codepath should be equally fast (no additional color conversions), and while _InputHSV with _ShowRGB will be slightly slower than _InputRGB with _ShowRGB, it will be just as slow as _InputRGB with _ShowHSV. Basically, it continues to be true that you have to pay the cost of color conversion when your input flag and your show flag aren't the same.

I think it would be worth adding something to the demo, maybe working on another variable than the color one used everywhere, e.g. add color_stored_as_hsv ? It's ok if this variable only gets used once or twice.

Done. The color picker section of the demo is getting a little long, but it's there now:

2019-02-26-103811_923x1942_scrot

Looking at the demo i contact, I think we should rename ImGuiColorEditFlags_RGB ImGuiColorEditFlags_HSV ImGuiColorEditFlags_HEX right now, adding a Show prefix to them and #ifndef IMGUI_DISABLE_OBSOLETE_FUNCTIONS block with the old name, and making sure none of the demo/internal code use the old name.

Done.

If you do this, would you mind splitting the commit in two and force-pushing?

Done. I actually wrote it this way first and then squashed to push, so I just resurrected it from the reflog, no problem. I checked out the intermediate commit and ensured that it builds against code that uses both the old and new names.

Thanks! Let me know if there's anything else you'd like to see.

@ocornut ocornut changed the title add ability to pass HSV data to all Color widgets Add ability to pass HSV data to all Color widgets Feb 27, 2019
ocornut added a commit that referenced this pull request Feb 27, 2019
…gs to respectively ImGuiColorEditFlags_DisplayRGB/_DisplayHSV/_DisplayHex. This is anticipation of adding new flags to ColorEdit/ColorPicker functions which would make those ambiguous. (#2384) [@haldean]
@ocornut
Copy link
Owner

ocornut commented Feb 27, 2019

I have merged the first commit because that's simple (until I review the rest).

After looking around I decided to rename _ShowXXX to _DisplayXXX so your second commit will currently break but should be trivial to fix up (I can also trivially do it when I will review it). Sorry for the trouble!

@ocornut
Copy link
Owner

ocornut commented Feb 27, 2019

Here's your second commit with conflicts fixed
patch.zip

This allows for lossless data round-trips through the color picker and
color edit systems, and lets you do things like track hue in
applications even when saturation and/or value are zero.
@haldean haldean force-pushed the color-edit-input-hsv branch from 1e24dc4 to c907e1d Compare February 27, 2019 18:56
@haldean
Copy link
Contributor Author

haldean commented Feb 27, 2019

Updated the branch to match the patch you provided. Thanks!

@@ -4630,12 +4694,22 @@ void ImGui::ColorTooltip(const char* text, const float* col, ImGuiColorEditFlags
}

ImVec2 sz(g.FontSize * 3 + g.Style.FramePadding.y * 2, g.FontSize * 3 + g.Style.FramePadding.y * 2);
ColorButton("##preview", ImVec4(col[0], col[1], col[2], col[3]), (flags & (ImGuiColorEditFlags_NoAlpha | ImGuiColorEditFlags_AlphaPreview | ImGuiColorEditFlags_AlphaPreviewHalf)) | ImGuiColorEditFlags_NoTooltip, sz);
ColorButton("##preview", ImVec4(col[0], col[1], col[2], flags & ImGuiColorEditFlags_NoAlpha ? 1.0f : col[3]), (flags & (ImGuiColorEditFlags__InputMask | ImGuiColorEditFlags_NoAlpha | ImGuiColorEditFlags_AlphaPreview | ImGuiColorEditFlags_AlphaPreviewHalf)) | ImGuiColorEditFlags_NoTooltip, sz);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only change I don't understand, why did you change the col[3] with flags & ImGuiColorEditFlags_NoAlpha ? 1.0f : col[3] here?

(please don't submit a force-pushed PR because I already have a locally modified PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I intended to break this out into a different bugfix PR and forgot, sorry. It's unrelated to the change in this PR but it's a bug I noticed as I was working on this stuff. According to the comment on this function and the docs on the NoAlpha flag, if NoAlpha is set, we aren't supposed to access col[3]; this modifies the behavior to not perform the access the docs say this function shouldn't do.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooh, you are totally right! Definitively a bug fix then! Thanks!

ocornut added a commit that referenced this pull request Mar 8, 2019
… contract of never reading the 4th float in the array (value was read and discarded). (#2384)
@ocornut
Copy link
Owner

ocornut commented Mar 8, 2019

This is now merged (with minor tweaks, changelog + fixes for static analyzer saying the R,G,B,H,S,V variables could be uninitialized).

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Information is lost on a round trip through ColorPicker4 for applications that store colors as HSV
2 participants