Improve usability of the SpriteFramesEditor #80448

nonunknown · 2023-08-09T13:26:00Z

Currently when the user is adding a lot of animations in the SpriteFramesEditor he has to open the sprite everytime and set values all again:

PR-usability-1.mp4

with the changes of this PR, I tried to make it as more reliable as possible, the editor will "save" the settings and know the settings and last texture used in the process:

PR-usability-2.mp4

Some info:

If you close the editor, and open again, it will still know your last configurations
If you (HOLD SHIFT + Click) on Add frames from spritesheet it will open the Open a file to select a new texture

Bugsquad edit:

Closes: "Select Frames" window should remember previous settings godot-proposals#7021

editor/plugins/sprite_frames_editor_plugin.cpp

AThousandShips

Make sure to check that you actually applied the suggestions, pay attention to detail :)

editor/plugins/sprite_frames_editor_plugin.cpp

nonunknown · 2023-08-10T15:06:03Z

@AThousandShips ok, did the requested changes, saving only dominant parameter related to the spin box that was mofied!

Also I've removed the _save_data function, because now its saving everytime you change a value.

Also thank you very much for explaining me how the dominant_param works I've understood it well! Hope theres no bug left xD what a PR damn :D

AThousandShips · 2023-08-10T15:06:24Z

editor/plugins/sprite_frames_editor_plugin.cpp

