Improve SpriteFrameEditor frame addition ordering

[AThousandShips]

See the original proposal, implements drawing the index of selected frames and the modes suggested

Fixes: godot-proposals#5679

[kleonc]

Not sure if it's just me but to me the horizontal/vertical part of the orderings seem inverted, e.g. I'd expect "Left to Right, Top to Bottom" to go from left to right first in a column by column manner, and then top to bottom for each such column. But it currently goes row by row from top to bottom and then left to right for each such row (which I think "Top to Bottom, Left to Right" should be doing). 🤔

It might be not clear what the added dropdown is for. Especially when no frames are selected and thus changing selected ordering doesn't affect anything. Thus probably a Label before the dropdown is needed, just like for the other options.

Another thing is this dialog is getting way too wide because of that many options stacked in a single horizontal line. I suggest moving these into a HFlowContainer, grouped appropriately into HBoxContainers (so contents of each such HBoxContainers would stay together):

Overall LGTM. Code looks fine, seems to be working as intended. The usage of map/pair might make the code a little confusing but there's not much of it so it should be fine (few more comments for clarification should do the thing).

Also maybe the numbers/text should not be drawn when zoomed out enough. The text can be bigger than the frame at a certain zoom.

[AThousandShips]

Will take a look at some of the visual details and clean up some code

Would "Left to right, then top to bottom" be more clear? I personally read "Left to right, top to bottom" as "left to right in each row, then top to bottom", i.e. row major order, can implement the original suggestion for separate modes for horizontal and vertical and will look into this

Edit: now I can see the potential confusion between what order to visualise them as opposed to which order one would sort them, as in a lexicographical sort, will think of how to clarify it

[kleonc]

Would "Left to right, then top to bottom" be more clear?

I don't think it would change anything, "then" is kinda implicitly in there already. I mean with/without it I think the interpretation would be the same (whatever the interpretation is).

I personally read "Left to right, top to bottom" as "left to right in each row, then top to bottom", , i.e. row major order,

Yeah, I can see how it can be interpreted in a different way. That's why I've started my comment with "Not sure if it's just me (...)". Maybe I'm in the minority in here, I don't know. 🙂

can implement the original suggestion for separate modes for horizontal and vertical and will look into this

If you're referring to my original suggestion (up to 3 dropdowns) then I don't think it would be good. A single dropdown is way easier to use. I considered also submenus (another dropdown from the dropdown) but it would be harder to use and it's not like there are many options to choose from so there's no need to categorize like that. Maybe just adding some text into preceding separators could help? 🤔 (you can pass a text to OptionButton::add_separator)

[AThousandShips]

Oh didn't know about separators as text, that I will look into! Thank you

[AThousandShips]

Fixed the various style and code suggestions, added clarifications to the drop down hopefully that makes it clearer, fixed the width of the popup, and made the indices stop rendering when too large

@kleonc thank you for your feedback, anything else you can think of?

[kleonc]

anything else you can think of?

Frame numbers text needs a different color border or something like that, currently it can be not visible depending on the background color. You could probably look up how it's solved e.g. in the TileMap editor.

Regarding the HFlowContainer:

• VSeparators between HBoxConatiners don't look good in this setup, especially when they move between the rows:
  
• The rows are not separated visually anyhow, making the groups (HBoxConatiners) not easily distinguishable.

I think what could help is to remove all these VSeparators and move each group (HBoxContainer) to a PanelContainer (or something like that) so each such group would have its own darker background separating it from the surroundings (both horizontally and vertically).

Also maybe some fixed minimum size should be set on this dialog to disallow the options to be clamped like in the GIF above. Personally I do like I can shrink it as much as possible but maybe it's not "visually pleasant" or something like that. Not sure, I'm not a "GUI guy". 😄

[kleonc]

Approving functionality / code, LGTM.
And works fine as far as I've tested.

Leaving the visuals / presentation / wording etc. for the others. 🙂

[AThousandShips]

@kleonc would you mind pointing some relevant code owners to this, I don't know how to and who would be appropriate

[kleonc]

@kleonc would you mind pointing some relevant code owners to this, I don't know how to and who would be appropriate

cc @godotengine/2d-editor I guess.

You could for example ask for a review in the Contributors Chat (for this one in the editor channel?). And in general you can ask/discuss things in there.

[AThousandShips]

Thank you will check that!

[AThousandShips]

Testing an alternative layout:

[AThousandShips]

New version with toggleable settings panel will clean up and tweak some more tomorrow and push

Considering splitting the "Select/Clear All Frames" button into two for convenience

[hsandt]

Nice, I didn't test it yet but the screenshots seem to match what I was proposing.

I also get the ambiguity about Left-to-right, Top-to-bottom and reversely. This seems to be the official wording for it, but for users not used to it, I think the categories "By row" and "By column" should make the distinction. The user can play around with the settings the first time and observe the numbers to understand the differences. That said, to make the items clearer there are a few possibilities:

• we could be more explicit e.g. displaying "Left to right inside a row, Top to bottom rows" or "Left to right inside a row, rows navigated Top to bottom". But it will get longer so that's a compromise to do
• we could add an icon showing the navigation order (e.g. a Z-shaped arrow for Left to right, Top to bottom). I don't know how easy or hard it is to add an icon like this inside or besides text, and I wouldn't expect it for this PR, but it could be a future improvement.

[AThousandShips]

@hsandt thank you for you feedback, I'll look into icons!

[hsandt]

OK, I tested it, I think it's pretty clear even without icons since there are headers "By Row" and "By Column" so we cannot be confused on which one is iterated on first.

The selection is also refreshed when changing ordering mode, for immediate feedback, so it's very convenient to use.

The only question is whether users will think about revealing the pane on the right when hidden, but it's likely that they do when the slicing is not correct anyway. Plus, I think it was revealed by default, so users have a chance to see it once before they hide it the first time.

I also see it was approved by reviewer in the meantime, so it should be good to go!

[AThousandShips]

I'll go ahead and squash in a little bit if there's no questions about the interface

[YuriSizov]

A couple of nitpicks, but overall the code looks good. The feature looks great!

[AThousandShips]

Thank you! And I'm always happy to get good feedback

(Sorry my rebase went weird and it briefly included a previous commit updated, hence the random team)

[YuriSizov]

Thanks!

[AThousandShips]

Thank you!