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

[3.x] Refactor layer property editor grid #51040

Merged
merged 1 commit into from
Aug 2, 2021

Conversation

pouleyKetchoupp
Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp commented Jul 29, 2021

Implements proposal godotengine/godot-proposals#2770 on the 3.x branch (see #51039 for 4.0), based on different comments and suggestions.

List of changes:

  • Now able to display up to 32 layers for physics in 4x2 blocks (still 20 layers in 5x2 blocks for render because some layers are reserved)
  • Also show 32 layers for physics/navigation layer names in project settings
  • Adjustable grid size to fit available space in dock
  • Expansion icon to display more layers vertically
  • Layer numbers in cells to help with selection (cells are also slightly larger to fit the numbers)

For now I haven't implemented the label for layer names like in godotengine/godot-proposals#2770 (comment), because tooltips are already displaying them and it can wait for feedback before doing further improvements if needed.

Physics layers:
layer_grid_physics

Render layers:
layer_grid_render

Rescaling demo:
collision_layers_grid

- Now able to display up to 32 layers in physics (still 20 for render)
- Adjustable grid size to fit available space in dock
- Expansion icon to display more layers vertically
- Layer numbers in cells to help with selection
@pouleyKetchoupp
Copy link
Contributor Author

pouleyKetchoupp commented Jul 30, 2021

I've pushed text color changes based on #51039 (comment) (CC @Calinou).

In comparison to 4.0, I'm using dark_color_3 instead of font_hover_color for selected cells in order to get better contrast (especially on light themes), due to cells using less of the highlight color.

This is the result with the default theme:
layer_grid_physics_theme_2

And with a light theme:
layer_grid_physics_theme_2_light


For reference, that was the result on a light theme with the same settings as on master:
layer_grid_physics_theme_font_light

@akien-mga
Copy link
Member

Looks good to me.

I was just wondering if it could be problematic for UX that the numbers are not contiguous, e.g. when looking at the first row:

 1  2  3  4   9 10 11 12

One alternative could be to write top-to-bottom and then left-to-right like this:

 1  3  5  7   9 11 13 15
 2  4  6  8  10 12 14 16

But I don't know if that's better.

@YuriSizov
Copy link
Contributor

I was just wondering if it could be problematic for UX that the numbers are not contiguous, e.g. when looking at the first row:

I think that contiguous rows is how it is currently, but we still split it in chunks, so it is even more confusing IMO:

 1  2  3  4  5 < -- >  6  7  8  9 10
11 12 13 14 15 < -- > 16 17 18 19 20

So this is actually better for my taste.

@pouleyKetchoupp
Copy link
Contributor Author

Numbers are organized by blocks to keep things consistent when resizing and follow left-to-right reading in each block to make it intuitive in English. It could be inverted for RTL languages, but apart from that I don't see another approach that really works in term of UX.

@akien-mga akien-mga merged commit 7b97243 into godotengine:3.x Aug 2, 2021
@akien-mga
Copy link
Member

Thanks!

@nitaku
Copy link

nitaku commented Nov 7, 2021

Wouldn't it be better to use a zero-based numbering, to keep it consistent with bit numbers in set_collision_mask_bit()?

0 1 2 3
4 5 6 7 etc.

I started using Godot 3.4 today, and loved the feature of controlling all 32 layers, but lost half an hour right away because I was working with the wrong bit in code.

Maybe others could get confused as well.

(Sorry if I'm writing here, I didn't feel like opening a full bug report or proposal for such a small thing.)

@pouleyKetchoupp
Copy link
Contributor Author

@nitaku This issue is going to be solved in 4.0 with #51532, but rather than making the layers 0-based in the inspector (which is harder to read, and especially confusing to non-programmers) the script functions have been renamed and changed to use 1-based indices too.

@nitaku
Copy link

nitaku commented Nov 9, 2021

@pouleyKetchoupp Thanks, that solution will be even better!

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.

4 participants