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

Make ClientTimesDisplay static #1147

Merged
merged 1 commit into from Nov 16, 2020
Merged

Make ClientTimesDisplay static #1147

merged 1 commit into from Nov 16, 2020

Conversation

lvaness
Copy link
Member

@lvaness lvaness commented Nov 13, 2020

Closes #1135 and also the discussion on #1061

Turns ClientTimesDisplay into a static panel and makes its update logic much less wasteful. The details are in the commit comment. The only thing I'm really unhappy with is that I couldn't name the init function Init() like with the other static panels because CBaseGameSystem, which is a superclass of the panel, already provides Init().

I also looked into whether it was possible to stop the request focus spam in the console without directly editing panel.cpp but found no good way to do it and declaring it a console-style panel doesn't make sense either.

Checklist

  • I have thoroughly tested all of the code I have modified/added/removed to ensure something else did not break
  • If there is a localization token change, I have updated the momentum_english_ref.res file with the changes, ran tokenizer.py to generate an up-to-date localization file, and have committed both the .res file changes and the new localization .txt file
  • If I introduced new h/cpp files, I have added them to the appropriate project's VPC file (server_momentum.vpc / client_momentum.vpc / etc)
  • If I have added or modified any visual assets (models, materials, panels, effects, etc), I have taken screenshots / videos of them and attached them to this PR directly (screenshots uploaded through github, videos uploaded to youtube and linked)
  • If I have modified any console command, console variable, or momentum entity, I have opened an issue (or a PR) for it in the Momentum Mod documentation repository
  • My commits are relatively small and scoped to the best of my ability
  • My branch has a clear history of changes that can be easy to follow when being reviewed commit-by-commit
  • My branch is functionally complete; the only changes to be done will be those potentially requested in code review

@lvaness
Copy link
Member Author

lvaness commented Nov 15, 2020

I'm almost ready for second review however during testing, I came across a weird bug that happens when resizing while having had a non-local leaderboard panel open:
https://streamable.com/a9rtjd

m_pCurrentLeaderboards is still correctly set to the global panel and that causes the place colors of the local panel not to show up. But I don't understand why it shows the local panel after res change with the buttons also somehow changing state accordingly.

Do you, by chance, have any idea what VGUI mechanism might cause this? From what I found, the only time the buttons' state changes is in OnCommand of ClientTimesDisplay and that doesn't get called on res change.

As a last resort, I could add a method to LeaderboardTimes to just set all its panels invisible and then set the current panel visible for this edge case or - like it is on Steam atm - just default to local times in this case.

I've pushed my WIP code in the meantime, after this bug is fixed, I'll clean it up.

@Gocnak
Copy link
Member

Gocnak commented Nov 15, 2020

I think if people are changing resolution the leaderboards panel should just close itself.

@lvaness
Copy link
Member Author

lvaness commented Nov 15, 2020

Ah I think I was a bit vague, what I meant by open is that the last panel that was opened was one of the global times panel. Of course you can't really change resolution while it's open.

@Gocnak
Copy link
Member

Gocnak commented Nov 15, 2020

Ah okay I understand now. Changing resolution reloads the res files and there might be something making an assumption. Previously the entire panel was destroyed. I think something to try is re-evaluating the buttons after changing resolution perhaps.

@lvaness
Copy link
Member Author

lvaness commented Nov 15, 2020

Of course it's the controls file, I completely forgot that visibility etc. is set there, thanks! W.r.t. reevaluating this, should the focus be to respect the res file and set visibility to whatever's specified there or keep what was last displayed?

@Gocnak
Copy link
Member

Gocnak commented Nov 15, 2020

Yeah I think visibility can be forced off ("0") in there and instead be evaluated in code.

@lvaness
Copy link
Member Author

lvaness commented Nov 16, 2020

OK so apart from the code review changes, to fix two bugs that I noticed, I

  • parented the leaderboards to clientdll so that you can't hold tab and then press escape to make the leaderboards shine through the main menu
  • moved the panel switch logic to its own function to fix control reload. This ended up a lot more boilerplate-y and verbose than I really wanted to but the old code was very optimized to its usecase and made lots of assumptions. This way, at least the panel would be future-proofed if e.g. a rivals leaderboard/steam group/whatever were to be added

This also includes some changes to its update logic:
As soon as it gets toggled open, it gets a full update that checks if it's time to get new online times etc. and as long as it's open, it will only get updated by game events.
The panel also doesn't get reset and recreated constantly anymore (e.g. on window size change), which was caused by it being a ViewportPanel.
@lvaness lvaness requested a review from Gocnak November 16, 2020 01:59
@Gocnak Gocnak merged commit f52f6a2 into momentum-mod:develop Nov 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Game crash when changing resolution on a map (Related to async replays)
3 participants