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

Core: Remove Universally Unique ID Requirements (Per-Game Data Packages) #1933

Merged
merged 55 commits into from
Jun 1, 2024

Conversation

ThePhar
Copy link
Member

@ThePhar ThePhar commented Jul 3, 2023

What is this fixing or adding?

Note: This is a breaking change that may require some clients (and libraries) to handle potential ID conflicts.

The goal of this PR is to remove the requirement that item and location IDs must be unique across all worlds. IDs must still remain unique to their own world, but other worlds may now utilize that ID space for their implementations. This means it's expected for a client to check which game an ID is relevant for, prior to lookup.

This will allow any world developer to use more sensible IDs without need of a base_id_offset`-type addition when building item and location tables, without worry of an ID conflict with another hypothetical world, if so desired.

Data Package Changes / Expectations

Clients that rely on looking up names for location and item IDs will most likely need to change how they perform these lookups to support this paradigm, otherwise, at best, names could be incorrect, and at worst, games will not function. The only changes to the Data Package itself will be the removal of version as this update also drops data_version support. If a client or library relies on version for data package caching, it will also stop working.

For library and client developers, the recommended way to perform name lookup for an ID is to check the relevant player's NetworkSlot data for their world, then lookup from within the corresponding game's GamePackage.

An example lookup scenario:

  1. You connect and authenticate to the Archipelago server and the server responds with a Connected packet.

  2. You locally cache the slot_info data from Connected packet for later lookup.

  3. Eventually, you need to scout information in a location, so you send out a LocationScouts packet for location: 100.

  4. Server responds with a LocationInfo packet with the following payload:

    [{
        "cmd": "LocationInfo",
        "locations": [{ 
            "item": 1, 
            "location": 100, 
            "player": 4, 
            "flags": 1
        }]
    }] 
    
  5. You check player 4 in the slot_info data you locally cached on connection and find out they're playing Blique.

  6. You can now do your lookup inside the Blique game package and get the name for item 1.

CommonClient/CommonContext changes

CommonContext.item_names and CommonContext.location_names is now a specialized dict (called NameLookupDict) for looking up names, separated by game. To make it easier for developers who derive from CommonContext, some additional helper methods were created to make getting these names without explicitly finding the game name yourself.

def lookup_in_slot(self, code: int, slot: Optional[int] = None) -> str:
"""Returns the name for an item/location id in the context of a specific slot or own slot if `slot` is omitted."""

def lookup_in_game(self, code: int, game_name: Optional[str] = None) -> str:
"""Returns the name for an item/location id in the context of a specific game or own game if `game` is omitted."""

# Example usage:
# received_item: NetworkItem
# sender_name: str

item_name = ctx.item_names.lookup_in_slot(received_item.item)
location_name = ctx.location_names.lookup_in_slot(received_item.location, received_item.player)
logger.log(f"Received {item_name} from {sender_name} ({location_name})")

It is still possible to lookup names via {type}_names[id], but this should be considered deprecated and may fail to load the correct name if multiple items or locations utilize the same id. Instead, developers should switch to using the above helper methods or defining the game if accessing directly from the dict itself (e.g., item_names[game][id]) Eventually, this implicit fallback will be removed.

Core Changes

This change also increments the version of Archipelago to 0.5.0 considering the level of breaking change this contains.

How was this tested?

Used a modified version of Clique that has ids that match another game and confirmed MultiServer and TextClient worked fine.

Couple unit tests also added for changes to CommonContext.

I have provided a website for "Clique1933" which shares the same IDs as "Clique" to test the client and server changes, as well as to test your own clients/lib to ensure it doesn't crash when this game interacts. See link for apworld.

Clique1933 Client: http://darkshare.site.nfoservers.com/clique_1933/
Clique1933 APWorld: http://darkshare.site.nfoservers.com/clique_1933/clique_1933.apworld

Please be sure to mark libraries and clients that have updated for this change in #3394.

If this makes graphical changes, please attach screenshots.

N/A

@ThePhar ThePhar added is: documentation Improvements or additions to documentation. meta: help wanted Additional review/assistance is requested for these issues or pull requests. is: refactor/cleanup Improvements to code/output readability or organizization. affects: core Issues/PRs that touch core and may need additional validation. affects: webhost Issues/PRs that touch webhost and may need additional validation. labels Jul 3, 2023
@ThePhar ThePhar self-assigned this Jul 3, 2023
@black-sliver
Copy link
Member

black-sliver commented Jul 3, 2023

Other things that should probably be part of this PR

  • update docs and protocol
  • define and require min client version
  • reject upload of non-checksum seeds in webhost
  • either upgrade the DB or disable spinning up of non-checksum seeds - upgrade could be done on demand or as a click. If we decide to disable old seeds instead, we should probably clean out the database at some point.
  • remove code path from custom server and/or MultiServer that loads the Datapackage from worlds/

@ThePhar
Copy link
Member Author

ThePhar commented Jul 3, 2023

I know Berserker and I were talking about potentially deleting old seeds. @Berserker66 would probably have a better idea on the feasibility/practicality of upgrading the DB.

@ThePhar ThePhar added the is: maintenance Regular updates to requirements and utilities that do not fix bugs or change/add features. label Jul 4, 2023
MultiServer.py Fixed Show fixed Hide fixed
@ScipioWright
Copy link
Collaborator

ScipioWright commented Jul 22, 2023

The Noita mod/client now supports this change. It has a cache for the room to link up the slot number for each player with the game they are playing, and uses that as a reference for item/location names.

Edit: okay rereading this, this comment isn't super clear. I'm just following the "ideal way" as mentioned above with no real variation.

MultiServer.py Outdated Show resolved Hide resolved
WIP process on refactoring `tracker.py` to handle changes and prep for
@ThePhar ThePhar force-pushed the per_game_datapackage branch from 7a1708c to c02dad2 Compare November 3, 2023 00:58
@el-u
Copy link
Collaborator

el-u commented Nov 5, 2023

I don't disagree with the import and string changes in general, but including so many unrelated changes in this already large PR unnecessarily makes it less clear to read.

@ThePhar
Copy link
Member Author

ThePhar commented Nov 5, 2023

I don't disagree with the import and string changes in general, but including so many unrelated changes in this already large PR unnecessarily makes it less clear to read.

This PR is going to be renamed later. Part 1 of tracker clean up goes hand in hand with this PR. Not going to hackily make tracker.py work and then redo all my work again afterwards.

@ThePhar ThePhar changed the title Core: Support "Per Game Data Packages" and drop data_version support from core. Core: Tracker API refactor, Per Game Data Packages, and drop support for World.data_version. Nov 5, 2023
@ThePhar ThePhar changed the title Core: Tracker API refactor, Per Game Data Packages, and drop support for World.data_version. Core, WebHost: Tracker API refactor, Per Game Data Packages, and drop support for World.data_version. Nov 5, 2023
@ThePhar
Copy link
Member Author

ThePhar commented May 29, 2024

Yep it was hints, just had to pass along player info in the node. Seems to work for me now.

CommonClient.py Outdated Show resolved Hide resolved
@@ -1,180 +0,0 @@
<!DOCTYPE html>
Copy link
Member

Choose a reason for hiding this comment

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

Why is this getting deleted in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it snuck back into main somehow. The real tracker was renamed tracker__OcarinaOfTime.html so this isn't evne used anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose it could have been in another PR.

Copy link
Member

@Berserker66 Berserker66 left a comment

Choose a reason for hiding this comment

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

Looked it over, remarked what I noticed and my clients seem to still work.

@ThePhar ThePhar merged commit 5aa6ad6 into ArchipelagoMW:main Jun 1, 2024
16 checks passed
@ThePhar ThePhar deleted the per_game_datapackage branch June 1, 2024 11:07
ThePhar added a commit to ThePhar/Archipelago that referenced this pull request Jun 1, 2024
ThePhar added a commit that referenced this pull request Jun 3, 2024
wu4 pushed a commit to wu4/Archipelago that referenced this pull request Jun 6, 2024
wu4 pushed a commit to wu4/Archipelago that referenced this pull request Jun 6, 2024
agilbert1412 pushed a commit to agilbert1412/Archipelago that referenced this pull request Jun 13, 2024
jnschurig pushed a commit to Tranquilite0/Archipelago-SoulBlazer that referenced this pull request Jun 13, 2024
jnschurig pushed a commit to Tranquilite0/Archipelago-SoulBlazer that referenced this pull request Jun 13, 2024
sflavelle pushed a commit to sflavelle/Archipelago-tgc that referenced this pull request Jun 20, 2024
sflavelle pushed a commit to sflavelle/Archipelago-tgc that referenced this pull request Jun 20, 2024
qwint pushed a commit to qwint/Archipelago that referenced this pull request Jun 24, 2024
qwint pushed a commit to qwint/Archipelago that referenced this pull request Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: core Issues/PRs that touch core and may need additional validation. affects: webhost Issues/PRs that touch webhost and may need additional validation. is: documentation Improvements or additions to documentation. is: refactor/cleanup Improvements to code/output readability or organizization. meta: help wanted Additional review/assistance is requested for these issues or pull requests. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants