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 a color picker UI element #740

Merged
merged 3 commits into from
Jul 17, 2023
Merged

Conversation

Krzysztof-Dmitruk-Mobica
Copy link
Contributor

Description

The color picker is available in ImGui, but not in the Drawer class, so I added it there. The color picker functionality is useful for the dynamic blending sample I'm working on now. I used the color write enable sample to show the color picker in action.

General Checklist:

Please ensure the following points are checked:

  • My code follows the coding style

  • I have reviewed file licenses

  • I have commented any added functions (in line with Doxygen)

  • I have commented any code that could be hard to understand

  • My changes do not add any new compiler warnings

  • My changes do not add any new validation layer errors or warnings

  • I have used existing framework/helper functions where possible

  • My changes do not add any regressions

  • I have tested every sample to ensure everything runs correctly

  • This PR describes the scope and expected impact of the changes I am making

Note: The Samples CI runs a number of checks including:

  • I have updated the header Copyright to reflect the current year (CI build will fail if Copyright is out of date)
  • My changes build on Windows, Linux, macOS and Android. Otherwise I have documented any exceptions

1
2

tomadamatkinson
tomadamatkinson previously approved these changes Jul 4, 2023
Copy link
Collaborator

@tomadamatkinson tomadamatkinson left a comment

Choose a reason for hiding this comment

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

LGTM! I think we should push to use more complex UI items like this. We should look at using Implot and possibly imnode in the future

@SaschaWillems SaschaWillems self-requested a review July 8, 2023 18:29
SaschaWillems
SaschaWillems previously approved these changes Jul 8, 2023
Copy link
Contributor

@asuessenbach asuessenbach left a comment

Choose a reason for hiding this comment

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

Nice idea.
Would you mind to port the color_picker function to hpp_gui as well?

@@ -1245,4 +1245,26 @@ void Drawer::text(const char *formatstr, ...)
va_end(args);
}

bool Drawer::color_picker(const char *caption, const char channel_count, float *color, uint16_t width, ImGuiColorEditFlags flags)
{
assert((channel_count == 3 || channel_count == 4) && "Channel count value must be 3 or 4");
Copy link
Contributor

Choose a reason for hiding this comment

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

If channel_count needs to be 3 or 4, you could use some bool parameter. Or maybe an enum with just two possible values.

To stop the separation of channel_count and color, and still let the compiler tell you when you're wrong, you could also change the interface to a template on channel_count, like this:

	template <size_t N>
	bool color_picker(std::string const &caption, std::array<float, N> &colors, uint16_t width = 0, ImGuiColorEditFlags flags = 0)
	{
		static_assert((N == 3) || (N == 4), "booh");

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 felt bad about making my method the first template in the Drawer class if it wasn't necessary. Instead, I created two overloaded functions for different sizes of the array. I think this solution is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, instead of four functions, each one doing more or less the same, I would prefer a templatized approach, like this:

	enum class ColorOp
	{
		Edit,
		Pick
	};

	template <ColorOp OP, size_t N>
	bool color_op(const std::string & caption, std::array<float, N> &color, float width = 0.0f, ImGuiColorEditFlags flags = 0)
	{
		static_assert((N == 3) || (N == 4), "booh");

		ImGui::PushItemWidth(width);
		bool res = color_op_impl<OP, N>(caption.c_str(), color.data(), flags);
		ImGui::PopItemWidth();
		if (res)
			dirty = true;
		return res;
	}

With some private implementation functions (unfortunately, we're at C++14, not C++17, where we could use some constexpr if instead) like that:

	template <ColorOp OP, size_t N>
	bool color_op_impl(const char *caption, float *colors, ImGuiColorEditFlags flags)
	{
		assert(false);
	}

	template <>
	bool color_op_impl<ColorOp::Edit, 3>(const char *caption, float *colors, ImGuiColorEditFlags flags)
	{
		return ImGui::ColorEdit3(caption, colors, flags);
	}

	template <>
	bool color_op_impl<ColorOp::Edit, 4>(const char *caption, float *colors, ImGuiColorEditFlags flags)
	{
		return ImGui::ColorEdit4(caption, colors, flags);
	}

	template <>
	bool color_op_impl<ColorOp::Pick, 3>(const char *caption, float *colors, ImGuiColorEditFlags flags)
	{
		return ImGui::ColorPicker3(caption, colors, flags);
	}

	template <>
	bool color_op_impl<ColorOp::Pick, 4>(const char *caption, float *colors, ImGuiColorEditFlags flags)
	{
		return ImGui::ColorPicker4(caption, colors, flags);
	}

But, of course, your solution works as well, and if nobody else objects, I'd be ok as well.
(As there's no vulkan stuff with this set of functions, we could even move them out to some place, where they could be called by both, vkb::Drawer and vkb::HPPDrawer. But that might be a different story.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After giving it a second thought, I really liked your idea. I changed the code according to it.

Replace sliders in the color_write_enable sample with a color picker
element.
Refactor color picker headers
Add support in HPPDrawer
@gary-sweet
Copy link
Contributor

LGTM! I think we should push to use more complex UI items like this. We should look at using Implot and possibly imnode in the future

Just please be aware that not all platforms have very usable inputs for the samples. I have to drive the samples from the Linux console using the keyboard inputs. I'm not sure how well that will work for complex input UI items.

@gary-sweet
Copy link
Contributor

LGTM! I think we should push to use more complex UI items like this. We should look at using Implot and possibly imnode in the future

Just please be aware that not all platforms have very usable inputs for the samples. I have to drive the samples from the Linux console using the keyboard inputs. I'm not sure how well that will work for complex input UI items.

Turns out that the color picker can be driven from the keyboard, so no issue with this one at least.

@marty-johnson59 marty-johnson59 merged commit 07d2721 into KhronosGroup:main Jul 17, 2023
23 checks passed
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.

6 participants