feat(multiset): send sets to emulators #2857

wescopeland · 2024-11-17T01:20:43Z

Relies on RetroAchievements/rcheevos#384.

This PR modifies dorequest.php?r=patch to send resolved subsets to emulators to enable subset+core functionality.

How it works:

GetPatchData() has been removed. The tests are still intact and unchanged.
The "patch" case in dorequest.php has been modified to execute a new action: BuildClientPatchDataAction.
If this new action receives only a game ID (legacy client), the existing legacy response is returned.
If this new action receives a game hash (updated client), then ResolveAchievementSetsAction is executed.
A new field: "Sets" is added to the response. It is not added for legacy clients.

The shape of Sets is:

{
    "GameAchievementSetID": 1234,
    "CoreGameID": 1234,
    "Title": "Gold Medals",
    "Type": "bonus",
    "ImageIcon": "12345",
    "ImageIconURL": "foo",
    "Achievements": [ ... ],
    "Leaderboards": [ ... ]
}

In other words, a multiset patch response will look like:

{
    "Success": true,
    "PatchData": {
        "ID": 1,
        "Title": "Sonic the Hedgehog",
        "ImageIcon": "12345",
        "RichPresencePatch": "foo",
        "ConsoleID": 1,
        "ImageIconURL": "foo",
        "Achievements": [ ... ],
        "Leaderboards": [ ... ],
        "Sets": [ ... ]
    }
}

Tests for BuildClientPatchDataAction to verify its behavior have been included in this changeset.

…copeland/RAWeb into multiset-resolve-sets-action

wescopeland · 2024-11-24T21:04:48Z

This is now ready for review. I've merged in latest and resolved all conflicts from #2862.

wescopeland · 2024-11-24T21:11:05Z

app/Connect/Actions/BuildClientPatchDataAction.php

+
+// TODO more tests
+
+// OPEN QUESTION when given a GameHash, should the root Achievements & Leaderboards be returned as `null`?


This is a discussion point I'm not sure how to best handle. I also referenced this in a test:

RAWeb/tests/Feature/Connect/Actions/BuildClientPatchDataActionTest.php

Lines 402 to 407 in 75f2b68

// 🔴 JAMIRAS LOOK HERE

// "Sets" only contains $baseGame and $bonusGame2.

// However, PatchData still contains all the pre-multiset stuff, eg

// - "Title" is "Dragon Quest III [Subset - Bonus]"

// - "Achievements" has 2 published achievements

// Is this ok left as-is, or should this data be changed?

Given that the user is using a modern client and we send a list of sets to that client from r=patch, what should the root-level data be?

Do root-level Achievements and Leaderboards even matter?

Should root-level Title, ImageIcon, and ImageIconURL always point to the core game?

Should RichPresencePatch take on the RP script of the "subset game"?

For reference, here is a full data structure of the response in the test case above:

array:2 [ "Success" => true "PatchData" => array:9 [ "ID" => 2 "Title" => "Dragon Quest III [Subset - Bonus]" "ImageIcon" => "/Images/000011.png" "RichPresencePatch" => """ Display:\n Test """ "ConsoleID" => 1 "ImageIconURL" => "/media/Images/000011.png" "Achievements" => array:2 [ ... two achievements ... ] "Leaderboards" => [] "Sets" => array:2 [ 0 => array:8 [ "GameAchievementSetID" => 1 "CoreGameID" => 1 "Title" => null "Type" => "core" "ImageIcon" => "/Images/000011.png" "ImageIconURL" => "/media/Images/000011.png" "Achievements" => array:1 [ ... one achievement ... ] "Leaderboards" => [] ] 1 => array:8 [ "GameAchievementSetID" => 5 "CoreGameID" => 3 "Title" => "Bonus 2" "Type" => "bonus" "ImageIcon" => "/Images/000011.png" "ImageIconURL" => "/media/Images/000011.png" "Achievements" => array:3 [ ... three achievements ... ] "Leaderboards" => [] ] ] ] ] // tests/Feature/Connect/Actions/BuildClientPatchDataActionTest.php:410

Should root-level Title, ImageIcon, and ImageIconURL always point to the core game?

I think that if the client is requesting patch data by a subset hash, the top-level data should be from the core set, and the subset as a child record.

Do root-level Achievements and Leaderboards even matter?

The response structure still has root-level fields, it would make sense to return the core data at the root level.