+		dominant_param = int(EditorSettings::get_singleton()->get_project_metadata("sprite_frames", "dominant_param", -1));
+		if (dominant_param == PARAM_SIZE) {
+			split_sheet_size_x->set_value(split_data["dom_x"]);
+			split_sheet_size_y->set_value(split_data["dom_y"]);


Need to compute the other values here (i.e. count for x/y and vice versa), including the ranges for sep/offset I believe, otherwise it looks good, will take a deeper look when I have time!

To ensure correctness I'd also suggest using wildly different sized textures to see what happens when you swap over and catch nasty cases

sorry didnt understood, the sep/offset is below outside the if statement, you mean compute them before? the dominants?

also I have found a problem, if you close the project and open again, the dominant values are set, but the others, lets suppose dominant is hor/vert then size x/y is saved, and when open the project the size x/y is set_value but hor/vert is not calculated, didnt understood why!

See here:

godot/editor/plugins/sprite_frames_editor_plugin.cpp

Lines 446 to 460 in 7df3933

case PARAM_SIZE: {

const Size2i frame_size = _get_frame_size();

const Size2i offset_max = texture_size - frame_size;

split_sheet_offset_x->set_max(offset_max.x);

split_sheet_offset_y->set_max(offset_max.y);

const Size2i sep_max = size - frame_size * 2;

split_sheet_sep_x->set_max(sep_max.x);

split_sheet_sep_y->set_max(sep_max.y);

const Size2i separation = _get_separation();

const Size2i count = (size + separation) / (frame_size + separation);

split_sheet_h->set_value(count.x);

split_sheet_v->set_value(count.y);

godot/editor/plugins/sprite_frames_editor_plugin.cpp

Lines 463 to 477 in 7df3933

case PARAM_FRAME_COUNT: {

const Size2i count = _get_frame_count();

const Size2i offset_max = texture_size - count;

split_sheet_offset_x->set_max(offset_max.x);

split_sheet_offset_y->set_max(offset_max.y);

const Size2i gap_count = count - Size2i(1, 1);

split_sheet_sep_x->set_max(gap_count.x == 0 ? size.x : (size.x - count.x) / gap_count.x);

split_sheet_sep_y->set_max(gap_count.y == 0 ? size.y : (size.y - count.y) / gap_count.y);

const Size2i separation = _get_separation();

const Size2i frame_size = (size - separation * gap_count) / count;

split_sheet_size_x->set_value(frame_size.x);

split_sheet_size_y->set_value(frame_size.y);

and when open the project the size x/y is set_value but hor/vert is not calculated, didnt understood why!

The min/max of the sep/offset values are updated here and should be done so here as well IMO

and when open the project the size x/y is set_value but hor/vert is not calculated, didnt understood why!

This is exactly what I'm talking about, they need to be calculated, see the code I referenced for how it is done

AThousandShips · 2023-08-10T16:18:06Z

editor/plugins/sprite_frames_editor_plugin.cpp

@@ -458,6 +471,9 @@ void SpriteFramesEditor::_sheet_spin_changed(double p_value, int p_dominant_para
 			const Size2i count = (size + separation) / (frame_size + separation);
 			split_sheet_h->set_value(count.x);
 			split_sheet_v->set_value(count.y);
+
+			split_data["dom_x"] = int(split_sheet_h->get_value());
+			split_data["dom_y"] = int(split_sheet_v->get_value());


This is now reversed, when PARAM_SIZE you store the size, and calculate the other value, and vice versa

The split_sheet_x/y should be calculated from the texture size, and vice versa, when loading

KoBeWi · 2023-09-06T13:01:04Z

Is this intended that changing spritesheet will reset the values to default?

KoBeWi · 2023-09-06T13:06:49Z

editor/plugins/sprite_frames_editor_plugin.cpp

+		file_split_sheet->popup_file_dialog();
+	} else {
+		// If there's a path to a texture goto the editor directly!
+		String path = EditorSettings::get_singleton()->get_project_metadata("sprite_frames", "last_texture", "");


Why is last texture remembered globally? Is it so common to use a single texture between editor sessions?

KoBeWi · 2023-09-06T13:57:08Z

editor/plugins/sprite_frames_editor_plugin.cpp

 	if (updating_split_settings) {
 		return;
 	}
 	updating_split_settings = true;

 	if (p_dominant_param != PARAM_USE_CURRENT) {
 		dominant_param = p_dominant_param;
+		EditorSettings::get_singleton()->set_project_metadata("sprite_frames", "dominant_param", dominant_param);


Suggested change

EditorSettings::get_singleton()->set_project_metadata("sprite_frames", "dominant_param", dominant_param);

EditorSettings::get_singleton()->set_project_metadata("sprite_frames_editor", "dominant_param", dominant_param);

Makes its purpose more clear.
(remember to replace other references too)

KoBeWi · 2023-09-06T13:58:56Z

editor/plugins/sprite_frames_editor_plugin.cpp

 		} break;
 	}

+	// Save relevant data


Suggested change

// Save relevant data

// Save relevant data.

KoBeWi · 2023-09-06T14:02:18Z

editor/plugins/sprite_frames_editor_plugin.cpp

-	bool new_texture = texture != split_sheet_preview->get_texture();
+	split_data = EditorSettings::get_singleton()->get_project_metadata("sprite_frames", "split_data", Dictionary());
+	String last_texture = EditorSettings::get_singleton()->get_project_metadata("sprite_frames", "last_texture", "");
+	bool has_valid_data = (split_data != Dictionary() && last_texture != "");


Suggested change

bool has_valid_data = (split_data != Dictionary() && last_texture != "");

bool has_valid_data = (!split_data.is_empty() && !last_texture.is_empty());

KoBeWi · 2023-09-06T14:07:51Z

editor/plugins/sprite_frames_editor_plugin.cpp

+	split_sheet_offset_x->set_max(size.x);
+	split_sheet_offset_y->set_max(size.y);
+
+	updating_split_settings = true;


You can remove this flag if you use set_value_no_signal().

KoBeWi · 2023-09-06T14:08:46Z

editor/plugins/sprite_frames_editor_plugin.cpp

 	}

+	updating_split_settings = false;
+
+	// Reset zoom.


If it's done for a specific purpose, this comment would need to be clarified. Otherwise you can remove it, as it's obvious from the method name.

KoBeWi · 2023-09-06T14:13:30Z

editor/plugins/sprite_frames_editor_plugin.cpp

@@ -1896,7 +1942,7 @@ SpriteFramesEditor::SpriteFramesEditor() {
 	load->set_shortcut_context(frame_list);
 	load->set_shortcut(ED_SHORTCUT("sprite_frames/load_from_file", TTR("Add frame from file"), KeyModifierMask::CMD_OR_CTRL | Key::O));
 	load_sheet->set_shortcut_context(frame_list);
-	load_sheet->set_shortcut(ED_SHORTCUT("sprite_frames/load_from_sheet", TTR("Add frames from sprite sheet"), KeyModifierMask::CMD_OR_CTRL | KeyModifierMask::SHIFT | Key::O));
+	load_sheet->set_shortcut(ED_SHORTCUT("sprite_frames/load_from_sheet", TTR("Add frames from sprite sheet | (Shift + Click) to load new texture"), KeyModifierMask::CMD_OR_CTRL | KeyModifierMask::SHIFT | Key::O));


This can't be added like that, because it becomes a part of shortcut's name:

Instead you should use set_tooltip_text() for the note.

Suggested change

load_sheet->set_shortcut(ED_SHORTCUT("sprite_frames/load_from_sheet", TTR("Add frames from sprite sheet | (Shift + Click) to load new texture"), KeyModifierMask::CMD_OR_CTRL | KeyModifierMask::SHIFT | Key::O));

load_sheet->set_shortcut(ED_SHORTCUT("sprite_frames/load_from_sheet", TTR("Add frames from sprite sheet"), KeyModifierMask::CMD_OR_CTRL | KeyModifierMask::SHIFT | Key::O));

load_sheet->set_tooltip_text("Shift + Click: Load new texture.")

btw the way it is implemented, if a shortcut includes Shift (like the default one), the file dialog will be opened when you press it.

YuriSizov changed the title ~~SpriteFramesEditor usability improvement~~ Improve usability of the SpriteFramesEditor Aug 9, 2023

nonunknown force-pushed the frame-usability branch from ce01fe5 to 9c989da Compare August 9, 2023 13:28

YuriSizov added enhancement topic:editor topic:2d labels Aug 9, 2023

YuriSizov added this to the 4.x milestone Aug 9, 2023

AThousandShips reviewed Aug 9, 2023

View reviewed changes

nonunknown force-pushed the frame-usability branch from 9c989da to cb2de7b Compare August 9, 2023 13:42

AThousandShips reviewed Aug 9, 2023

View reviewed changes

editor/plugins/sprite_frames_editor_plugin.cpp Outdated Show resolved Hide resolved

AThousandShips reviewed Aug 9, 2023

View reviewed changes

editor/plugins/sprite_frames_editor_plugin.cpp Outdated Show resolved Hide resolved

editor/plugins/sprite_frames_editor_plugin.cpp Outdated Show resolved Hide resolved

akien-mga requested review from kleonc and KoBeWi August 9, 2023 13:46

AThousandShips reviewed Aug 9, 2023

View reviewed changes

editor/plugins/sprite_frames_editor_plugin.cpp Outdated Show resolved Hide resolved

nonunknown force-pushed the frame-usability branch 2 times, most recently from 038b3bb to 19a46ee Compare August 9, 2023 14:00

AThousandShips reviewed Aug 9, 2023

View reviewed changes

editor/plugins/sprite_frames_editor_plugin.cpp Outdated Show resolved Hide resolved

nonunknown force-pushed the frame-usability branch 3 times, most recently from 0ee6b62 to d78b48b Compare August 10, 2023 15:04

AThousandShips reviewed Aug 10, 2023

View reviewed changes

SpriteFramesEditor Usability Improvement

4719367

nonunknown force-pushed the frame-usability branch from d78b48b to 4719367 Compare August 10, 2023 16:07

AThousandShips reviewed Aug 10, 2023

View reviewed changes

KoBeWi reviewed Sep 6, 2023

View reviewed changes

LeulMulugeta mentioned this pull request Feb 16, 2024

Remember frames when selecting SpriteFrame frames #88413

Merged

AThousandShips mentioned this pull request Aug 11, 2024

Make the Sprite Frames Editor automatically "guess" the amount of rows and columns in a Sprite Sheet when loading it instead of defaulting to 4x4 godotengine/godot-proposals#10415

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve usability of the SpriteFramesEditor #80448

Improve usability of the SpriteFramesEditor #80448

nonunknown commented Aug 9, 2023 •

edited by AThousandShips

Loading

AThousandShips left a comment

nonunknown commented Aug 10, 2023

AThousandShips Aug 10, 2023 •

edited

Loading

nonunknown Aug 10, 2023

nonunknown Aug 10, 2023

AThousandShips Aug 10, 2023 •

edited

Loading

AThousandShips Aug 10, 2023 •

edited

Loading

KoBeWi commented Sep 6, 2023

KoBeWi Sep 6, 2023

KoBeWi Sep 6, 2023 •

edited

Loading

KoBeWi Sep 6, 2023

KoBeWi Sep 6, 2023

KoBeWi Sep 6, 2023

KoBeWi Sep 6, 2023

KoBeWi Sep 6, 2023

	case PARAM_SIZE: {
	const Size2i frame_size = _get_frame_size();

	const Size2i offset_max = texture_size - frame_size;
	split_sheet_offset_x->set_max(offset_max.x);
	split_sheet_offset_y->set_max(offset_max.y);

	const Size2i sep_max = size - frame_size * 2;
	split_sheet_sep_x->set_max(sep_max.x);
	split_sheet_sep_y->set_max(sep_max.y);

	const Size2i separation = _get_separation();
	const Size2i count = (size + separation) / (frame_size + separation);
	split_sheet_h->set_value(count.x);
	split_sheet_v->set_value(count.y);

	case PARAM_FRAME_COUNT: {
	const Size2i count = _get_frame_count();

	const Size2i offset_max = texture_size - count;
	split_sheet_offset_x->set_max(offset_max.x);
	split_sheet_offset_y->set_max(offset_max.y);

	const Size2i gap_count = count - Size2i(1, 1);
	split_sheet_sep_x->set_max(gap_count.x == 0 ? size.x : (size.x - count.x) / gap_count.x);
	split_sheet_sep_y->set_max(gap_count.y == 0 ? size.y : (size.y - count.y) / gap_count.y);

	const Size2i separation = _get_separation();
	const Size2i frame_size = (size - separation * gap_count) / count;
	split_sheet_size_x->set_value(frame_size.x);
	split_sheet_size_y->set_value(frame_size.y);

	EditorSettings::get_singleton()->set_project_metadata("sprite_frames", "dominant_param", dominant_param);
	EditorSettings::get_singleton()->set_project_metadata("sprite_frames_editor", "dominant_param", dominant_param);

	bool has_valid_data = (split_data != Dictionary() && last_texture != "");
	bool has_valid_data = (!split_data.is_empty() && !last_texture.is_empty());

	load_sheet->set_shortcut(ED_SHORTCUT("sprite_frames/load_from_sheet", TTR("Add frames from sprite sheet \| (Shift + Click) to load new texture"), KeyModifierMask::CMD_OR_CTRL \| KeyModifierMask::SHIFT \| Key::O));
	load_sheet->set_shortcut(ED_SHORTCUT("sprite_frames/load_from_sheet", TTR("Add frames from sprite sheet"), KeyModifierMask::CMD_OR_CTRL \| KeyModifierMask::SHIFT \| Key::O));
	load_sheet->set_tooltip_text("Shift + Click: Load new texture.")

Improve usability of the SpriteFramesEditor #80448

Are you sure you want to change the base?

Improve usability of the SpriteFramesEditor #80448

Conversation

nonunknown commented Aug 9, 2023 • edited by AThousandShips Loading

AThousandShips left a comment

Choose a reason for hiding this comment

nonunknown commented Aug 10, 2023

AThousandShips Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AThousandShips Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

AThousandShips Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

KoBeWi commented Sep 6, 2023

Choose a reason for hiding this comment

KoBeWi Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nonunknown commented Aug 9, 2023 •

edited by AThousandShips

Loading

AThousandShips Aug 10, 2023 •

edited

Loading

AThousandShips Aug 10, 2023 •

edited

Loading

AThousandShips Aug 10, 2023 •

edited

Loading

KoBeWi Sep 6, 2023 •

edited

Loading