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

Fix performance for godot's interface #19871

Merged

Conversation

guilhermefelipecgs
Copy link
Contributor

@guilhermefelipecgs guilhermefelipecgs commented Jul 1, 2018

Make godot performance great again :)

Fix #19864, Fix #12391.

This fix isn't only for TileMap, it's for all control nodes. All interface now is very smooth when doing resizing.

This was a regression from changes that I made here #19334.

@@ -942,6 +942,7 @@ void ItemList::_notification(int p_what) {
}
}

minimum_size_changed();
Copy link
Contributor Author

@guilhermefelipecgs guilhermefelipecgs Jul 1, 2018

Choose a reason for hiding this comment

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

FYI, I'm adding this because ItemList don't update minimum size when adding/removing items and this will prevent a regression in the about dialog.

@@ -160,9 +160,16 @@ void Control::_update_minimum_size_cache() {
Size2 minsize = get_minimum_size();
minsize.x = MAX(minsize.x, data.custom_minimum_size.x);
minsize.y = MAX(minsize.y, data.custom_minimum_size.y);

bool size_changed;
Copy link
Member

Choose a reason for hiding this comment

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

Please initialize to false, don't rely on compilers to maybe mess it up ;)

@akien-mga akien-mga added this to the 3.1 milestone Jul 1, 2018
@akien-mga
Copy link
Member

Fix #12391

Does it really fix this one? It was reported before 3.0 stable was released, so it can't be caused by #19334.

@guilhermefelipecgs
Copy link
Contributor Author

guilhermefelipecgs commented Jul 1, 2018

Actually this has been fixed here 005b69c by @reduz, "-Changed UI resizing code, gained huge amount of speed.", but this new improvements only exists on master and I unintentionally created a regression of his changes.

But to be sure, I've tested this issue on stable and master branch with my patch and I can clearly see a big difference. After this patch, I can't notice any delays. I can move the 2D viewport with the tilemap selected and the performance remains the same as moving the screen without selecting it.

@akien-mga akien-mga merged commit 82e03b2 into godotengine:master Jul 1, 2018
@guilhermefelipecgs guilhermefelipecgs deleted the fix_performance_ui branch July 1, 2018 13:16
@Zylann
Copy link
Contributor

Zylann commented Jul 1, 2018

I noticed the slowdown back then on a laptop, so if you have a powerful computer you may indeed not have seen the difference, so it needs to be tested on weaker computers (refer to the video I posted in #12391 to see the amplitude of the difference).

@guilhermefelipecgs
Copy link
Contributor Author

guilhermefelipecgs commented Jul 1, 2018

@Zylann I saw your video and I was able to reproduce the problem in all stable branchs, the lag is there and it's very noticible on my machine! and after this pr fixed for me. You can try in an older machine and if you think this not fixed your issue, feel free to reopen it.

@guilhermefelipecgs
Copy link
Contributor Author

guilhermefelipecgs commented Jul 1, 2018

I also did a simple "benchmark" test here, monitoring the cpu usage when working with TileMap and without it, and I can't see any noticeable difference, cpu usage is the same (with this pull request).

@Zylann
Copy link
Contributor

Zylann commented Jul 1, 2018

@guilhermefelipecgs are you referring to the newer lag or the one I had back in #12391 ? (it sounds like they are two different occurences of lag getting worse)

@guilhermefelipecgs
Copy link
Contributor Author

guilhermefelipecgs commented Jul 1, 2018

I'm referring of what you reported in #12391.

The "newer lag" is kind of the same as you had in #12391, but much worse and technically the cause for both issues is different, but now both cases was fixed.

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.

[Regression] TileMap response time with mouse it's very bad Tilemap edition has slower framerate than usual
3 participants