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

Added a call to _shape() in the Label::_notification() to fix vertical space not being updated in one lined labels with word wrapping. #76058

Closed
wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Apr 14, 2023

This pull request is in reference to the issue #75402 .

I'm not sure if this the right way to fix it, I've been running against the wall for three weeks now, so I wanted to get some eyes in the issue and my findings, my process is explained in the issue comments as I've discovered things and how the code works.

Production edit: Fixes #75402

…l space not being updated in one lined labels with word wrapping.
@ghost ghost self-requested a review as a code owner April 14, 2023 12:28
@@ -609,6 +609,7 @@ void Label::_notification(int p_what) {

case NOTIFICATION_RESIZED: {
lines_dirty = true;
_shape();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should be queue_redraw(); instead, processing text should not be necessary until control is displayed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

queue_redraw() doesn't fix it! I tried before calling _shape()

@YuriSizov YuriSizov added this to the 4.1 milestone Apr 14, 2023
@RandomShaper
Copy link
Member

It's very hard to assess what the proper fix is. I've been not very long ago trapped in the complexities of the layout algorithm of the whole GUI and its relationship with labels, re-layout cascades, etc. At this point the only thing I can say is that unless this patch causes any major downside (e.g., prohibitely expensive additional re-layout passes), I'd accept it.

@bruvzg, I'm not sure if you told that this whole area may need an overhaul to make it more predictable?

@ghost
Copy link
Author

ghost commented Apr 15, 2023

Hi! I went ahead and tested some other Label issues similar to mine to see if the patch fixed them, and this patch also fixes issues: #74052 #75087 (With this one I was able to reproduce the issue when not calling _shape() there first try, but when having the call in the resize notification, it made me unable to reproduce it, so I assume it fixed it, but maybe I just got really lucky the ~20 times I tried reproducing it) and I also tested #76000 , but that one seems to be more related to HBoxContainer sizing it's content boxes to 0, since when you set the Label to Expand horizontally, it works fine.

@ghost
Copy link
Author

ghost commented Apr 18, 2023

I tried using the Godot profiler to check if the patch introduced a lot of processing time, however, I can't seem to be able to start the profiler before starting the game, so I can't catch the draw calls, so I went ahead with something a little bit more rudimentary and used the method propossed by the documentation about "The Profiler", to calculate how long it takes to instantiate and add 1000 labels 11 times to a scene via this script:

ProfilerScript

I'll first share the times I got without the call to _shape(). Please note that there are some instances that took a time spike, but that's out of my control and probably not Godot's fault, that's the reason I repeated the experiment 11 times, per each attempt for 3 attempts, giving us a total of 33 times for without _shape() and 33 times for with _shape().
Also note that even though the text says "label scenes", it means the whole scene from my minimal reproduction project, it's not loading only one label.

Without the _shape() call:

AddingLabelsWithoutShape
AddingLabelsWithoutShape2
AddingLabelsWithoutShape3

Ignoring the spikes mentioned before, it seems to be around low to mid 0.6 seconds. Not sure what happened in the first batch of 11, seems to be higher than the norm.

Now with the _shape() call:

AddingLabelsWithShape
AddingLabelsWithShape2
AddingLabelsWithShape3

With this, the time seems to be hovering around mid to high 0.6 seconds.

Which would mean that the _shape() call is adding aroun 0.05 seconds per 1000 (3000 actually, since the scene has 3 labels in it) labels maybe?

That being said, this is all assuming that I'm not mistaken thinking that instantiating, adding a scene as a child, and then waiting for the next process_frame runs all the relevant code.
And also, it's ignoring the variance that comes from how the operating system works and the interruptions and what not. So I don't know how conclusive this is.

Cheers.

@YuriSizov
Copy link
Contributor

Thanks for your contribution. Seems like this was superseded by a couple other PRs, mainly by #78009. Sorry for no communication for a long time, this is a hard issue to debug and validate. Hope you'll find something else to fix :)

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.

One-line Labels with word wrapping add a lot of vertical space when including white spaces
3 participants