Implement sending CDN entries to Bedrock clients to download resource packs from #4205

onebeastchris · 2023-10-10T20:12:12Z

Since 1.20.30, it is possible to send clients a link to download a resource pack from - instead of having to add the pack to Geyser's pack folder. This should help with proxy networks & overall traffic usage, as now resource intensive pack sending can be offloaded to a designated location. However, unlike on Java, it is possible to send multiple entries for the client to load packs from!

Notes:

This will only work on 1.20.30 and up. A workaround can be to block all versions below using the BlockVersion extension. ~~Or, an extension/a plugin could manually query the Bedrock client version and add either a link, or a pack (depending on version).~~ Update: If the client fails at getting the packs from the url, or does not support the entries, it will request the packs the old way.

Changes:

add new GeyserUrlPackCodec (and UrlPackCodec for api) that can create a pack based on url + content key
add a resource-pack-urls entry in the config for remote packs
try and use more fastutil for resource packs

RFC:

~~Should users be able to set pack encryption keys for the url's in the config?~~ No, users should use api instead for that.
~~What should be done if the link stops working (e.g. because packs do not actually provide the correct pack anymore? Should Geyser then attempt to reload all packs (or just the remote packs)?~~ Geyser will then serve the cached pack, and re-run a check of the url once - and warn about it.

We can detect it if the client sends a ResourcePackChunkRequestPacket - if the pack requested there is one with the UrlPackCodec, we can assume that either ~~1. the client does not support remote pack~~ to my knowledge , all platforms now do, or that 2. the remote pack changed/an error occurred/etc and that Geyser might want to re-check it.
However, it is not currently possible to modify registry contents, which makes that a bit more challenging.

~~Should Geyser download the remote packs once and provide that, or should Geyser rather try and get it directly from the url?~~ We'll be downloading the pack once, and cache it to avoid unnecessary re-downloads

TODO:

Testing; more of it
javadocs spellchecks
Proper error handling: URL invalid/offline, corrupt pack, manifest not where it's supposed to be, etc.
documentation on wiki
remove debug
dont let resource pack downloading block main thread - this is actually wanted; since Geyser should not start before we know all the resource packs
Don't re-download the pack if there's no need to (hash from url + size)

Would close #4060 - atleast the resource pack part.

…acks

- Deprecate GeyserLoadResourcePacksEvent in favor of GeyserDefineResourcePacksEvent - Load CDNentries properly

Camotoy · 2023-10-10T20:18:58Z

In the config I would use, probably, resource-pack-urls or remote-resource-packs. I don't think most people will understand what a CDN is, and some resource packs won't be hosted on content delivery networks per se.

Konicai

just a quick review without much depth - and i agree with Camo.

it would be nice if ResourcePackCDNEntry could be RemoteResourcePack, but it doesn't implement ResourcePack so... unsure about. the naming so far feels clunky tbh.

also consider using fastutil more, in the core module.

Konicai · 2023-10-10T20:38:22Z

api/src/main/java/org/geysermc/geyser/api/event/bedrock/SessionLoadResourcePacksEvent.java

+    /**
+     * Gets an unmodifiable list of {@link ResourcePackCDNEntry}s that will be sent to the client.
+     *
+     * @return an unmodifiable list of resource pack CDN entries that will be sent to the client.
+     */
+    public abstract @NonNull List<ResourcePackCDNEntry> cdnEntries();


Its not like we have a formal style guide, but if the javadoc body is the same as the @return tag, can we just have the return tag only?

Makes sense, but this has been accepted before & i'd rather keep it consistent (or change all instances of that in api)

RFC :P

and it exists elsewhere afaik

api/src/main/java/org/geysermc/geyser/api/event/lifecycle/GeyserDefineResourcePacksEvent.java

api/src/main/java/org/geysermc/geyser/api/event/lifecycle/GeyserLoadResourcePacksEvent.java

core/src/main/java/org/geysermc/geyser/registry/loader/ResourcePackLoader.java

core/src/main/resources/config.yml

…licate deprecation annotation

onebeastchris · 2023-10-10T22:27:32Z

Yeah naming really isn't my strong suit, I agree. I kind of like RemotePack actually.
Or we could take a page from kyori's adventure, their new resource pack method takes in a ResourcePackRequest (Like).. not sure I really like that though. Open for suggestions :)

Kas-tle · 2023-10-12T05:12:47Z

How exactly are pack versions handled with this? Is it just pulled from the downloaded pack? What happens if the version of the pack at the link is updated while the server is running?

onebeastchris · 2023-10-12T07:22:30Z

We don't extract / need the pack version, just the pack UUID. Konica and I had a brief talk where he mentioned that possibility where a pack UUID might change mid-game.
Since we can't really detect that, the next best thing would be to use some kind of cache and re-download the configured packs to re-check the pack ids.
This still leaves the question of what to do with api - imo we shouldn't do the same check for those remote packs provided there, since they need to provide the pack UUID themselves.

Possible solutions:

add a user-facing reload command that would re-gather all remote pack uuids
(e.g. /geyser reload packs) or similar; would also close a different issue
Could expose an api method that downloads the pack, extracts it & provides a UUID version to avoid duplicate code in extensions that also want to check
Could add a secondary registerRemotePacks/..CdnPacks method to the GeyserDefineResourcePacks only that would allow providing just a url and leave us querying the pack id ourselves.

More generally, we'd need to find out what exactly the client would do in the event that the pack is provided and the one received in said pack mismatch.

Kas-tle · 2023-10-12T15:24:39Z

Hmm ok but then how does this work? Does this mean the client has to download the entire pack every time to check if the version changed? The normal ResourcePackStack and ResourcePacksInfo packets have a field for version. So does a CDN entry also require an entry in the normal Entry list and this just tells the client where it can download said pack? Or will the client just download a CDN entry and not check the version at all?

I'm mainly wondering how this works if the user just adds an entry to their config for one of these. In this scenario you're saying we never have to know the pack version?

onebeastchris · 2023-10-14T00:00:32Z

~~By what I can tell, we never have to get/tell the client the pack version.This does raise the question of whether the client will always download the pack...~~
I'll definitely have to do more testing here; I am not fully sure about the exact workings of this feature. In my (brief) testing, the optional pack seems to have been loaded fine.

After some testing:

We need to send an entry in the ResourcePackInfo packet, even for these cdn packs.
the "packid" in the cdn entry consists of uuid_version, not just the UUID
the client falls back to requesting the packs normally/the old way if the download didn't succeed
We can actually detect the last part, and try and re-load packs. Not sure if we can do that fully automatically, but in theory, should be doable.

Instead: - add UrlPackCodec & GeyserUrlPackCodec - try and provide the resource pack via a stream from the url - either because the client does not support packs via the url, or because it failed to get the packs

…ails

…codec

…t, since it seems to work without it !?

…haos ensues since the client will either show bogus resource pack size values (invalid sizes), or skip resource packs altogether (if not properly zipped).

onebeastchris · 2023-11-09T11:51:33Z

From more testing locally, here's what i found:
This PR should be functional, but Bedrock is very picky about what sort of packs from a url are accepted/downloaded.

The link must be a direct link.
The size must be set properly in the Content-Length header. If size is wrong, or not present (-1), the client will show a bogus, stupidly large size value and wont allow downloading the packs.
Similarly, the Content-Type header must be set to application/zip - otherwise, the client wont download the resource packs.

Further, the resource pack can be either a .zip or .mcpacks file that does not contain the pack files at its root, but rather a folder with the actual resource pack. (In other words: zip/manifest.json wont work, zip/mypack/manifest.json will....)

If anyone wishes to test this PR, you can set up a working file server using caddy and the following config - please do not use it in production, it doesnt even use https.

http://localhost {
    file_server browse
    route {
        header Content-Type application/zip
        header Content-Length {file.size}
    }
}