For legacy compatibility, when requesting a subset by game_id, the subset information should be returned at the top level, and no child records should be returned.

If we're going to eliminate the top-level records, maybe we should have a new API (i.e. patch2) rather than trying to overload the existing API.

Should RichPresencePatch take on the RP script of the "subset game"?

I think subsets shouldn't have their own RP scripts. This may need further discussion.

It sounds like elimination of the top-level records is probably not the right way to go (which also aligns with how this PR is currently implemented). We just need to make sure the top-level values being sent to emulators are correct.

Just to confirm my understanding:

I think that if the client is requesting patch data by a subset hash, the top-level data should be from the core set, and the subset as a child record.

The response structure still has root-level fields, it would make sense to return the core data at the root level.

In other words, let's say I load "Dragon Quest III [Subset - Bonus]". Is it correct that the top-level Title, ImageIcon, ImageIconURL, Achievements, and Leaderboards should be from "Dragon Quest III", not "Dragon Quest III [Subset - Bonus]"?

For legacy compatibility, when requesting a subset by game_id, the subset information should be returned at the top level, and no child records should be returned.

Agreed - this fallback functionality should already be present in this PR.

I think subsets shouldn't have their own RP scripts. This may need further discussion.

This is where I feel less sure. Let's have a follow-up chat, but I can imagine some developers will get frustrated if their RP scripts attached to subsets are suddenly considered invalid.

In other words, let's say I load "Dragon Quest III [Subset - Bonus]". Is it correct that the top-level Title, ImageIcon, ImageIconURL, Achievements, and Leaderboards should be from "Dragon Quest III", not "Dragon Quest III [Subset - Bonus]"?

Yes.

I can imagine some developers will get frustrated if their RP scripts attached to subsets are suddenly considered invalid.

Are there non-exclusive subsets with unique RP?

Subset:

Core:

Subset:

Core:

Subset:

Core:

Found one. Though I could argue that the core RP is better. Maybe the prefix could be merged into that?

Subset:

Core:

Perhaps since "specialty" and "exclusive" subsets require loading unique hashes, they could also have unique RP, but generic "bonus" sets would not.

All this logic should now be implemented in latest, with additional tests added to verify it.

RP patch data from specialty and exclusive subset games is now prioritized when a user loads a hash associated with one of those games.

If the user is not globally opted out of multiset, root-level data reflects the values of the core/base game.

Thought related to RP: how are we going to maintain/create sessions for the subsets? Will ping be overloaded to send all game_ids that are active? Will we just run the active subset logic on the server like we do for building the patch data?

I thin this comes down to "specialty" and "exclusive" subsets again. Do we still need "active players" for generic bonus subsets?

Do we still need subset sessions in the database for associating unlocks, or can they be tied to the core subset?

wescopeland · 2024-11-24T21:14:05Z

public/dorequest.php

+            $response = (new BuildClientPatchDataAction())->execute(
+                gameHash: $gameHash,
+                game: $game,
+                user: $user,
+                flag: AchievementFlag::tryFrom($flag),
+            );
+
+            // Based on the user's current client support level, we may want to attach
+            // some metadata into the patch response. We'll do that as part of a separate
+            // action to keep the original data construction pure.
+            $response = (new InjectPatchClientSupportLevelDataAction())->execute(
+                $response,
+                $clientSupportLevel,
+                $gameHash,
+                $game,
+            );


This is how I integrated the new stuff from #2862.

First, execute BuildClientPatchDataAction, which always should build the correct patch package for the game regardless of the user's client.

Next, call InjectPatchClientSupportLevelDataAction on the returned patch data. This is almost like a side-effect, injecting whatever warning achievements and warning metadata we may want to send to a client.

wescopeland · 2024-11-24T21:14:33Z

tests/Feature/Connect/Actions/ResolveAchievementSetsActionTest.php

No functional changes in here, just some cleanup for readability.

Jamiras

I'm still waffling on how to handle "root data" for specialty/exclusive sets. In those cases, the core set is secondary or not even present, so is the player still playing the base game? My gut says the root id and achievements shouldn't be from the base game, but maybe the title and image should?

Jamiras · 2024-11-30T14:53:13Z

tests/Feature/Connect/Actions/BuildClientPatchDataActionTest.php

