-
-
Notifications
You must be signed in to change notification settings - Fork 682
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
Implement sending remote resource packs to Bedrock clients #4205
Conversation
- Deprecate GeyserLoadResourcePacksEvent in favor of GeyserDefineResourcePacksEvent - Load CDNentries properly
In the config I would use, probably, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
api/src/main/java/org/geysermc/geyser/api/event/bedrock/SessionLoadResourcePacksEvent.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/geysermc/geyser/api/event/lifecycle/GeyserDefineResourcePacksEvent.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/geysermc/geyser/api/event/lifecycle/GeyserLoadResourcePacksEvent.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/geysermc/geyser/registry/loader/ResourcePackLoader.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/geysermc/geyser/registry/loader/ResourcePackLoader.java
Outdated
Show resolved
Hide resolved
…licate deprecation annotation
Yeah naming really isn't my strong suit, I agree. I kind of like RemotePack actually. |
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? |
This comment was marked as outdated.
This comment was marked as outdated.
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? |
After some testing:
|
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
…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).
From more testing locally, here's what i found:
Further, the resource pack can be either a 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.
It however would ensure correct content type/content lengths being set. On a positive note, downloading even a 100MB pack is insanely quick! |
// This doesn't seem to be a requirement (anymore?). Logging to debug as it might be interesting though. | ||
if (type == null || !type.equals("application/zip")) { | ||
logger.warning(String.format("Application type received from remote pack at URL %s uses the content type: %s! This may result in packs not loading " + | ||
"for Bedrock players.", url, type)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion this should say something along the lines of Try using a link that is application/zip type instead!
So that people know what the content type should be instead of being confused on why the content type wont work and not being able to know what the right one is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! Yeah; this is temporary to figure out what application types are (or aren't) supported by Bedrock edition while testing. This will be changed before merging.
So far, it unfortunately seems that only application/zip works - the popular octet-stream type seems to not work. Which is unfortunate, as that's what many services use
Put on hold until we transition to using configurate - otherwise these changes will result in confusion about why packs are loaded that don't seem to be in the config. |
Superseded by #4978 |
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! Further, this allows updating the remote resource packs while Geyser is still running: Geyser will detect the change, and re-download the pack. Unfortunately, this does not change when Bedrock clients can receive resource packs: only on the initial join.
Important
Note: You'll need to update Bedrock to the latest client version (1.21.1). 1.21.0 is unfortunately bugged, and ignores remote resource packs. 1.20.80 should also support this.
New config option:
This will ensure that all connecting clients will, by default, get the GeyserOptionalPack.
You can download builds here:
Technical details:
What is known:
application/zip
content type seems to work best for downloading packs. The often usedtext/html
type does not work (rip onedrive links)..zip
or.mcpack
Changes:
GeyserUrlPackCodec
(andUrlPackCodec
for api) classes that can create a pack based on url + content keyresource-pack-urls
entry in the config for remote packsTODO:
Would close #4060 - atleast the resource pack part.