fix many color picker related things:

- make ColorPickerScene the correct size, use a background and a line surrounding it, if not in fullscreen mode, swallow actions, since It doesn't stop updating, since the background scene still receives callback, when the color changes
-  fix images, SDL_image was reporting unsupported sRGB profile, and the didn't have the same amount of edges
- use double instead of const double& in all slider related callbacks
- use better colors in the settings as example
- fix callback hell in many places, in slider, and in color picker, which means, that a callback is only fired, when needed, when the color isn#t changed, it is skipped
- fix some issues, where widgets would interpret the ButtonUp of the click as end of dragging, now  it only interprets that as the end of teh dragging, if it is  currently dragging
- fix color picker conversions, by not double converting in many places (e.g. rgb-> hsv->rgb) which reduced color glitches on input
- fix inversion of the v value, (s,v) (0,0) is on the bottom left, not top left
- changed handling of after_color_change, for the future and also to support some things better

Author: Totto16

assets/icons/hsv_color_selector.png

@@ -145,8 +145,6 @@ void DiscordInstance::clear_activity(bool wait) {
 
 
 DiscordActivityWrapper::DiscordActivityWrapper(const std::string& details, discord::ActivityType type) {
-
-
     m_activity.SetDetails(details.c_str());
     m_activity.SetType(type);
 }

assets/icons/rgb_color_selector.png

@@ -83,22 +83,37 @@ detail::ColorPickerScene::ColorPickerScene(
     : Scene{
           service_provider, layout
     }, 
-    m_color_picker{ service_provider, starting_color, std::move(callback), std::pair<double, double>{ 0.1, 0.3 }, ui::Alignment{ ui::AlignmentHorizontal::Middle, ui::AlignmentVertical::Bottom }, layout, false } { }
+    m_color_picker{ service_provider, starting_color, std::move(callback), std::pair<double, double>{ 0.95, 0.95 }, ui::Alignment{ ui::AlignmentHorizontal::Middle, ui::AlignmentVertical::Bottom }, layout, false } { }
 
 [[nodiscard]] scenes::Scene::UpdateResult detail::ColorPickerScene::update() {
     if (m_should_exit) {
-        if (m_should_exit) {
-            return UpdateResult{ scenes::SceneUpdate::StopUpdating, Scene::Pop{} };
-        }
+        return UpdateResult{ scenes::SceneUpdate::StopUpdating, Scene::Pop{} };
     }
     return UpdateResult{ scenes::SceneUpdate::StopUpdating, helper::nullopt };
 }
 
 void detail::ColorPickerScene::render(const ServiceProvider& service_provider) {
+    service_provider.renderer().draw_rect_filled(get_layout().get_rect(), Color::black());
+    if (not get_layout().is_full_screen()) {
+        service_provider.renderer().draw_rect_outline(get_layout().get_rect(), Color::white());
+    }
+
     m_color_picker.render(service_provider);
 }
 bool detail::ColorPickerScene::handle_event(const SDL_Event& event, const Window* window) {
-    return m_color_picker.handle_event(event, window);
+
+    if (utils::event_is_action(event, utils::CrossPlatformAction::CLOSE)) {
+        m_should_exit = true;
+        return true;
+    }
+
+    const auto result = m_color_picker.handle_event(event, window);
+    if (result) {
+        return result;
+    }
+
+    // swallow all events
+    return true;
 }
 
 

src/discord/core.cpp

@@ -25,7 +25,7 @@ namespace scenes {
                     std::pair<double, double>{ 0.05, 0.03 },
                     layout
     },
-    m_colors{COLOR_LITERAL("#FF33FF"), COLOR_LITERAL("hsv(120, 0.07, 0.93)"), COLOR_LITERAL("hsv(140, 0.07, 0.93)"), COLOR_LITERAL("rgb(246, 255, 61)")}
+    m_colors{COLOR_LITERAL("#FF33FF"),  COLOR_LITERAL("hsv(281.71, 0.70085, 0.45882)"), COLOR_LITERAL("rgb(246, 255, 61)"),COLOR_LITERAL("hsv(103.12, 0.39024, 0.32157)")}
 {
         auto focus_helper = ui::FocusHelper{ 1 };
 
@@ -56,7 +56,7 @@ namespace scenes {
                     const auto value = service_provider->music_manager().get_volume();
                     return value.has_value() ? value.value() : 0.0F;
                 },
-                [service_provider](const double& amount) {
+                [service_provider](double amount) {
                     const auto mapped_amount = amount <= 0.0F ? helper::nullopt : helper::optional<double>{ amount };
                     return service_provider->music_manager().set_volume(mapped_amount, false, false);
                 },

src/scenes/settings_menu/color_setting_row.cpp

@@ -2,6 +2,7 @@
 
 #include <functional>
 #include <spdlog/spdlog.h>
+#include <type_traits>
 #include <utility>
 
 #include "graphics/rect.hpp"
@@ -15,8 +16,7 @@ namespace ui {
     public:
         using Range = std::pair<Type, Type>;
         using Getter = std::function<Type()>;
-        using Setter = std::function<void(const Type&)>;
-
+        using Setter = std::function<void(Type)>;
 
     private:
         Range m_range;
@@ -155,10 +155,12 @@ namespace ui {
                     }
 
                 } else if (utils::event_is_click_event(event, utils::CrossPlatformClickEvent::ButtonUp)) {
-                    m_is_dragging = false;
-                    SDL_CaptureMouse(SDL_FALSE);
-                    handled = true;
-
+                    // only handle this, if already dragging, otherwise it's a button down from previously or some other widget
+                    if (m_is_dragging) {
+                        m_is_dragging = false;
+                        SDL_CaptureMouse(SDL_FALSE);
+                        handled = true;
+                    }
                 } else if (utils::event_is_click_event(event, utils::CrossPlatformClickEvent::Motion)) {
 
                     if (m_is_dragging) {
@@ -201,7 +203,6 @@ namespace ui {
 
         void on_change() {
             m_current_value = m_getter();
-            m_setter(m_current_value);
             change_layout();
         }
     };

src/scenes/settings_menu/settings_menu.cpp

@@ -3,7 +3,9 @@
 #include "color_picker.hpp"
 #include "graphics/point.hpp"
 #include "graphics/rect.hpp"
+#include "helper/color.hpp"
 #include "helper/graphic_utils.hpp"
+#include "helper/utils.hpp"
 #include "manager/resource_manager.hpp"
 #include "ui/components/textinput.hpp"
 #include "ui/layout.hpp"
@@ -35,9 +37,8 @@ detail::ColorSlider::ColorSlider(
     const auto h = bar_rect().height();
 
     for (u32 x = 0; x < w; x++) {
-        const Color color{
-            HSVColor{(static_cast<double>(x) / static_cast<double>(w)) * 360.0, 1.0, 1.0}
-        };
+        const Color color =
+                HSVColor((static_cast<double>(x) / static_cast<double>(w)) * 360.0, 1.0, 1.0).to_rgb_color();
 
         service_provider->renderer().draw_line(
                 shapes::UPoint{ x, 0 }, shapes::UPoint{ x, static_cast<u32>(h - 1) }, color
@@ -122,17 +123,19 @@ void detail::ColorCanvas::render(const ServiceProvider& service_provider) const
 void detail::ColorCanvas::draw_pseudo_circle(const ServiceProvider& service_provider) const {
     const auto& renderer = service_provider.renderer();
 
-    u32 diameter = 2;
+    u32 diameter = 5;
 
     // width == height here, since we assured that in the construction of the layout fro this component, so instead of taking the min of width and height i just use width, this fact is only used here, since it's not that bad, if it changes sometime in teh future
-    if (const double one_percent = static_cast<double>(layout().get_rect().width()) * 0.01; one_percent >= 2.0) {
-        diameter = static_cast<u32>(one_percent);
+    if (const double percentage_diameter = static_cast<double>(layout().get_rect().width()) * 0.03;
+        percentage_diameter >= static_cast<double>(diameter)) {
+        diameter = static_cast<u32>(percentage_diameter);
     }
 
     const auto [width, height] = layout().get_rect().to_dimension_point().cast<double>();
 
-    const auto center = shapes::AbstractPoint<double>(m_current_color.s * width, m_current_color.v * height).cast<u32>()
-                        + layout().get_rect().top_left;
+    const auto center =
+            shapes::AbstractPoint<double>(m_current_color.s * width, (1.0 - m_current_color.v) * height).cast<u32>()
+            + layout().get_rect().top_left;
 
     const auto circle_color = m_is_dragging ? "#C7C7C7"_c : "#FFFFFF"_c;
 
@@ -158,11 +161,12 @@ detail::ColorCanvas::handle_event(const SDL_Event& event, const Window* window)
                 };
             }
         } else if (utils::event_is_click_event(event, utils::CrossPlatformClickEvent::ButtonUp)) {
-
-            m_is_dragging = false;
-            SDL_CaptureMouse(SDL_FALSE);
-            handled = true;
-
+            // only handle this, if already dragging, otherwise it's a button down from previously or some other widget
+            if (m_is_dragging) {
+                m_is_dragging = false;
+                SDL_CaptureMouse(SDL_FALSE);
+                handled = true;
+            }
         } else if (utils::event_is_click_event(event, utils::CrossPlatformClickEvent::Motion)) {
 
             if (m_is_dragging) {
@@ -174,8 +178,9 @@ detail::ColorCanvas::handle_event(const SDL_Event& event, const Window* window)
 
     if (handled) {
 
-        const auto& [x, y] = utils::get_raw_coordinates(window, event);
+        const auto previous_color = m_current_color;
 
+        const auto& [x, y] = utils::get_raw_coordinates(window, event);
 
         if (x <= static_cast<i32>(fill_rect.top_left.x)) {
             m_current_color.s = 0.0;
@@ -188,15 +193,20 @@ detail::ColorCanvas::handle_event(const SDL_Event& event, const Window* window)
         }
 
         if (y <= static_cast<i32>(fill_rect.top_left.y)) {
-            m_current_color.v = 0.0;
-        } else if (y >= static_cast<i32>(fill_rect.bottom_right.y)) {
             m_current_color.v = 1.0;
+        } else if (y >= static_cast<i32>(fill_rect.bottom_right.y)) {
+            m_current_color.v = 0.0;
         } else {
             const double percentage =
-                    static_cast<double>(y - fill_rect.top_left.y) / static_cast<double>(fill_rect.height());
+                    1.0 - static_cast<double>(y - fill_rect.top_left.y) / static_cast<double>(fill_rect.height());
             m_current_color.v = percentage;
         }
 
+        // if we hover at e.g. the edges, and don't change anything, we don't need to call the callback
+        if (previous_color.s == m_current_color.s && previous_color.v == m_current_color.v) {
+            return handled;
+        }
+
         const shapes::AbstractPoint<double> point = { m_current_color.s, m_current_color.v };
         m_callback(point);
 
@@ -209,8 +219,25 @@ detail::ColorCanvas::handle_event(const SDL_Event& event, const Window* window)
     return false;
 }
 
-void detail::ColorCanvas::on_change(const Color& color) {
-    m_current_color = color.to_hsv_color();
+void detail::ColorCanvas::on_change(ColorChangeOrigin origin, const HSVColor& color) {
+    switch (origin) {
+        case detail::ColorChangeOrigin::TextInput: {
+            m_current_color = color;
+            break;
+        }
+        case detail::ColorChangeOrigin::Canvas: {
+            //nothing to do, shouldn't be reached
+            utils::unreachable();
+            break;
+        }
+        case detail::ColorChangeOrigin::Slider: {
+            // only need to change the h value
+            m_current_color.h = color.h;
+            break;
+        }
+        default:
+            utils::unreachable();
+    }
 
     redraw_texture();
 }
@@ -230,14 +257,11 @@ void detail::ColorCanvas::redraw_texture() {
 
     const auto hue = m_current_color.h;
 
+    //TODO: try to speed this up, since it is a performance bottle neck, if hovering like a madman (xD) over the color slider
     for (u32 y = 0; y < h; y++) {
+        const auto v = 1.0 - (static_cast<double>(y) / static_cast<double>(h));
         for (u32 x = 0; x < w; x++) {
-            const Color color{
-                HSVColor{
-                         hue, static_cast<double>(x) / static_cast<double>(w),
-                         1.0 - (static_cast<double>(y) / static_cast<double>(h)),
-                         }
-            };
+            const Color color = HSVColor(hue, static_cast<double>(x) / static_cast<double>(w), v).to_rgb_color();
 
             m_service_provider->renderer().draw_pixel(shapes::UPoint{ x, y }, color);
         }
@@ -276,7 +300,7 @@ ui::ColorPicker::ColorPicker(
                 hsv_color.s = value.x;
                 hsv_color.v = value.y;
                 this->m_color = Color{ hsv_color };
-                this->after_color_change(ColorChangeType::SV);
+                this->after_color_change(detail::ColorChangeOrigin::Canvas, hsv_color);
             },
             ui::Layout{ main_rect }, false
     );
@@ -313,11 +337,21 @@ ui::ColorPicker::ColorPicker(
     m_color_slider = std::make_unique<detail::ColorSlider>(
             service_provider, std::pair<double, double>{ 0.0, 360.0 },
             [this]() -> double { return this->m_color.to_hsv_color().h; },
-            [this](const double& value) {
+            [this](double value) {
                 auto hsv_color = m_color.to_hsv_color();
                 hsv_color.h = value;
+
+                const auto previous_color = this->m_color;
+
                 this->m_color = Color{ hsv_color };
-                this->after_color_change(ColorChangeType::Hue);
+
+                // if we hover at e.g. the edges, and don't change anything, we don't need to call after_color_change
+                // this also helps, since the slider reports change on click and on release, even if teh position doesn't change there
+                if (previous_color == this->m_color) {
+                    return;
+                }
+
+                this->after_color_change(detail::ColorChangeOrigin::Slider, hsv_color);
             },
             5.0, ui::Layout{ color_bar_rect }, false
     );
@@ -334,7 +368,6 @@ ui::ColorPicker::ColorPicker(
 
     const auto rgb_image_path = utils::get_assets_folder() / "icons" / "rgb_color_selector.png";
 
-
     m_rgb_button = std::make_unique<ui::ImageButton>(
             service_provider, rgb_image_path, true, focus_id_unused,
             [this](const ImageButton&) -> bool {
@@ -369,7 +402,9 @@ ui::ColorPicker::ColorPicker(
             ui::Alignment{ ui::AlignmentHorizontal::Middle, ui::AlignmentVertical::Center }, textinput_layout, false
     );
 
-    after_color_change(ui::ColorChangeType::Both);
+    // using SLider, that behaviour simulates the one, that I want, since the ColorSlider already gets the change by the getter,
+    // but the textinput and the canvas need to be set manually by this call
+    after_color_change(detail::ColorChangeOrigin::Slider, m_color.to_hsv_color());
 }
 
 ui::ColorPicker::ColorPicker(
@@ -423,21 +458,32 @@ ui::ColorPicker::handle_event(const SDL_Event& event, const Window* window) {
     return m_color_canvas->handle_event(event, window);
 }
 
-void ui::ColorPicker::after_color_change(ColorChangeType type) {
-    if (type != ColorChangeType::Hue) {
-        m_color_slider->on_change();
-    }
+void ui::ColorPicker::after_color_change(detail::ColorChangeOrigin origin, const HSVColor& color) {
+    switch (origin) {
+        case detail::ColorChangeOrigin::TextInput: {
+            m_color_slider->on_change();
 
-    if (type != ColorChangeType::SV) {
-        m_color_canvas->on_change(m_color);
+            m_color_canvas->on_change(origin, color);
+            break;
+        }
+        case detail::ColorChangeOrigin::Canvas: {
+            //TODO: change the text in the textinput!
+            break;
+        }
+        case detail::ColorChangeOrigin::Slider: {
+            m_color_canvas->on_change(origin, color);
+
+            //TODO: change the text in the textinput!
+            break;
+        }
+        default:
+            utils::unreachable();
     }
 
     m_callback(m_color);
-
-    //TODO: change the text in the textinput!
 }
 
 void ui::ColorPicker::after_color_mode_change() {
     //TODO
-    //TODO: handle textinput chnages and events and also change it's vaöue every time the color is change
+    //TODO: handle textinput chnages and events and also change it's value every time the color is changed
 }

src/ui/components/abstract_slider.hpp

@@ -14,6 +14,7 @@
 namespace detail {
 
     // it is intended, that this never has focus, than the scroll wheel doesn't work, but it shouldn't work, since scrolling a color slider isn't intended behaviour
+    //TODO: it can scroll, fix that !
     struct ColorSlider : public ui::AbstractSlider<double> {
     private:
         Texture m_texture;
@@ -36,6 +37,8 @@ namespace detail {
     };
 
 
+    enum class ColorChangeOrigin : u8 { TextInput, Canvas, Slider };
+
     struct ColorCanvas : public ui::Widget {
     public:
         using Callback = std::function<void(const shapes::AbstractPoint<double>& value)>;
@@ -62,7 +65,7 @@ namespace detail {
 
         Widget::EventHandleResult handle_event(const SDL_Event& event, const Window* window) override;
 
-        void on_change(const Color& color);
+        void on_change(ColorChangeOrigin origin, const HSVColor& color);
 
         void draw_pseudo_circle(const ServiceProvider& service_provider) const;
 
@@ -78,9 +81,6 @@ namespace ui {
 
     enum class ColorMode : u8 { RGB, HSV };
 
-    enum class ColorChangeType : u8 { Both, Hue, SV };
-
-
     struct ColorPicker final : public Widget {
         using Callback = std::function<void(const Color&)>;
 
@@ -122,7 +122,7 @@ namespace ui {
         [[nodiscard]] Color get_color() const;
 
     private:
-        void after_color_change(ColorChangeType type);
+        void after_color_change(detail::ColorChangeOrigin origin, const HSVColor& color);
         void after_color_mode_change();
     };