+        $this->assertEquals($baseGame->id, $result['PatchData']['ID']);
+        $this->assertEquals($baseGame->title, $result['PatchData']['Title']);
+        $this->assertEquals($baseGame->ImageIcon, $result['PatchData']['ImageIcon']);
+        $this->assertCount(3, $result['PatchData']['Achievements']); // the base game's achievements


I'm not convinced this is right. If I'm playing a specialty set, that's what I'm focused on and the core achievements are "bonus".

Jamiras · 2024-11-30T14:56:15Z

tests/Feature/Connect/Actions/BuildClientPatchDataActionTest.php

+        $this->assertEquals($baseGame->id, $result['PatchData']['ID']);
+        $this->assertEquals($baseGame->title, $result['PatchData']['Title']);
+        $this->assertEquals($baseGame->ImageIcon, $result['PatchData']['ImageIcon']);
+        $this->assertCount(3, $result['PatchData']['Achievements']); // ... base game's achievements ...


Exclusive sets load without the core achievements, so there's even less reason for the root data to be from the core game.

wescopeland added 12 commits November 11, 2024 19:06

refactor(AchievementFlag): use a backed enum

a801b78

fix: game page flag

76b0af4

feat(multiset): add action to resolve sets for emulators

cb43c1a

Merge branch 'master' into multiset-resolve-sets-action

b9c13d7

ci: fallback install

e26484c

Merge branch 'multiset-resolve-sets-action' of https://github.com/wes…

f0c5dbd

…copeland/RAWeb into multiset-resolve-sets-action

chore: revert ci change

f241d39

feat: include core game ids

d550f43

chore: clarify

40cc59a

feat(multiset): send sets to emulators

c2d9446

Merge branch 'master' into multiset-resolve-sets-action

216e613

Merge branch 'multiset-resolve-sets-action' into multiset-patch-data

7b598ab

wescopeland requested a review from a team November 17, 2024 01:20

wescopeland mentioned this pull request Nov 19, 2024

add user agent information to emulator filament console #2862

Merged

wescopeland added 2 commits November 19, 2024 21:41

Merge branch 'master' into multiset-resolve-sets-action

0e17857

Merge branch 'multiset-resolve-sets-action' into multiset-patch-data

1ae1571

wescopeland removed the request for review from a team November 23, 2024 20:49

wescopeland marked this pull request as draft November 23, 2024 20:49

wescopeland added 3 commits November 24, 2024 15:16

Merge branch 'master' into multiset-patch-data

7279a62

refactor: use an inject action, change namespace

c939cb8

test: add coverage

75f2b68

wescopeland marked this pull request as ready for review November 24, 2024 21:03

wescopeland requested a review from a team November 24, 2024 21:04

chore: remove todo

472ff2d

wescopeland commented Nov 24, 2024

View reviewed changes

wescopeland added 2 commits November 29, 2024 15:34

fix: address pr feedback

c0daa17

Merge branch 'master' into multiset-patch-data

d885e96

wescopeland requested a review from Jamiras November 29, 2024 20:44

Jamiras reviewed Nov 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(multiset): send sets to emulators #2857

feat(multiset): send sets to emulators #2857

wescopeland commented Nov 17, 2024 •

edited

Loading

wescopeland commented Nov 24, 2024

wescopeland Nov 24, 2024

Jamiras Nov 27, 2024

wescopeland Nov 27, 2024

Jamiras Nov 27, 2024

Jamiras Nov 27, 2024

wescopeland Nov 29, 2024

Jamiras Nov 30, 2024

wescopeland Nov 24, 2024

wescopeland Nov 24, 2024

Jamiras left a comment

Jamiras Nov 30, 2024

Jamiras Nov 30, 2024


		// TODO more tests

		// OPEN QUESTION when given a GameHash, should the root Achievements & Leaderboards be returned as `null`?

	// 🔴 JAMIRAS LOOK HERE
	// "Sets" only contains $baseGame and $bonusGame2.
	// However, PatchData still contains all the pre-multiset stuff, eg
	// - "Title" is "Dragon Quest III [Subset - Bonus]"
	// - "Achievements" has 2 published achievements
	// Is this ok left as-is, or should this data be changed?

feat(multiset): send sets to emulators #2857

Are you sure you want to change the base?

feat(multiset): send sets to emulators #2857

Conversation

wescopeland commented Nov 17, 2024 • edited Loading

wescopeland commented Nov 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jamiras left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wescopeland commented Nov 17, 2024 •

edited

Loading