It however would ensure correct content type/content lengths being set.

On a positive note, downloading even a 100MB pack is insanely quick!

Camotoy · 2023-11-09T16:00:36Z

Is it possible we can make a website that tests if a website hosting a pack is working correctly? We can also bundle such functionality in Geyser, but a website means it would be applicable to a wider Bedrock audience.

onebeastchris · 2023-11-09T16:05:19Z

Said functionality would already be bundled in this PR :)
We're basically downloading the pack once to get all the info, and while at it, we check size/content type/manifest location (which, to my knowledge, is what's required).
As for creating a separate website - sure, but I think that would be a bit out of scope for this PR, but it's not a bad idea in theory.

Camotoy · 2023-11-09T16:38:52Z

OK, neat. We should add to the config comment that you should expect your pack to be analyzed/downloaded within Geyser.

onebeastchris · 2023-11-09T17:07:02Z

Well, we basically have to download it. We need to include the pack in the same resource pack stack packet, and we need to get the UUID/version for it.
Another neat thing is that we can also detect if the url doesn't properly work anymore (e.g. pack changed mid download/link offline), and then Geyser would re-test the pack.

# Conflicts: # core/src/main/java/org/geysermc/geyser/configuration/GeyserConfiguration.java # core/src/main/java/org/geysermc/geyser/network/UpstreamPacketHandler.java

