ocornut/imgui

Fork 0

mirror of https://github.com/ocornut/imgui.git synced 2026-11-10 19:03:30 +00:00

Color edit marked as changed without actual changes #6722

New issue

Closed

opened 2023-08-15 16:21:59 +00:00 by GamingMinds-DanielC · 8 comments

GamingMinds-DanielC commented

2023-08-15 16:21:59 +00:00

(Migrated from github.com)

Version/Branch of Dear ImGui:

Version: 1.89.8 WIP (18974)
Branch: docking

Back-end/Renderer/Compiler/OS

Back-ends: Win32 + DX11
Compiler: VS 2019 / 2022
Operating System: Win 10

My Issue/Question:

I used the color widget as a reference for a custom widget with mode switching and noticed an unwanted behavior that I then tried with the original color widget. Whenever the display mode is changed or the value is copied into the clipboard, the widget is marked as "deactivated after edit".

Standalone, minimal, complete and verifiable example:

		static float color[ 4 ] = { 1.0f, 1.0f, 1.0f, 1.0f };
		static int   counter    = 0;

		ImGui::Text(
		    "Right click on the widget to open the color edit options,\n"
		    "then change the display mode or copy the value into the\n"
		    "clipboard. Notice how the counter is increased." );

		ImGui::ColorEdit4( "Color", color );
		if ( ImGui::IsItemDeactivatedAfterEdit() )
			counter++;

		ImGui::Text( "Deactivated after Edit: %i", counter );

Source of the problem and possible solution:

The popup is first displayed and later opened with constant label "context" within the group that defines the widget, so any action within the popup affects the flags of the group.

In my custom widget, I solved this by displaying (if open) the popup before starting the group. To not have ID conflicts, I surrounded id with PushID("context") and PopID() and gave the popup itself the label of the widget as ID. I deferred the opening of the popup from within the group to after it, surrounding it with the same "context" ID again. This solved the issue for my custom widget, a change of display mode no longer marks the widget as having been edited. Maybe this fix is feasible for the color widget as well.

**Version/Branch of Dear ImGui:** Version: 1.89.8 WIP (18974) Branch: docking **Back-end/Renderer/Compiler/OS** Back-ends: Win32 + DX11 Compiler: VS 2019 / 2022 Operating System: Win 10 **My Issue/Question:** I used the color widget as a reference for a custom widget with mode switching and noticed an unwanted behavior that I then tried with the original color widget. Whenever the display mode is changed or the value is copied into the clipboard, the widget is marked as "deactivated after edit". **Standalone, minimal, complete and verifiable example:** ```c++ static float color[ 4 ] = { 1.0f, 1.0f, 1.0f, 1.0f }; static int counter = 0; ImGui::Text( "Right click on the widget to open the color edit options,\n" "then change the display mode or copy the value into the\n" "clipboard. Notice how the counter is increased." ); ImGui::ColorEdit4( "Color", color ); if ( ImGui::IsItemDeactivatedAfterEdit() ) counter++; ImGui::Text( "Deactivated after Edit: %i", counter ); ``` **Source of the problem and possible solution:** The popup is first displayed and later opened with constant label "context" within the group that defines the widget, so any action within the popup affects the flags of the group. In my custom widget, I solved this by displaying (if open) the popup before starting the group. To not have ID conflicts, I surrounded id with `PushID("context")` and `PopID()` and gave the popup itself the label of the widget as ID. I deferred the opening of the popup from within the group to after it, surrounding it with the same "context" ID again. This solved the issue for my custom widget, a change of display mode no longer marks the widget as having been edited. Maybe this fix is feasible for the color widget as well.

GamingMinds-DanielC commented

2023-08-16 12:09:05 +00:00

(Migrated from github.com)

I found a better way with less ID pushing and popping since the already present BeginGroup() and PushID() can be switched (as they are in the color picker). Next thing was to implement the fix and make a pull request, but I ran into some problems.

I managed to get the context menu of the color edit out of its group, same for the one from the color picker, all without changes to any interfaces. Changing the display mode then doesn't affect the flags of the group itself. But calling into the color picker from within the color edit (same for the use of color edits in the color picker) poses a problem. Those nested calls put their popups into the surrounding group, display style changes then mark the outer widget as edited again. And I can't pull the nested calls out of the group because then the group would loose legit edits.

So far I got no solution for that problem, so no pull request for now. A partial fix wouldn't be good since it would introduce an inconsistency into the marking as edited, based on where exactly the display options are changed. A basic idea maybe: if we had a flag that prevents items from affecting the status of the group they are in, that would allow for a full fix without the need to pull stuff out of groups.

I found a better way with less ID pushing and popping since the already present `BeginGroup()` and `PushID()` can be switched (as they are in the color picker). Next thing was to implement the fix and make a pull request, but I ran into some problems. I managed to get the context menu of the color edit out of its group, same for the one from the color picker, all without changes to any interfaces. Changing the display mode then doesn't affect the flags of the group itself. But calling into the color picker from within the color edit (same for the use of color edits in the color picker) poses a problem. Those nested calls put their popups into the surrounding group, display style changes then mark the outer widget as edited again. And I can't pull the nested calls out of the group because then the group would loose legit edits. So far I got no solution for that problem, so no pull request for now. A partial fix wouldn't be good since it would introduce an inconsistency into the marking as edited, based on where exactly the display options are changed. A basic idea maybe: if we had a flag that prevents items from affecting the status of the group they are in, that would allow for a full fix without the need to pull stuff out of groups.

GamingMinds-DanielC commented

2023-08-17 14:18:34 +00:00

