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

Don't emit changed signal on Color Picker close #44895

Merged
merged 1 commit into from
Jan 5, 2021

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Jan 3, 2021

I just noticed that #40879 is still not fixed (although I'm pretty sure I tested the PR). The culprit was the _popup_closed method in EditorPropertyColor. It emitted the modified signal every time the color picker popup was closed, which is just dumb. Even if it was necessary for something, looks like _color_changed() is called anyways when the popup is closed, which resulted in double set calls when the color was modified:
0I71fE10Ih

btw I inspected the _color_changed() call on close and it seems to be called with a slightly different color. Dunno what is it, maybe ColorPicker doesn't set proper RGB value and it's corrected (rounded?) on close.

@KoBeWi KoBeWi added bug topic:editor cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Jan 3, 2021
@KoBeWi KoBeWi added this to the 4.0 milestone Jan 3, 2021
@Chaosus
Copy link
Member

Chaosus commented Jan 3, 2021

Well, I think you should test it with VisualShader's Color constant node - when the color picker popup is going to close it should apply its color to the node.

@KoBeWi
Copy link
Member Author

KoBeWi commented Jan 3, 2021

The color is updated even before the dialog is closed:
CDj60uyiRt
So everything is still fine.

@Chaosus
Copy link
Member

Chaosus commented Jan 3, 2021

Alright, then, I'm fine with it.

@@ -2191,7 +2187,6 @@ EditorPropertyColor::EditorPropertyColor() {
add_child(picker);
picker->set_flat(true);
picker->connect("color_changed", callable_mp(this, &EditorPropertyColor::_color_changed));
picker->connect("popup_closed", callable_mp(this, &EditorPropertyColor::_popup_closed));
Copy link
Member

Choose a reason for hiding this comment

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

There's now nothing connected to the popup_closed signal.

It used to be emitted when receiving modal_closed, which happens when the ColorPicker popup receives NOTIFICATION_MODAL_CLOSE. Are you sure that this doesn't trigger in situations where you might still want to validate the input?

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you sure that this doesn't trigger in situations where you might still want to validate the input?

That could only happen when you modify the color without triggering _color_changed() and then close the popup. AFAIK it's not possible.

@akien-mga akien-mga merged commit 47353fb into godotengine:master Jan 5, 2021
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the the_color_DID_NOT_change branch January 5, 2021 12:45
@akien-mga
Copy link
Member

Cherry-picked for 3.2.4.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jan 5, 2021
@Chaosus
Copy link
Member

Chaosus commented Jan 9, 2021

I think you've broke the visual shader color picker by doing this, visually it's indeed changed but if you switch the graph from vertex to fragment mode (for example) you will see that it's not true and the color still is white - so Color is not applied.
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants