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

Add multi-selection for SpriteFramesEditor #85494

Conversation

Hobitus
Copy link

@Hobitus Hobitus commented Nov 29, 2023

Enabled multi-selection in the sprite_frame_editor
This was proposal #1696

You can now use multi-selections with CTR to add or remove a frame. Shift to selection until selection.
With a multi-selection you can:

Copy and Paste multiple-frame
Since you can't put arrays of frame in the Editor clipboard, I've remove it and saved the data locally.

Move left or right selected frame
For example, if you have 1 and 3 selected and press move right, you will have 2,1,4,3.

Set frame duration to all selected frame.
Takes the first frame for the duration value.

Add empty frame, after or before the selection

Delete multiple frame at the same time

image

I didn't see a way to add test for this class. Anything I can do?

Bugsquad edit:

@Chaosus Chaosus added this to the 4.3 milestone Nov 29, 2023
@Chaosus Chaosus changed the title Proposal 1696 : Added multi-selection for sprite_frame_editor Added multi-selection for sprite_frame_editor Nov 29, 2023
@Chaosus
Copy link
Member

Chaosus commented Nov 29, 2023

Static check failed, you should apply the clang-format tool to format it properly. After making the change, don't forget to squash commits (see https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html#modifying-a-pull-request).

@Hobitus Hobitus force-pushed the Allow-multi-selection-of-frames-in-the-SpriteFrames-animation-editor branch from cc14c00 to 698a370 Compare November 29, 2023 21:24
@Hobitus
Copy link
Author

Hobitus commented Nov 29, 2023

I feel like the move left and move right command are over complicated. It moves all the selected animation (with gap in selection), which is great, but is it really useful?
For simplicity we could pick the first or last item in the multi-selection and move that one only.

@Hobitus
Copy link
Author

Hobitus commented Nov 29, 2023

How do I apply the clang-format tool? using scons?
I did the fix by hand. I hope I got them all.

@Chaosus
Copy link
Member

Chaosus commented Nov 30, 2023

@Hobitus Hobitus force-pushed the Allow-multi-selection-of-frames-in-the-SpriteFrames-animation-editor branch from 698a370 to ffd9a17 Compare November 30, 2023 12:29
@TokageItLab TokageItLab requested review from a team and dalexeev February 11, 2024 12:51
editor/plugins/sprite_frames_editor_plugin.cpp Outdated Show resolved Hide resolved
editor/plugins/sprite_frames_editor_plugin.h Outdated Show resolved Hide resolved
editor/plugins/sprite_frames_editor_plugin.cpp Outdated Show resolved Hide resolved
editor/plugins/sprite_frames_editor_plugin.h Outdated Show resolved Hide resolved
@Hobitus Hobitus force-pushed the Allow-multi-selection-of-frames-in-the-SpriteFrames-animation-editor branch from ffd9a17 to 32dae1d Compare February 13, 2024 02:43
@Hobitus Hobitus force-pushed the Allow-multi-selection-of-frames-in-the-SpriteFrames-animation-editor branch from 32dae1d to 077a4f7 Compare February 15, 2024 02:37
@AThousandShips AThousandShips changed the title Added multi-selection for sprite_frame_editor Add multi-selection for SpriteFramesEditor Feb 18, 2024
@KoBeWi
Copy link
Member

KoBeWi commented Feb 28, 2024

Deleting frames should also remove selection, otherwise it looks confusing.

godot.windows.editor.dev.x86_64_QWbMGUQzzJ.mp4

EDIT:
Same with pasting, although it's more expected to select the pasted frames.

EDIT2:
If you move multiple frames and undo, the selection will be wrong:

godot.windows.editor.dev.x86_64_zPOTQrfeUz.mp4

@Hobitus
Copy link
Author

Hobitus commented Feb 28, 2024

@KoBeWi

Deleting frames should also remove selection, otherwise it looks confusing.

From what I tested with the current build of godot (4.2.1). it works like what you see with my PR.
-Pasting new frame keeps the old selection.
-undoing doesn't change selection. It keeps what you currently have.
-Deleting frame keep the current selection ( 3 frames with the 1 selected, press delete) and its still the 1 that is selected.

You want me to change the current behavior?

@KoBeWi
Copy link
Member

KoBeWi commented Feb 29, 2024

Looks like the current behavior is not that great :/
You could leave it as is then, it can be improved in a later PR.

use multi-selection for copy/paste
move up or down
frame duration set while multi-selected
@Hobitus Hobitus force-pushed the Allow-multi-selection-of-frames-in-the-SpriteFrames-animation-editor branch from 077a4f7 to 8faba24 Compare March 1, 2024 01:01
@Hobitus
Copy link
Author

Hobitus commented Mar 1, 2024

I'll leave the selection behavior like it is now, but I agree with you it's not great.

@akien-mga akien-mga merged commit 8eb0852 into godotengine:master Mar 4, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@hsandt
Copy link
Contributor

hsandt commented Jun 23, 2024

Nice, I'll have to try this one!

While I agree that deleting selected entries should clear selection in general, I see a point, on single delete, in selecting the new sprite replacing the deleted sprite at the same slot (or the one before if deleting the last sprite), so user can chain delete if they want to. This would have to be combined with godotengine/godot-proposals#9387 so user keeps focus and can chain usage of the Delete key, or do some other operations like changing the Frame Duration multiplier (it really shines when doing many of these operations - like "merging" identical sprite frames together by removing all the duplicates and setting the Frame Duration multiplier).

Currently, clicking the bin button actually does that, so you can chain delete by clicking on the bin button multiple times. Now it's a technique I learned because batch selection was not implemented and it becomes much less useful now that we can delete multiple sprite frames at once. However it's worth considering the current behavior of single delete, whether with Delete key or bin button, to decide whether or not to maintain selection.

That said, in the case of multi-selection delete, I agree it's confusing. In fact, in an app like Ubuntu's Image Viewer / Eye of Gnome, when the Image Gallery is opened at the bottom, if you multi-select images and Delete them, the selection will fallback back to the image right after the last selected deleted entry (or before if last entry was deleted). So still a unique selection.

To sum up, we may want to preserve the single delete behavior of falling back to the next available entry, but when deleting multiple frames, it may be less confusing to fall back selection to just the next available entry after the last selected frame (whether "last" means "last selected" or "last in chronological order" would remain to be decided).

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.

Allow multi-selection of frames in the SpriteFrames animation editor
8 participants