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

Color Pickers Respect Default Editor Settings #62581

Merged
merged 1 commit into from
Jul 22, 2022

Conversation

Guh-Feng
Copy link
Contributor

@Guh-Feng Guh-Feng commented Jul 1, 2022

Updated editor_node with set_color_picker function called by color pickers throughout Godot to respect the editor's settings.
Another attempt to fix #57154.
Implemented changes noted in #57402 and rewrote certain color pickers to match the function call and keep everything consistent.

@Guh-Feng Guh-Feng requested review from a team as code owners July 1, 2022 03:58
@KoBeWi KoBeWi added this to the 4.0 milestone Jul 1, 2022
editor/editor_node.cpp Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented Jul 1, 2022

Looks good overall, but you shouldn't change ColorPicker nodes to ColorPickerButtons as they won't work correctly.

Instead you can manually setup the picker, e.g. here

case EDIT_PICK_COLOR: {
color_panel->popup();
} break;

here
if (show_above) {
popup->set_position(get_screen_position() - Vector2(0, minsize.y));
} else {
popup->set_position(get_screen_position() + Vector2(0, get_size().y));
}
popup->popup();

etc.
(you can also connect to the custom popups there)

scene/gui/gradient_edit.h Outdated Show resolved Hide resolved
@Guh-Feng
Copy link
Contributor Author

Guh-Feng commented Jul 4, 2022

Made changes from the recommendations. Things to note that I forgot to mention initially, I looked into fire-forge's suggestion for making everything use the same color picker. It seems just a little out of the scope of what I can do but I might consider working on it in the future. I also had a different perspective in consolidating the script used to sync the color picker settings into a single method. It might be a bandage solution but it could be an easier way to achieve the same goal. I'd like some feedback on the idea.

Also, I reverted the ColorPicker variables and adjusted the connections appropriately but kept them in the original locations to avoid calling it repeatedly.

editor/property_editor.cpp Outdated Show resolved Hide resolved
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Looks good now. You need to squash the commits.

@Guh-Feng Guh-Feng force-pushed the Color-Picker-Update branch 2 times, most recently from 828170b to 1ba6e50 Compare July 4, 2022 04:28
@KoBeWi
Copy link
Member

KoBeWi commented Jul 4, 2022

Ah now I noticed that GradientEdit isn't editor class, so it can't include editor-related code. You need to add a public method that returns the popup and then setup it in gradient editor plugin.

Also style checks failed, so you need to run clang-format.

@Guh-Feng Guh-Feng force-pushed the Color-Picker-Update branch from 1ba6e50 to 028cc31 Compare July 7, 2022 04:34
@akien-mga
Copy link
Member

Needs a rebase/rework after #62075.

@@ -85,6 +85,7 @@ void GradientEditor::reverse_gradient() {
}

GradientEditor::GradientEditor() {
GradientEdit::get_popup()->connect("about_to_popup", callable_mp(EditorNode::get_singleton(), &EditorNode::setup_color_picker), varray(GradientEdit::get_picker()));
Copy link
Member

@KoBeWi KoBeWi Jul 8, 2022

Choose a reason for hiding this comment

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

Suggested change
GradientEdit::get_popup()->connect("about_to_popup", callable_mp(EditorNode::get_singleton(), &EditorNode::setup_color_picker), varray(GradientEdit::get_picker()));
get_popup()->connect("about_to_popup", callable_mp(EditorNode::get_singleton(), &EditorNode::setup_color_picker), varray(GradientEdit::get_picker()));

Not necessary.

@KoBeWi
Copy link
Member

KoBeWi commented Jul 8, 2022

The rebase should be easy, just initialization code has changed from

int default_color_mode = EDITOR_GET("interface/inspector/default_color_picker_mode");
if (default_color_mode == 1) {
picker->get_picker()->set_hsv_mode(true);
} else if (default_color_mode == 2) {
picker->get_picker()->set_raw_mode(true);
}
int picker_shape = EDITOR_GET("interface/inspector/default_color_picker_shape");
picker->get_picker()->set_picker_shape((ColorPicker::PickerShapeType)picker_shape);

to
int default_color_mode = EDITOR_GET("interface/inspector/default_color_picker_mode");
picker->get_picker()->set_color_mode((ColorPicker::ColorModeType)default_color_mode);
int picker_shape = EDITOR_GET("interface/inspector/default_color_picker_shape");
picker->get_picker()->set_picker_shape((ColorPicker::PickerShapeType)picker_shape);

@Guh-Feng
Copy link
Contributor Author

The rebase should be easy, just initialization code has changed from...

Do I add back the _picker_created function with these suggested changes?

@KoBeWi
Copy link
Member

KoBeWi commented Jul 19, 2022

You need to update your setup_color_picker() function.

@Guh-Feng Guh-Feng requested review from a team as code owners July 19, 2022 23:51
@aaronfranke aaronfranke removed request for a team July 19, 2022 23:53
@Guh-Feng Guh-Feng force-pushed the Color-Picker-Update branch from e6209fe to 7a9faf3 Compare July 20, 2022 02:25
@Guh-Feng
Copy link
Contributor Author

Sorry that it got a little messy, but I cleaned everything up and I think all the changes appropriately accommodate the rebase.

@Guh-Feng Guh-Feng force-pushed the Color-Picker-Update branch 3 times, most recently from 11444ad to 4d72a78 Compare July 21, 2022 20:56
editor/editor_node.cpp Outdated Show resolved Hide resolved
Updated editor_node with function that sets up color pickers throughout Godot to respect editor's settings.
@Guh-Feng Guh-Feng force-pushed the Color-Picker-Update branch from 4d72a78 to 1b8652e Compare July 21, 2022 22:11
@akien-mga akien-mga merged commit e5df1e6 into godotengine:master Jul 22, 2022
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

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