- better exception catching - proper error handling - caching of packs if size, etag, and last modified are the same

Tim203

Looks fine for the most part, just one small thing

Tim203 · 2023-12-31T14:06:43Z

core/src/main/java/org/geysermc/geyser/registry/loader/ResourcePackLoader.java

+            contentKey = Files.exists(keyFile) ? Files.readString(keyFile, StandardCharsets.UTF_8) : "";
+        } catch (IOException e) {
+            GeyserImpl.getInstance().getLogger().error("Failed to read content key for resource pack " + path.getFileName(), e);
+            contentKey = "";


Tim203 · 2023-12-31T14:15:14Z

api/src/main/java/org/geysermc/geyser/api/pack/UrlPackCodec.java

+     * @return the encryption key of the resource pack
+     */
+    @NonNull
+    public abstract String contentKey();


I personally don't like empty strings, can we make contentKey null when there is no contentKey?

https://github.com/GeyserMC/Geyser/blob/master/api/src/main/java/org/geysermc/geyser/api/pack/ResourcePack.java#L54-L60
Not against it; however, this is already being done the same way for other resource packs - i'd rather keep it consistent.

…asily

onebeastchris added 4 commits October 10, 2023 15:26

Initial stab at implementing 1.20.30's new CDN feature for resource p…

68516a8

…acks

Small tweaks: record formatting/javadocs

02d6473

- Don't require configuring the packId, just the link instead

dbfc153

- Deprecate GeyserLoadResourcePacksEvent in favor of GeyserDefineResourcePacksEvent - Load CDNentries properly

Remove forRemoval

f74d36a

onebeastchris added PR: Feature When a PR implements a new feature API The issue/feature request relates to the Geyser API labels Oct 10, 2023

Konicai requested changes Oct 10, 2023

View reviewed changes

onebeastchris added 2 commits October 10, 2023 23:02

Rename cdn-resource-packs to resource-pack-urls, fix test, remove dup…

94f2ea9

…licate deprecation annotation

Rename CDN entry list

fd69b0c

Move loading cdn entries to separate function

c0227d3

onebeastchris added the Work in Progress The issue is currently being worked on. label Oct 16, 2023

onebeastchris marked this pull request as draft October 16, 2023 17:43

onebeastchris added 10 commits October 17, 2023 19:06

Remove CDNEntry; those do not work as expected.

b9c5bdd

Instead: - add UrlPackCodec & GeyserUrlPackCodec - try and provide the resource pack via a stream from the url - either because the client does not support packs via the url, or because it failed to get the packs

remove outdated javadocs

27c1562

Fallback system - download the pack to serve the client in case cdn f…

5d06edd

…ails

Ensure GeyserUrlPackCodec.create returns a ResourcePack with the URL …

880de2d

…codec

Check downloaded resource packs, yeet cdn naming scheme

76a62ab

start on proper url checking (application type/size)

3670914

Change fallback system

498a415

Testing: Don't require application/zip or size or weird zip forma…

0004f5b

…t, since it seems to work without it !?

Merge remote-tracking branch 'upstream/master' into rp

cdd2aba

We need to ensure no invalid packs end up being loaded - otherwise, c…

15b8b93

…haos ensues since the client will either show bogus resource pack size values (invalid sizes), or skip resource packs altogether (if not properly zipped).

onebeastchris marked this pull request as ready for review November 9, 2023 11:44

onebeastchris added 5 commits November 10, 2023 20:24

Re-add debug: Apparently, not just application/zip works....???

d4f0d8a

Merge remote-tracking branch 'upstream/master' into rp

0ac91eb

# Conflicts: # core/src/main/java/org/geysermc/geyser/configuration/GeyserConfiguration.java # core/src/main/java/org/geysermc/geyser/network/UpstreamPacketHandler.java

remove debug, ensure we fully check failed packs, merge master

626189f

More robust downloading/caching

f121299

- better exception catching - proper error handling - caching of packs if size, etag, and last modified are the same

oops

303327a

onebeastchris added PR: Needs review Indicates that a PR is functional and review-ready. and removed Work in Progress The issue is currently being worked on. labels Dec 22, 2023

Tim203 requested changes Dec 31, 2023

View reviewed changes

onebeastchris requested review from Tim203 and Konicai January 15, 2024 18:54

onebeastchris added 2 commits January 16, 2024 00:47

Add a registerAll method to register a collection of resource packs e…

d2622a4

…asily

Merge remote-tracking branch 'upstream/master' into rp

4d99250

onebeastchris mentioned this pull request Jan 25, 2024

Draft: Document remote resource packs GeyserMC/GeyserWiki#244

Draft

2 tasks

Allow null content key

2e776c4

onebeastchris added PR: On hold When a PR is on hold like if it requires a dependency to be updated first and removed PR: Needs review Indicates that a PR is functional and review-ready. labels Feb 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement sending CDN entries to Bedrock clients to download resource packs from #4205

Implement sending CDN entries to Bedrock clients to download resource packs from #4205

onebeastchris commented Oct 10, 2023 •

edited

Camotoy commented Oct 10, 2023

Konicai left a comment •

edited

Konicai Oct 10, 2023

onebeastchris Oct 10, 2023

Konicai Oct 10, 2023

onebeastchris commented Oct 10, 2023 •

edited

Kas-tle commented Oct 12, 2023

onebeastchris commented Oct 12, 2023

Kas-tle commented Oct 12, 2023 •

edited

onebeastchris commented Oct 14, 2023 •

edited

onebeastchris commented Nov 9, 2023

Camotoy commented Nov 9, 2023

onebeastchris commented Nov 9, 2023

Camotoy commented Nov 9, 2023 •

edited

onebeastchris commented Nov 9, 2023

Tim203 left a comment

Tim203 Dec 31, 2023

Tim203 Dec 31, 2023

onebeastchris Jan 2, 2024

Implement sending CDN entries to Bedrock clients to download resource packs from #4205

Are you sure you want to change the base?

Implement sending CDN entries to Bedrock clients to download resource packs from #4205

Conversation

onebeastchris commented Oct 10, 2023 • edited

Camotoy commented Oct 10, 2023

Konicai left a comment • edited

Choose a reason for hiding this comment

Konicai Oct 10, 2023

Choose a reason for hiding this comment

onebeastchris Oct 10, 2023

Choose a reason for hiding this comment

Konicai Oct 10, 2023

Choose a reason for hiding this comment

onebeastchris commented Oct 10, 2023 • edited

Kas-tle commented Oct 12, 2023

onebeastchris commented Oct 12, 2023

Kas-tle commented Oct 12, 2023 • edited

onebeastchris commented Oct 14, 2023 • edited

onebeastchris commented Nov 9, 2023

Camotoy commented Nov 9, 2023

onebeastchris commented Nov 9, 2023

Camotoy commented Nov 9, 2023 • edited

onebeastchris commented Nov 9, 2023

Tim203 left a comment

Choose a reason for hiding this comment

Tim203 Dec 31, 2023

Choose a reason for hiding this comment

Tim203 Dec 31, 2023

Choose a reason for hiding this comment

onebeastchris Jan 2, 2024

Choose a reason for hiding this comment

onebeastchris commented Oct 10, 2023 •

edited

Konicai left a comment •

edited

onebeastchris commented Oct 10, 2023 •

edited

Kas-tle commented Oct 12, 2023 •

edited

onebeastchris commented Oct 14, 2023 •

edited

Camotoy commented Nov 9, 2023 •

edited