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 colored margin in Inspector for arrays and dictionaries #6987

Closed
ajreckof opened this issue May 31, 2023 · 3 comments · Fixed by godotengine/godot#75482
Closed

Add colored margin in Inspector for arrays and dictionaries #6987

ajreckof opened this issue May 31, 2023 · 3 comments · Fixed by godotengine/godot#75482

Comments

@ajreckof
Copy link
Member

Describe the project you are working on

anything

Describe the problem or limitation you are having in your project

arrays and dictionaries have a problem with readability in the Inspector

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Add a colored border for arrays and dictionaries as it was done for Resources in godotengine/godot#45907.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams


An implementation of the proposal is available here : godotengine/godot#75482

If this enhancement will not be used often, can it be worked around with a few lines of script?

it will be used often and can't be worked around ith a few lines of code.

Is there a reason why this should be core and not an add-on in the asset library?

No it would need a plugin rebuilding the full editor which would be a huge work and with poorer performance

@Cammymoop
Copy link

The only objection I can think of is what @groud mentioned in the PR godotengine/godot#75482 (comment)

I am not 100% against the idea, but I kind of find useful that only sub-resources are highlighted. It is kind of making clear which parts of the properties might be things that are not part of the current scene. Thus adding highlighting to dictionaries is a bit confusing to me, and adds a lot of noise.'d think

I'd rather have bit more feedback on that solution before merging it, and that it might require a dedicated proposal to be sure it has enough baking.

But that being said, I don't have a better solution for now to solve the readability issue, so it might be an acceptable solution for now.

I think the idea is that the sub-inspector colored borders serves 2 purposes:

  1. Showing nesting/grouping
  2. Indicating the properties may be external to the current scene.

I think 1 is much more important and applies just as much to arrays and dictionaries, the readability for them when nested is even worse than it originally was for sub-resources. 2 is nice to have but not at the expense of 1 in cases where the hierarchy isn't entirely resources. Also 2 isn't entirely gone with this, it's just weaker.

I'm in support of adding this as is.

@ajreckof
Copy link
Member Author

I just realised that all resources are not external and as such as is the functionallity does not even properly indicate that the properties are external if it is something wante dI could had a parameter that would have two possibilities either color on variant type (coloring Dictionaries Array Resource and when they will be implemented struct) or color based on externality (color based on wether resource is internal or external) I could even add a last one for people wanting the functionality as it was (only resource colored)
This would be a bit more work but shouldn't be hard at all. I would really like to have your point of view on this idea @groud.

@groud
Copy link
Member

groud commented Feb 13, 2024

This would be a bit more work but shouldn't be hard at all. I would really like to have your point of view on this idea @groud.

Well, I still think it look overwhelming when everything gets colored, but I think it would not hurt to let users choose what they prefer. So as a settings, I think it is acceptable.

@akien-mga akien-mga added this to the 4.3 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants