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.

Sign up for GitHub

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

Jump to bottom

feat(match2): properly attach participants to their playerid #4645

Merged
merged 24 commits into from
Sep 12, 2024
Merged

feat(match2): properly attach participants to their playerid #4645

merged 24 commits into from
Sep 12, 2024

Conversation

Rathoz
Copy link
Collaborator

@Rathoz Rathoz commented Aug 26, 2024 •
edited
Loading

Summary

Create a common's function to find the match2playerid of a player input. Use this function in wikis with refactored MGI's

How did you test this change?

Some testing by me, some by hjp

@Rathoz Rathoz requested a review from hjpalpha August 26, 2024 09:15
Base automatically changed from mgi-split to main August 26, 2024 10:18
@hjpalpha
Copy link
Collaborator

needs rebase

@Rathoz Rathoz force-pushed the attach-participant-properly branch from 95a50dc to f7d9cd6 Compare August 26, 2024 10:28
@Rathoz Rathoz marked this pull request as ready for review August 26, 2024 10:34
hjpalpha
hjpalpha reviewed Aug 26, 2024
View reviewed changes
Copy link
Collaborator

@hjpalpha hjpalpha left a comment

Choose a reason for hiding this comment

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

do we need any handling in case we do not find the player id?
(like in sc(2) custom i just append them to match2players)

LGTM modulo the above question

This was referenced Aug 26, 2024
Merged
Merged
Merged
@Rathoz Rathoz requested a review from hjpalpha August 30, 2024 08:43
hjpalpha
hjpalpha reviewed Aug 30, 2024
View reviewed changes
Copy link
Collaborator

@hjpalpha hjpalpha left a comment •
edited
Loading

Choose a reason for hiding this comment

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

this completely breaks sc(2) matches with team opponents

  • missing data
  • keys renamed
  • faction processing doesn't work as intended

@Rathoz Rathoz requested a review from hjpalpha September 2, 2024 14:04
hjpalpha
hjpalpha approved these changes Sep 3, 2024
View reviewed changes
Copy link
Collaborator

@hjpalpha hjpalpha left a comment

Choose a reason for hiding this comment

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

is the latest version tested?

i see 1 edge case that gets handled differently on sc(2): TBD player input in maps
currently it would add an tbd player in match2players for each TBD in maps
the new version only does so for the first map (or none at all if there already is a TBD player in match2players)

i think this should not cause any issues though and data wise might even be preferable

overall lgtm if tested

components/match2/commons/match_group_input_util.lua Show resolved Hide resolved
@hjpalpha hjpalpha mentioned this pull request Sep 3, 2024
Merged
@hjpalpha hjpalpha mentioned this pull request Sep 4, 2024
Merged
@Rathoz
Copy link
Collaborator Author

Rathoz commented Sep 10, 2024 •
edited
Loading

is the latest version tested?

i see 1 edge case that gets handled differently on sc(2): TBD player input in maps currently it would add an tbd player in match2players for each TBD in maps the new version only does so for the first map (or none at all if there already is a TBD player in match2players)

i think this should not cause any issues though and data wise might even be preferable

overall lgtm if tested

I've tested on dota2 match page, but that's it

@hjpalpha
Copy link
Collaborator

is the latest version tested?
i see 1 edge case that gets handled differently on sc(2): TBD player input in maps currently it would add an tbd player in match2players for each TBD in maps the new version only does so for the first map (or none at all if there already is a TBD player in match2players)
i think this should not cause any issues though and data wise might even be preferable
overall lgtm if tested

I've tested on dota2 match page, but that's it

can test on sc2 later today or tomorrow (probably tomorrow)

hjpalpha
hjpalpha approved these changes Sep 10, 2024
View reviewed changes
Copy link
Collaborator

@hjpalpha hjpalpha left a comment

Choose a reason for hiding this comment

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

tested on sc2

LGTM

hjpalpha
hjpalpha reviewed Sep 12, 2024
View reviewed changes
Copy link
Collaborator

@hjpalpha hjpalpha left a comment

Choose a reason for hiding this comment

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

did some more preview tests across the affected wikis
apart form some missing nil checks (see comments) no errors found so far

components/match2/commons/match_group_input_util.lua Show resolved Hide resolved
components/match2/wikis/brawlstars/match_group_input_custom.lua Show resolved Hide resolved
components/match2/wikis/valorant/match_group_input_custom.lua Show resolved Hide resolved
@Rathoz Rathoz merged commit a626f60 into main Sep 12, 2024
6 checks passed
@Rathoz Rathoz deleted the attach-participant-properly branch September 12, 2024 14:40
@Rathoz Rathoz restored the attach-participant-properly branch September 12, 2024 14:40
@Rathoz Rathoz deleted the attach-participant-properly branch September 12, 2024 14:40
hjpalpha added a commit that referenced this pull request Sep 13, 2024
@hjpalpha
#4645
a46879f
hjpalpha added a commit that referenced this pull request Sep 13, 2024
@hjpalpha
#4645
33daf93
Rathoz added a commit that referenced this pull request Sep 17, 2024
@hjpalpha @Rathoz
refactor(match2): warcraft MGI (#4631)
681362d 
* matchsummary

* a start

* add hero stuff

* not needed

* assume 4608 gets merged

* Update match_summary.lua

* fix some stuff

* fix player ext custom (same as on sc(2))

* linter: break line

* fix legacy wrapper for `skip` score input

* fix legacy wrapper

if an solo opponent has unset name but set faction/flag use TBD as name to display the flag/faction data

* legacy wrapper fixes

- race , flag, link params
- convert skip score to default loss as it seems more fitting according to usage ..

* kick unused require

* Update match_group_input_custom.lua

* the skip shit again

* skip fixing

* Update match_group_input_custom.lua

* Update components/match2/wikis/warcraft/match_group_input_custom.lua

* Update components/match2/wikis/warcraft/match_group_input_custom.lua

* as per review

* use commons

* assume maps in legacy matches are finished

* Update components/match2/wikis/warcraft/match_group_input_custom.lua

Co-authored-by: Rikard Blixt <[email protected]>

* fix map handling

* bold submatch scores on submatch draw

* #4650

* #4645

* kick unused import

* missing faction normalization

* do it in the correct file......

---------

Co-authored-by: Rikard Blixt <[email protected]>
Rathoz added a commit that referenced this pull request Sep 17, 2024
@hjpalpha @Rathoz
refactor(match2): stormgate MGI (#4651)
33af674 
* ms adjusts

* player ext adjusts

* input processing

* linter: line is too long

* as per review in warcraft

* from wc review that applies here but not there

* double space fix

* kick unused function

* fix map handling

* bold submatch scores on submatch draw

* #4650

* #4645

* missing faction normalization

* #4752

* Update components/match2/wikis/stormgate/match_group_input_custom.lua

Co-authored-by: Rikard Blixt <[email protected]>

* Update components/match2/wikis/stormgate/match_group_input_custom.lua

Co-authored-by: Rikard Blixt <[email protected]>

---------

Co-authored-by: Rikard Blixt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hjpalpha hjpalpha hjpalpha approved these changes

Assignees
No one assigned
Labels
c: match2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Rathoz @hjpalpha