godotengine
Remove some control spacers from editor.
lot's of files have been changed, while I already have tested most of them here https://github.com/WhalesState/pixel-engine but this will need more testing to insure that nothing have changed, also the 3d plugins are untouched yet.
I also have added this to BoxContainer to prevent the spacer from empty fill which may make the CanvasItem's shader process the empty pixels all the time.
https://github.com/godotengine/godot/actions/runs/5845316423 you can compare this with any other build that was produced on github.
So the reason is that spacers are taxing on performance? This is a good find, but I think even now, most people will prefer spacers with slightly worse performance, over no spacers and slightly better performance. Spacers are important to improve the appearance of Godot and help with visual grepping. To me this looks more like a reason to look into optimizing spacers or StyleBoxLine instead.
(edit: I confused spacers and separators, but pretty much the point holds)
i already have made another comparison with the 4.2 dev editor build and this action editor build https://github.com/godotengine/godot/actions/runs/5845316423, and again this happens, you can explain it better than me.
So the reason is that spacers are taxing on performance? This is a good find, but I think even now, most people will prefer spacers with slightly worse performance, over no spacers and slightly better performance. Spacers are important to improve the appearance of Godot and help with visual grepping. To me this looks more like a reason to look into optimizing spacers or StyleBoxLine instead.
the control spacers do a job that the control size flags do, so simply instead of using a spacer you can use size flags like this.
Are those comparisons of two identical build setups, or is one the artifact and one the default published build?
Are those comparisons of two identical build setups, or is one the artifact and one the default published build?
one is the recent 4.2.dev official release and the other one from this github actions artifacts https://github.com/godotengine/godot/actions/runs/5845316423. do you still think i was wasting my time ?
I'm just running the same project.
And you are running windows?
yes, Windows 10 home latest release and everything is up to date.
They are not equivalent builds, compare with https://github.com/godotengine/godot/actions/runs/5830575651 for example
I ran your latest changes, and there's no reduction in RAM at all, in fact it is about 40 MB higher for the equivalent context, though I am not doing repeatable testing, but no massive reduction
Further, a number of controls now look much less clean and more stretched out, compare before and after for connect for example, which just shows the reason we do use spacers and why they can't just be ignored: Before:
After:
try to run the game with editor in both and this may happen.
the stretch is because BoxContainer is adding separation between every 2 control nodes. i don't remember the default value but maybe 4 pixels,
So in order to get the performance boost you have to have the engine running a game instance, then you don't think that's what is relevant there? Not the spacers? Probably caused by shared data between the two instances since they run the same data
In fact, does the process memory for the one process increase when you run the second instance?
I.e. does the one with 624 MB drop if you exit the one running from the 173 MB one
So in order to get the performance boost you have to have the engine running a game instance, then you don't think that's what is relevant there? Not the spacers? Probably caused by shared data between the two instances since they run the same data
In fact, does the process memory for the one process increase when you run the second instance?
I.e. does the one with 624 MB drop if you exit the one running from the 173 MB one
please if you have any concerns about comparison then you can do it better than me, I'm noob in c++ and comparison, all I know is how to use Control nodes efficiently and i didn't assume anything, this is an efficient way for UI designing in Godot, to not have extra Control nodes in the scene that may be useless. I'm just helping with some small performance boost, even if it was 1%, and a save of just 10 lines of code it will still be up to the Project Manager to merge it or not. Thank You ^^ !
I have tested it, and I see no difference, as I have been saying, so since I cannot replicate your performance situation
I do have concerns about the comparison, since I just can't replicate it, and I have questions as to how it is possible that you can get these results and I cannot
Try compare with running one copy of the editor at a time, not simultaneously
to not have extra Control nodes in the scene that may be useless.
They aren't useless, as you can see with the changes in the UI look
still be up to the Project Manager to merge it or not
There isn't any "project manager" that decides on merges, and I am providing feedback for this and showing the issues
Also you should be easier on the new contributors ^^ and to teach them instead of arguing with them to prove they are wrong, I know all of you have better understanding of the project structure and programming, and if I was wrong or have made a mistake you should correct it and not try to prove that I'm wrong or make fool of me! you aren't helping and have wasted so much of my time and made me don't want to open any other issues or work anymore on this pull request. but I will not take it personal, I'm doing this because I want Godot to be the best Engine ;) have a nice day!
There isn't any "project manager", and I am providing feedback for this and showing the issues
I am sorry that I have made you feel this way, but I have not tried to make a fool of anyone
I have pointed out how this performance just isn't possible, and I'm sorry if that makes you feel that way
wasted so much of my time
How have I wasted your time? I have given you feedback, do you mean that I have wasted your time by not accepting your suggestions?
I have provided clear and critical feedback, I'm not trying to "prove you wrong", except in asking that you show that your results are valid, how is me saying "I cannot confirm your claims" not "helping"?
The "Project Manager" isn't the one deciding what to merge or not
made me don't want to open any other issues or work anymore on this pull request.
I am sorry that this interaction has made you feel that way
But all I have done is say "I cannot confirm your results, and they are not possible without some other explanation", and pointed out what I consider a reduction in the UI quality
How would you like me to have approached this differently? I am genuinely asking
I can see that you came into this being very defensive from your first interaction with me on the issue, and it is understandable to be defensive about your work, and be sensitive to feedback or criticism, but it is fundamental to be receptive to that
No issues here ^^, better we focus on the main point rather than early concerns or comparisons since the pull request is a work in progress still and can be edited any time based on the feedback or the issues. I also remember you have helped me many times in other pull requests and I really appreciate it.
I mean, the comparisons are the point here, this is about performance improvements, right?
So to focus back:
- The UI has changed, and in a negative direction in my opinion, by the removal of spacers, please provide some before/after images of the dialogs and interfaces that have changed, to give a good idea of what to evaluate
- The performance results, positive or negative, need to be validated, and the possible performance losses due to this from unforeseen directions need to be considered
In my testing the performance results are negligible, and variable, so further testing of that is needed, others, on other platforms and OS versions would be appreciated (I have Win11 so could be Win10 specific)
Further, a number of controls now look much less clean and more stretched out, compare before and after for connect for example, which just shows the reason we do use spacers and why they can't just be ignored: Before:
After:
Yes I didn't realize this change, and I have been looking at many Dialog box styles and we can still do those ones without the Spacers
the one on the right looks much cleaner than all the others and the one in the bottom left is the same one used in windows and aseprite.
to make them more clean all buttons should have the same width like max(96, max_button_size).
I did some tests myself (also Windows 10)
Vanilla build 4714e9589, editor only:
Vanilla build, editor + game:
Your PR, editor only:
Your PR, editor + game:
Bonus test
#80573, editor only:
#80573, editor + game:
That said, "benchmarking" with Windows Task Manager is worthless and unreliable; I got different results every time I started the same build. I think someone more experienced should test it. CC @Calinou I'm not very optimistic about the results though. There is 0 impact on the CPU/GPU and the ~50MB saved RAM may as well be some measurement error.
You should use Resource Monitor to monitor Memory usage, NOT task manager.
Resource Monitor will Show you total Memory usage, not just "Private" memory usage, but also "Shared" memory usage. The True memory usage is Private+Shared memory usage, also known as Working Set memory.
Private Memory is Actively in Use, while Shared memory is cached and that may be freed for use for other programs (although with a small performance cost associated with it vs using actually Free memory)
As a summary, here's the cases that should define when to remove or keep the spacers.
Cases of remove:
-
Spacers that push elements to the end should be removed, since size flags
EXPAND | SHRINK_ENDcan be used instead. -
Spacers that only centers one element should be removed, since size flags
EXPAND | SHRINK_CENTERcan be used instead.
Excluding:
- Spacers that centers two or more controls, can't achieve the same result using size flags.
- Cases when the spacer is pushing nodes that can become hidden or visible on demand.
Conclusion:
- If the spacer is only used once, it's totally useless since we are adding a new
Controlnode instead of adjusting another node's size flags.
Don't add to archieve, I am just discarding the changes because the github source graph is not showing any commits.
This pull request is safely removing 28 Control nodes used as spacers, There are more spacers that still exists and most of them was added manually, not all of them can be removed, since we also should exclude cases like when the node pushed by spacers can be hid or shown on demand.
Yes, just a small cleanup, since spacers are usefull but we should not overuse them. I will do another check later to find other spacers that can be safely removed.