-
-
Notifications
You must be signed in to change notification settings - Fork 22.9k
Improve and reduce animation player/track editor width. #109361
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
base: master
Are you sure you want to change the base?
Conversation
ee0387c
to
e74d671
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely looks much better, I like it.
animation->set_accessibility_name(TTRC("Animation")); | ||
animation->set_custom_minimum_size(Size2(128 * EDSCALE, 0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it could be set to an even lower value, for example, 80.
UPD: I tried to remove it (in editor using EditorDebugger plugin) and it only affects when right_hb
moves to the next "row", so maybe there's no need to set a custom minimum size for it (or just a matter of taste, since it doesn't affect the minimum width of AnimationPlayerEditor
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added a minimum size for it to give more space for the animation name, previously it looked like this:
Yes, I understand this.
After the change it will move the right_hb to the next row instead of shrinking it more than that:
Note that the appearance will also depend on the font size (and, I suppose, also on theme spacing).
So in general it is just a matter of taste.
timeline_vbox->set_custom_minimum_size(Size2(0, 150) * EDSCALE); | ||
timeline_vbox->set_custom_minimum_size(Size2(0, 150 * EDSCALE)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is necessary, I haven't seen a unified variant for this in Godot codebase.
The same applies to other similar changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, this is codestyle change and does not affect anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand why this line with set_custom_minimum_size()
exists. I mean, changing Size2(0, 150) * EDSCALE
to Size2(0, 150 * EDSCALE)
is codestyle change and not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand why this line with
set_custom_minimum_size()
exists. I mean, changingSize2(0, 150) * EDSCALE
toSize2(0, 150 * EDSCALE)
is codestyle change and not necessary.
This is faster thank multiplying a float with both x and y for a vector, and is preferred in many other editor classes.
zoom->set_custom_minimum_size(Size2(200, 0) * EDSCALE); | ||
zoom->set_custom_minimum_size(Size2(128 * EDSCALE, 0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it could be set to an even lower value like 100 or 90.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I tried to change it in editor to 100 and below using EditorDebugger plugin.
At least for me, values below 80 look ugly, so I tested 100 and 90, and everything was still fine (saved a few free pixels and still didn't look ugly).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it supports expanding now, i don't see any issues of using 100px for minimum width.
e74d671
to
8a00994
Compare
Before:
AnimPL-old.mp4
After:
AnimPl-new.mp4