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

Draw horizontal lines and labels in the editor performance monitors #39719

Merged

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Jun 20, 2020

This partially addresses godotengine/godot-proposals#1014.

Preview

image

@Calinou Calinou added cherrypick:3.x Considered for cherry-picking into a future 3.x release enhancement topic:editor usability labels Jun 20, 2020
@Calinou Calinou added this to the 4.0 milestone Jun 20, 2020
@Calinou Calinou force-pushed the editor-monitors-horizontal-lines branch from 76510bd to 2d31174 Compare June 20, 2020 23:57
@Calinou Calinou changed the title Draw horizontal lines and labels in the performance monitors Draw horizontal lines and labels in the editor performance monitors Jun 20, 2020
Comment on lines 755 to 756
constexpr int margin = 3;
constexpr int point_sep = 5;
Copy link
Member

Choose a reason for hiding this comment

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

Why are these constexpr and not just const?

Also, we should likely start a broader discussion on whether this kind of micro optimizations actually make a difference at all. If yes, we should use them consistently in new code. If not, we should probably not refactor existing code to use it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed them to use const. And yes, we should probably look at using constexpr instead of symbolic #defines in the future.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, does using const for local variables really make a performance difference? Or are compilers clever enough to optimize all this code already and us doing it manually is overkill? I think this should be benchmarked before continuing to make all local variables const.

Copy link
Member Author

@Calinou Calinou Jun 23, 2020

Choose a reason for hiding this comment

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

I think it's more about code readability than performance. I don't think const local variables make any difference for compiler optimization. However, with const, it's guaranteed the variable isn't being reassigned below. This can make the code significantly easier to read and less error-prone. I've seen lots of old code in Godot where variables are being reassigned for no good reason, and that makes it harder to reason about the code.

For this reason, using const should also make online code reviews a bit easier 🙂

The only case where I consider const to be overkill is in function parameters for primitive types (like the ISO C++ Core Guidelines say).

@Calinou Calinou force-pushed the editor-monitors-horizontal-lines branch from 2d31174 to a593786 Compare June 21, 2020 18:02
@akien-mga akien-mga merged commit 3f4e39e into godotengine:master Jun 22, 2020
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 3.2.2.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jun 24, 2020
@Calinou Calinou deleted the editor-monitors-horizontal-lines branch August 23, 2020 14:04
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.

2 participants