Fix ColorPicker committing extra action to history

[aXu-AP]

Fixes #83642 Fixes #45186
ColorPicker committed value on both on color_change and popup_closed signals, so same value got submitted twice to undoredo.

Addionally, ColorPicker changed set value upon exit due to HTML value getting submitted. Since HTML color codes are 32-bit color, but Godot uses higher precision colors internally, this meant that color from HTML value differed from currently selected value, thus it was submitted resulting less precision on colors chosen via picker.
Fixed by checking if color is different with precision offered by 32-bit color.

EDIT: Combined #83787 into this pr since they are both needed to fix the problem.

[tokengamedev]

Can you check, #82514

[KoBeWi]

This is the same as #44895 and introduces the same regression.

[aXu-AP]

Hmm, that's a bit though. Does visual shader skip changing values to avoid recompiling the shader? In that case the requirements for VS editor and inspector are different. Is there any way to know from inside EditorProperty what context is it in?
Or could the inspector check if the latest value is the same as the current and not commit to undoredo?

[KoBeWi]

Or could the inspector check if the latest value is the same as the current and not commit to undoredo?

Such checks should be performed by the EditorProperty itself. There's this condition in the code you deleted:

if (picker->get_pick_color() != last_color) {

Its problem is that it compares the color stored at opening the picker, not after the changes. Though fixing this would probably still affect VisualShader.

Is there any way to know from inside EditorProperty what context is it in?

The EditorProperty is manually created in VisualShader (I think?), so there might be some way to pass this information.

[aXu-AP]

Looks like only other EditorProperty which uses the changing flag is EditorPropertyMultilineText, which ever sends only changing signal. So if multiline string property was added to a visual shader, it would also never trigger shader compilation.

I think that EditorProperty's interface needs to be better described and build usage of that interface depending on it. One cannot rely on a interface that is implemented differently by inheriting classes. Here's some possible ways to do it:

1. Either property_changed sends always changing flag or not.
   • In this case Visual shader editor could be modified to handle the change after a delay if changing is true (similiar to how text based shader is handled).
2. property_changed can have changing flag, but always at the end should send one signal without it is as a "confirmation".
   • In this case Inspector could ignore property change if changing is true after changing is false.
3. Variation of the option 2, add signal confirmed which is used instead in the end.
   • Inspector would simply ignore the confirmed signal.
   • Visual shader editor caches the value from property_changed with changing and applies it on confirmed.

I'm a bit leaning towards option 1 being the easiest to implement, since visual shader editor seems to be the only one ignoring changing properties now (haven't checked though).

[bs-mwoerner]

I think this check may be too late here. At this point, the internal color state (that's a class member, not a local variable) has already been overwritten with the return value of Color::from_string, so this only skips the change notification, doesn't it?

[aXu-AP]

Hmm, you're right.

[YuriSizov]

I assume this is still needed to address linked issues, even after #85749? If so, could you please rebase your PR?

[aXu-AP]

As @KoBeWi said, this pr introduces regression on visual shader color picker, so more than just a rebase is needed. I would appreciate feedback on which, if any, of my 3 options sounds most feasible (see my comment above).

[akien-mga]

Superseded by #88690. Thanks for the contribution!