-
Notifications
You must be signed in to change notification settings - Fork 244
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
spectator_hud: add widget spectator hud #2607
base: master
Are you sure you want to change the base?
Conversation
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.
First of all, this is an awesome widget; I use it all the time, and I've seen lots of others use it too :)
I did a skim through the code, and added some quick comments. I'll try to take a deeper look later.
Some more general notes:
- Performance is pretty bad, to the point that it's using 1.5+ orders of magnitude more time and memory that any other widget (as per the Widget Profiler widget). I think that in order to get this merged, that will have to be addressed. That said, I'm not sure if we have specific guidelines to aim for.
- The standard formatting for lua code in this repo is 4-width tabs, while this file uses spaces (See https://github.com/beyond-all-reason/Beyond-All-Reason/blob/master/.editorconfig)
- You have trailing whitespaces on a few lines. I recommend using something to automatically format your code on save (an IDE, probably)
luaui/Widgets/gui_spectator_hud.lua
Outdated
return | ||
end | ||
|
||
gl.PushMatrix() |
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 you don't need to push/pop the matrix, as you aren't modifying it, as far as I can tell.
Remove global variables topBarPosition and topBarShowButtons.
Outer function already contains variables teamID and unitID and therefore inner function don't need to respecify them.
Outer function already contains variables teamID and unitID and therefore inner function don't need to respecify them.
This reverts commit ff3c36a.
This reverts commit e4d2cbd.
Fixed in 099c29b
Fixed in c169135
Yeah, I have used lots of time and effort trying to chase performance problems. My competence in Lua and OpenGL ends here and would need help to improve further. |
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.
Overall, very nice. The code is exceptionally well organized and formatted which makes it very pleasant to read, great work there.
My only qualm about merging it is the performance is quite bad, easily tripling the performance cost of advplayerlist which is the current most expensive widget. I think the default update rate should be lowered at a minimum, and perhaps reactively reduce itself if sim speed slows down, or if the player is catching up in the simulation.
return mathfloor(num * mult + 0.5) / mult | ||
end | ||
|
||
local function formatResources(amount, short) |
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.
This can be replaced with the new formatSI
functionality from @salinecitrine
local function getOneStat(statKey, teamID) | ||
local result = 0 | ||
|
||
if statKey == "metalIncome" then |
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.
Not critical, but for safety from typos and to allow autocomplete, it would be good if these keys were stored in variables in a table. Like
keys = {
metalIncome = "metalIncome",
}
etc, which can then be accessed with keys.metalIncome
-- TODO: this is not working, find out why | ||
local mouseX, mouseY = spGetMouseState() | ||
if (mouseX > headerDimensions.left) and | ||
(mouseX < headerDimensions.right) and | ||
(mouseY > headerDimensions.bottom) and | ||
(mouseY < metricChangeBottom) then |
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.
What's not working here?
Mouse position checks like this can be simplified a fair bit by using a construct like Rect
from gridmenu https://github.com/beyond-all-reason/Beyond-All-Reason/blob/master/luaui/Widgets/gui_gridmenu.lua#L154
This can be used for nice concise checks just going header:contains(mouseX, mouseY)
. Again not a critical thing, and maybe not worth doing until I abstract that construct somewhere accessible by other widgets, but I haven't really gotten around to it and it may never happen with RMLUI around the corner.
Thank you @thehobojoe for taking the time and effort to review this PR. Much appreciated.
Yes, you are absolutely correct that there are some concerning performance issues with Spectator HUD. However, the whole story is a bit more complicated. Allow me to share my findings. My typical test setup is to load up a skirmish on Quicksilver with a few AI's on each side. In this scenario, there is not much going on for the first few minutes. Hence, the FPS is very high. For example, in this screenshot I have 171 FPS: Looking at the readings from Widget Profiler, Spectator HUD is quite a perpetrator. At 6% of CPU usage, Spectator HUD has the biggest reading whereas Top Bar at 2% comes in second: However, looking more carefully at the readings we notice that most CPU time is spent in Opening Widget Selector will drop FPS massively. In my case, from 171 FPS to less then 100 FPS: We see that Widget Selector is using nearly 70% of CPU usage and Spectator HUD has dropped to 3%. In the following screenshot Widget Selector is disabled and game is resumes (no longer paused). In this scenario, Top Bar starts using a lot more CPU. Spectator HUD sits at 5% with Top Bar matching at around 5% CPU usage: Indeed, in scenarios with less FPS Spectator HUD CPU usage drops because Here's a late-game catch-up (~4x simulation speed) screenshot: Spectator HUD CPU usage sits now at 2% matching that of AdvPlayerList. The biggest perpetrator now is Top Bar at over 12% CPU usage. Also noteworthy in this scenario is that the biggest CPU usage of Spectator HUD has now switched from A screenshot from the same late-game, but now without catch-up (1.0x simulation speed): Spectator HUD is now around 3%. As I admitted earlier, there are performance concerns with Spectator HUD. Only the fact that Widget Profiler shows nasty readings during early game is enough reason for me to be willing to fix the problems. Unfortunately, I have already spent an unreasonable amount of time and effort chasing Spectator HUD performance problems. Every recommended Lua performance optimization has been implemented. If the remaining problems are related to Lua or OpenGL, I cannot tell. My skills in both are lacking. If it was the case that by spending 10 more hours debugging, the problems would be fixed, I would be willing to invest that time. Sadly, without better tools or help from others, it would likely be 10 more hours wasted without real performance gains.
This is unlikely to result in any performance gains. The collection of statistics done in Instead, if it was possible to render the widget graphics into a buffer in But I know so little OpenGL I don't know if this is doable or if it's even the right thing to do. It might very well be OpenGL has some more sophisticated and less NIH solutions for this problem. Maybe it's DisplayLists, maybe the widget is using shaders wrong... So, how to progress in this area? If there's a tool that can create flame graphs for individual widgets that would be great. Then we could maybe get more accurate data as to where the problem or problems are. |
First, don't be down on yourself, this is a very nice widget, and optimization of this nature is very difficult. 🙂 Optimizing draw performance is very complicated, and I'd need to spend more time studying this widget to give any meaningful recommendations, but I suspect it's not the main problem. Accurate profiling is really difficult, to do it comprehensively you'd definitely need to use Tracy, but you can do quite a lot with just checking how long something takes to run with But most importantly, the default setting causing 15 updates per second is WAY too high. The information this widget displays does not need to update even remotely that quickly to give accurate and relevant information, even twice per second is probably too much. Sure, it can look cool to have the sliders moving around really reactively, but it doesn't actually explain anything useful and is not worth the performance cost. Hopefully with RMLUI the draw performance part can be improved a lot, but some of it may be fixable without that. |
Work done
Add a new widget Spectator HUD. This widget has been in development for some time and the relevant Discord there is here: https://discord.com/channels/549281623154229250/1182353569576407060
Test steps
Screenshots:
BEFORE:
AFTER: