search

FAForever / downlords-faf-client

Official client for Forged Alliance Forever
https://faforever.com
MIT License
194 stars 119 forks source link

Show division in replays when available #3042

Closed BlackYps closed 3 months ago

BlackYps commented 9 months ago

WIP grafik

codecov[bot] commented 9 months ago

Codecov Report

Attention: Patch coverage is 67.16418% with 44 lines in your changes are missing coverage. Please review.

Project coverage is 58.84%. Comparing base (618ba92) to head (fb2576a).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #3042 +/- ## ============================================= + Coverage 58.72% 58.84% +0.12% - Complexity 3956 3982 +26 ============================================= Files 573 576 +3 Lines 19164 19284 +120 Branches 1014 1018 +4 ============================================= + Hits 11254 11348 +94 - Misses 7421 7442 +21 - Partials 489 494 +5 ``` | [Files](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3042?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever) | Coverage Δ | | |---|---|---| | [.../java/com/faforever/client/api/FafApiAccessor.java](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3042?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever#diff-c3JjL21haW4vamF2YS9jb20vZmFmb3JldmVyL2NsaWVudC9hcGkvRmFmQXBpQWNjZXNzb3IuamF2YQ==) | `90.18% <100.00%> (+0.06%)` | :arrow_up: | | [...a/com/faforever/client/domain/api/GameOutcome.java](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3042?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever#diff-c3JjL21haW4vamF2YS9jb20vZmFmb3JldmVyL2NsaWVudC9kb21haW4vYXBpL0dhbWVPdXRjb21lLmphdmE=) | `100.00% <100.00%> (ø)` | | | [...m/faforever/client/domain/api/GamePlayerStats.java](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3042?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever#diff-c3JjL21haW4vamF2YS9jb20vZmFmb3JldmVyL2NsaWVudC9kb21haW4vYXBpL0dhbWVQbGF5ZXJTdGF0cy5qYXZh) | `66.66% <ø> (ø)` | | | [...aforever/client/domain/api/LeagueScoreJournal.java](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3042?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever#diff-c3JjL21haW4vamF2YS9jb20vZmFmb3JldmVyL2NsaWVudC9kb21haW4vYXBpL0xlYWd1ZVNjb3JlSm91cm5hbC5qYXZh) | `100.00% <100.00%> (ø)` | | | [.../java/com/faforever/client/replay/DisplayType.java](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3042?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever#diff-c3JjL21haW4vamF2YS9jb20vZmFmb3JldmVyL2NsaWVudC9yZXBsYXkvRGlzcGxheVR5cGUuamF2YQ==) | `100.00% <100.00%> (ø)` | | | [...a/com/faforever/client/mapstruct/ReplayMapper.java](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3042?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever#diff-c3JjL21haW4vamF2YS9jb20vZmFmb3JldmVyL2NsaWVudC9tYXBzdHJ1Y3QvUmVwbGF5TWFwcGVyLmphdmE=) | `52.17% <0.00%> (ø)` | | | [...ava/com/faforever/client/replay/ReplayService.java](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3042?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever#diff-c3JjL21haW4vamF2YS9jb20vZmFmb3JldmVyL2NsaWVudC9yZXBsYXkvUmVwbGF5U2VydmljZS5qYXZh) | `77.92% <0.00%> (-1.80%)` | :arrow_down: | | [...om/faforever/client/game/PlayerCardController.java](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3042?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever#diff-c3JjL21haW4vamF2YS9jb20vZmFmb3JldmVyL2NsaWVudC9nYW1lL1BsYXllckNhcmRDb250cm9sbGVyLmphdmE=) | `63.12% <76.00%> (+7.28%)` | :arrow_up: | | [...aforever/client/replay/ReplayDetailController.java](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3042?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever#diff-c3JjL21haW4vamF2YS9jb20vZmFmb3JldmVyL2NsaWVudC9yZXBsYXkvUmVwbGF5RGV0YWlsQ29udHJvbGxlci5qYXZh) | `77.51% <82.50%> (+3.27%)` | :arrow_up: | | [.../com/faforever/client/game/TeamCardController.java](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3042?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever#diff-c3JjL21haW4vamF2YS9jb20vZmFmb3JldmVyL2NsaWVudC9nYW1lL1RlYW1DYXJkQ29udHJvbGxlci5qYXZh) | `64.65% <52.00%> (-5.94%)` | :arrow_down: | ... and [2 files with indirect coverage changes](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3042/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever) ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3042?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3042?dropdown=coverage&src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever). Last update [618ba92...fb2576a](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3042?dropdown=coverage&src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever).
BlackYps commented 9 months ago

The core functionality is there, but it is still a bit rough around the edges. There is a slight delay because we have to fetch the division info from the api, so at the moment it behaves a bit ugly visually. I appreciate input how this could be solved better. Similarly the team title still displays (0) because I didn't manage to properly set the teamrating to null to trigger the removal of the rating display alltogether. Edit: Fixed, but I don't know if the way I did it is good style

Sheikah45 commented 4 months ago

@BlackYps were you ever going to get back to this?

There have also been a few changes to the underlying objects as an FYI.

BlackYps commented 4 months ago

I don't plan to abandon this, on the other hand I am really not motivated to do the refactoring that is required here. So I guess the honest answer is, I really don't know...

Sheikah45 commented 4 months ago

Then I might just close this in a week and it can be reopened when you revisit it.

BlackYps commented 4 months ago

I'm picking this up again

BlackYps commented 4 months ago

The population of the values of the controllers and the displaying of these values are now nicely decoupled. There is one issue though. In a matchmaker game the people that don't have a division still display the rating and the team rating is displayed as well. The problem is that the player cards without a division don't know about the presence of divisions in different player cards. We need some way to check if we found any league stats for the replay (the api call) and then disable rating display for the whole thing somehow

Sheikah45 commented 4 months ago

For displaying the whole thing you can just have a display type enum that has a property in the player card. Then you can set that property appropriately to display either rating or league. Could even just bind it to some property in the parent.

BlackYps commented 4 months ago

I guess a boolean would even be enough, something like gameHasLeagueScore. I don't have to differentiate multiple states, just this binary decision

Sheikah45 commented 4 months ago

Yeah I was just thinking of an enum to make it clearer what it signifies. Since hasLeagueScore doesn't imply as much that ratings won't be shown.

Where as something like display type would make that clearer.

BlackYps commented 4 months ago

yeah, I can see that.

Sheikah45 commented 4 months ago

Was testing this out and one thing that stuck out as a little bit odd to me. Why do we require pressing the show game result to show the game outcome on the team card. Maybe we should just display it always on load?

BlackYps commented 4 months ago

I think this kind as introduced to make it possible to watch a game without being spoiled on which side wins

Sheikah45 commented 4 months ago

Sure but now you can also play directly from the search result page

BlackYps commented 4 months ago

Hmm, that's true. Maybe we should discuss this with more users to better understand how they use this button at the moment. It's definitely an interesting suggestion, but imo it is unrelated to this PR

Sheikah45 commented 4 months ago

What is the status of this?

BlackYps commented 4 months ago

From my pov it is ready

BlackYps commented 4 months ago

Can I do something about that race condition?

Sheikah45 commented 4 months ago

You could make sure that you don't try to load the league data until after the team containers are populated

BlackYps commented 4 months ago

I think I first need to understand the race condition a little better. As I see it we run the risk that the league info is applied to the old teamcardcontrollers, so they get cleared right after and new controllers for the new replay get added in populateTeamsContainer if this happens after the application of the league info. Is this the correct one, or did you have a different race condition in mind? And why is createTeamCardControllers called asyncronously? I do not immediately see any operation in that method that would make this necessary.

Sheikah45 commented 4 months ago

They are created asynchronously so they don't block the application thread and cause the ui to freeze. And to cure the race condition you would just load the league info after create team controllers is called or make it apart of create team controllers

RoraRaven commented 2 months ago

Showing division is nice, but was it necessary to remove the rating change? It means you need to check your profile after every match and keep a spreadsheet if you want to track changes rather than it being plainly displayed in the replays.

BlackYps commented 2 months ago

Yes, the divisions are supposed to replace the rating. The rating system is considered to be part of the backend, it's not necessary to show it to the user when the division system exists

RoraRaven commented 2 months ago

Are we going to get transparency in how divisions are calculated? Currently divisions are more or less meaningless, every other system (ingame player lists, lobby entry requirements, matchmaking, game quality rating) works on rating.

Divisions are really large and imprecise, you could win 15 games in a row and see no change. Even systems that obfuscate Elo / MMR (eg LoL) show points to measure how far up or down you are inside a division.

BlackYps commented 2 months ago

What exactly is the problem you are having? I'm trying to und erstand it so I can hopefully address it in my answer. And did you have a look at the leaderboard tab? Your comment gives me the impression that you didn't, because you can see points there and it's not possible to win 15 games in a row without changing divisions, because each division is only 10 points big.

RoraRaven commented 2 months ago

I wasn't aware about the Leaderboards or Score system at all. I was under the impression that division was based on rating (e.g. Gold V being 800 - 900).

This does answer my questions.

I still don't understand why you would hide information from the player, even if you consider rating to be back-end, but that's your prerogative.