(Migrated from github.com)

Found a rather elegant solution, pull request incoming...

ocornut commented

2023-08-25 20:49:35 +00:00

(Migrated from github.com)

Hello,

The checkbox/selectables inside those popups are calling MarkItemEdited() indeed.

It seems like you are introducing a very large feature to fix this specific case.
As we know ColorEditOptionsPopup() doesn't perform any edit, here's my suggestion:

imgui-57ef206-WIP LockMarkEdited (#6722).patch

This would also conveniently allow removing the small amount of code using ImGuiInputTextFlags_NoMarkEdited.

What do you think?

Hello, The checkbox/selectables inside those popups are calling `MarkItemEdited()` indeed. It seems like you are introducing a very large feature to fix this specific case. As we know `ColorEditOptionsPopup()` doesn't perform any edit, here's my suggestion: [imgui-57ef206-WIP LockMarkEdited (#6722).patch](https://github.com/ocornut/imgui/files/12443439/imgui-57ef206-WIP.LockMarkEdited.6722.patch) This would also conveniently allow removing the small amount of code using `ImGuiInputTextFlags_NoMarkEdited`. What do you think?

ocornut commented

2023-08-25 20:56:31 +00:00

(Migrated from github.com)

In principles I can see that #6731 provides a wider feature, but I cannot see a real use for it presently, other than solving this very specific problem you are having which can be solved with those ~6 lines of code changes, so unless you see an issue this situation I'd be always tempted to pick the simplest one.

GamingMinds-DanielC commented

2023-08-27 02:04:22 +00:00

(Migrated from github.com)

Your version sure is a lot simpler in terms of code changes. It leaves the activation, deactivation and hovering reports in, but the main issue (deactivated after edit) should be solved*. Well, after adding two more lines in ColorPickerOptionsPopup (that one has the same problem as ColorEditOptionsPopup) it will be. That is already enough for my purposes (not filling up the undo stack with non-edits).

* Not tested yet since I'm not at my office PC at weekends, just judging by looking at the patch.

Your version sure is a lot simpler in terms of code changes. It leaves the activation, deactivation and hovering reports in, but the main issue (deactivated after edit) should be solved*. Well, after adding two more lines in `ColorPickerOptionsPopup` (that one has the same problem as `ColorEditOptionsPopup`) it will be. That is already enough for my purposes (not filling up the undo stack with non-edits). \* Not tested yet since I'm not at my office PC at weekends, just judging by looking at the patch.

ocornut commented

2023-08-28 08:43:04 +00:00

(Migrated from github.com)

Hello Daniel,

I think Activation/Deactivation aren't too problematic but I imagine someone could come up with a scenario where they'd like Hovered to not be set (e.g. for an overlay). For now I committed my solution as 33ea1e8 because we know it solves the most-common issue.

I'll close #6731 for now but I think there is a possibility we'll come back to it in some way, in which case your proposal seems close to being the right solution and I would likely use it as a base.
Thanks a lot !

(not filling up the undo stack with non-edits).

As a tangential comment, if you use a InputInt() to type one value without pressing Enter, then edit it back to initial value and deactivate you'd face the same issue, and I believe an Undo system should be able to "discard" the non-edit based on actual data.

Hello Daniel, I think Activation/Deactivation aren't too problematic but I imagine someone could come up with a scenario where they'd like Hovered to not be set (e.g. for an overlay). For now I committed my solution as 33ea1e8 because we know it solves the most-common issue. I'll close #6731 for now but I think there is a possibility we'll come back to it in some way, in which case your proposal seems close to being the right solution and I would likely use it as a base. Thanks a lot ! > (not filling up the undo stack with non-edits). As a tangential comment, if you use a `InputInt()` to type one value without pressing Enter, then edit it back to initial value and deactivate you'd face the same issue, and I believe an Undo system should be able to "discard" the non-edit based on actual data.

GamingMinds-DanielC commented

2023-08-28 09:03:45 +00:00

(Migrated from github.com)

I was just about to report back that I tested the patch and it works as expected. I did apply the locking to the picker popup as well, exactly as you did in the actual commit just now.

As a tangential comment, if you use a InputInt() to type one value without pressing Enter, then edit it back to initial value and deactivate you'd face the same issue, and I believe an Undo system should be able to "discard" the non-edit based on actual data.

Usually yes, but in some cases having an undo for explicitly edited values even if they didn't change can be expected and can make some things easier (f.e. with automation that makes a "change", exports some data and undoes the change). Just like pushing a style on a stack, using it and popping it again is easier than checking if the style actually changes and making the push and pop conditional.

I was just about to report back that I tested the patch and it works as expected. I did apply the locking to the picker popup as well, exactly as you did in the actual commit just now. > As a tangential comment, if you use a `InputInt()` to type one value without pressing Enter, then edit it back to initial value and deactivate you'd face the same issue, and I believe an Undo system should be able to "discard" the non-edit based on actual data. Usually yes, but in some cases having an undo for explicitly edited values even if they didn't change can be expected and can make some things easier (f.e. with automation that makes a "change", exports some data and undoes the change). Just like pushing a style on a stack, using it and popping it again is easier than checking if the style actually changes and making the push and pop conditional.

👍 1

ocornut commented

2023-08-28 09:09:27 +00:00

(Migrated from github.com)

Usually yes, but in some cases having an undo for explicitly edited values even if they didn't change can be expected

Great point, I hadn't thought about that.

> Usually yes, but in some cases having an undo for explicitly edited values even if they didn't change can be expected Great point, I hadn't thought about that.