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

Added Color Picker Widget #145

Merged
merged 1 commit into from
Sep 5, 2020
Merged

Added Color Picker Widget #145

merged 1 commit into from
Sep 5, 2020

Conversation

1aam2am1
Copy link
Contributor

@1aam2am1 1aam2am1 commented Sep 2, 2020

Added simple functional color picker widget.
Styled on gimp, with HSV circle and value sliders.

@texus
Copy link
Owner

texus commented Sep 3, 2020

It definitely has potential, but I did notice several issues after some quick testing.

First major issue that immediately stands out: the canvas is empty for me. If I clear it with something other than ColorTransparent then I do see the color used for clearing, but the m_canvas->draw just doesn't seem to draw anything for me. The only thing I tested so far is checking m_HSV->loadFromMemory and that line returned true, so I'm not sure why it isn't working.

When the program starts, moving the mouse inside the window already starts moving the colors, before I even clicked on the canvas.

When the mouse is outside the canvas while you are still dragging, the colors are still being changed. The fact that the colors are being changed is fine, the problem is that the values just don't make sense to me. If the mouse is e.g. too far to the right then the color should just be the one at the right side of the canvas at the y-position of the mouse. If the mouse is both below and too far to the right then the color should always equal the one from the corner, while right not it is still changing depending on where the mouse is exactly.
Edit: I didn't see how the image looked previously. Clamping does seem to be working.

If the mouse leaves the child window while dragging and is released outside it, then moving it back into the window causes the color picker to think that the mouse is still down. You probably need to override the leftMouseButtonNoLongerDown() function instead of leftMouseReleased(). The leftMouseReleased function is currently only called when the mouse is on top of the widget while leftMouseButtonNoLongerDown will also be called when released outside it after dragging.

Edit boxes should probably be numeric only (using edit->setInputValidator(EditBox::Validator::Int)) to prevent text from being typed.

TGUI 0.8 still supports SFML 2.3.2 while the setUniform function was only added in SFML 2.4. So the "m_HSV->setUniform(...)" calls should be replaced with the following lines:

#if SFML_VERSION_MAJOR == 2 && SFML_VERSION_MINOR < 4
    m_HSV->setParameter("V", ...);
    m_HSV->setParameter("resolution", ...);
#else
    m_HSV->setUniform("V", ...);
    m_HSV->setUniform("resolution", ...);
#endif

Some personal preferences:

  • I would rename "Back" to "Reset".
  • I find the window a bit too large. If you are going to keep everything at the same size then at least you should put the last and current color below the sliders to not have large empty areas in the window.
  • When I said I liked the one from GIMP, I meant this one:
    image
    I would already be happy to have some way to select the color, so I would still merge this PR with your shader, it's just not the one I thought you were going to make.

@texus
Copy link
Owner

texus commented Sep 3, 2020

I managed to get the shader to work by adding gl_FragColor.a = 1.0 to your shader.

I'm ok with keeping this color wheel for now. It's understandable now that I see what it looks like (I was just guessing before based on the color that was being chosen when I clicked somewhere in the canvas).

@1aam2am1
Copy link
Contributor Author

1aam2am1 commented Sep 3, 2020

I will repair the errors that you have found.
And will do force push.

@texus
Copy link
Owner

texus commented Sep 4, 2020

There are still a few small things that need to be changed. But after the following changes it will probably be finished enough to be merged.

  • OkKeyPressed should probably be renamed, it isn't a key but a button. But maybe "OkPressed" is easier than "OkButtonPressed"? Either are fine for me.

  • The color is still changing before I click in the canvas. The problem seems to be that m_colorRead is uninitialized and it can thus be true. Just adding "= false" behind the variable in the header file fixes the issue.

  • The "Cancel" and "Ok" buttons should be swapped and "Ok" should become "OK" (capital letter). This will make it more consistent with GIMP (and it looks weird to me to have an ok button in the middle of other buttons).

  • SFML 2.3 didn't have sf::TriangleStrip yet, you have to use sf::TrianglesStrip (note the extra "s").

  • Even though both work, "static double a" in "logInvCurve" should use "const" instead of "static".

  • There are warnings about double to float conversions when calling logInvCurve. The function should probably return float (TGUI doesn't use double for consistency with SFML). If you feel like the extra precision is useful in std::log then you can keep using a double inside that function, but just change the return type and parameter to float so you only have to cast in one place (in the function) instead of at 4 places (every time the function is called).

  • In c++14 mode you can't use sf::Color in the callback function. You missed one spot in your changes. The last dereference function in SignalImpl.hpp (line 86 in your code) should only be enabled when none of the other templates matches. So you should change the template to the following (adding the sf::Color check):

        template <typename Type, typename std::enable_if<!std::is_same<Type, std::string>::value
                                                      && !std::is_same<Type, sf::Vector2f>::value
                                                      && !std::is_same<Type, sf::Color>::value>::type* = nullptr>

Signed-off-by: Michal_Marszalek <1aam2am1@gmail.com>
@texus
Copy link
Owner

texus commented Sep 5, 2020

Thanks for contributing the widget and fixing the issues with it.

@texus texus merged commit e5fd0f5 into texus:0.8 Sep 5, 2020
texus pushed a commit that referenced this pull request Sep 5, 2020
Signed-off-by: Michal_Marszalek <1aam2am1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants