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

Implement client-side support of downloading RP from CDN #6064

Closed
wants to merge 7 commits into from

Conversation

alvin0319
Copy link
Contributor

Introduction

Implement client-side support of downloading Resource Pack from CDN.

Relevant issues

Changes

API changes

  • Added ResourcePackManager::getPackURLs(): Returns a map of uuid_version => cdn url

Behavioural changes

The server does not send ResourcePackChunkDataPacket since the client downloads the resource pack from CDN.

Backwards compatibility

There are no known BC breaking changes.

Follow-up

Tests

Test CDN server used with this test: https://github.com/alvin-pm-pl/ResourcePackCDN

Test Pack used with this test: https://mcpedl.com/legendary-bedrock-rtx-texture-pack/

2023-09-22.00-02-22.mp4

Copy link
Member

@ShockedPlot7560 ShockedPlot7560 left a comment

Choose a reason for hiding this comment

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

Looks OK for me

src/resourcepacks/ResourcePackManager.php Outdated Show resolved Hide resolved
src/resourcepacks/ResourcePackManager.php Outdated Show resolved Hide resolved
@BrandPVP
Copy link
Contributor

BrandPVP commented Oct 2, 2023

«However, the resource packs still need to be loaded into PocketMine.»

It should automatically download the pack from the url and cache the metadata, shouldn’t it? Like, why do users need to put it manually when PM can automatize the job

@alvin0319
Copy link
Contributor Author

«However, the resource packs still need to be loaded into PocketMine.»

It should automatically download the pack from the url and cache the metadata, shouldn’t it? Like, why do users need to put it manually when PM can automatize the job

I don't think it should be a PM's job. We should consider a offline environment.

@ShockedPlot7560
Copy link
Member

In fact, I'm going back on my opinion because I hadn't realized the problem that has just been raised. Although you need to be in an offline environment, it can be interesting to enable automation.

@alvin0319
Copy link
Contributor Author

Well, the problem is that:

  1. We can't download the resource packs from CDN every time we turn on the server.
  2. If we cache the downloaded resource pack, we have no way to detect whether resource pack content changed unless CDN provides a checksum or sha1 hash of the file.

The reason for requiring Resource Packs to be loaded into PM is that the client also expects info of remote resource packs in ResourcePacksInfoPacket. (after that, this isn't necessary since they download from external)

As an alternative way, we could let users write the UUID and version of the resource pack and optionally content key (which is the least information the client expects) in resource_packs.yml so that we don't let users load the resource pack into PM.

@dktapps
Copy link
Member

dktapps commented Oct 5, 2023

General sentiment in the dev community so far is that this feature is pretty useless for anyone who doesn't have a perfect connection (i.e. anywhere that isn't the US or Europe).

@ShockedPlot7560 ShockedPlot7560 added Category: API Related to the plugin API Category: Core Related to internal functionality Type: Enhancement Contributes features or other improvements to PocketMine-MP labels Dec 16, 2023
Copy link
Contributor

@jasonw4331 jasonw4331 left a comment

Choose a reason for hiding this comment

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

I would like to see added support for URLs without the https:// header also.

src/resourcepacks/ResourcePackManager.php Outdated Show resolved Hide resolved
Joshy3282 added a commit to Joshy3282/PocketMine-MP that referenced this pull request Oct 3, 2024
@dktapps
Copy link
Member

dktapps commented Nov 29, 2024

I do think the cleanest way to do this would be to just allow putting URLs in the resource_stack instead of filenames. The way this is currently done seems janky to me.

Well, the problem is that:

  1. We can't download the resource packs from CDN every time we turn on the server.
  2. If we cache the downloaded resource pack, we have no way to detect whether resource pack content changed unless CDN provides a checksum or sha1 hash of the file.

The reason for requiring Resource Packs to be loaded into PM is that the client also expects info of remote resource packs in ResourcePacksInfoPacket. (after that, this isn't necessary since they download from external)

As an alternative way, we could let users write the UUID and version of the resource pack and optionally content key (which is the least information the client expects) in resource_packs.yml so that we don't let users load the resource pack into PM.

I'm not understanding the logic here. What's stopping PM from downloading packs from CDN whenever the server boots up?

I don't really buy into the point about caching either. If we have users hardcode pack UUIDs into the YML, we're already caching info, just in a way that the server can't control programmatically. Vs if the pack is downloaded, at least the information is guaranteed to be correct for some version of the resource pack. We can also look at Cache-Control headers to see when a resource pack's local cache should expire.

@dktapps
Copy link
Member

dktapps commented Nov 29, 2024

This also needs to be updated for latest protocol changes

Copy link

github-actions bot commented Dec 8, 2024

This PR has been marked as "Waiting on Author", but we haven't seen any activity in 7 days.

If there is no further activity, it will be closed in 28 days.

Note for maintainers: Adding an assignee to the PR will prevent it from being marked as stale.

@github-actions github-actions bot added the Stale label Dec 8, 2024
Copy link

github-actions bot commented Jan 6, 2025

As this PR hasn't been updated for a while, unfortunately we'll have to close it.

@github-actions github-actions bot added the Resolution: Abandoned PR has been abandoned by its author label Jan 6, 2025
@github-actions github-actions bot closed this Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: API Related to the plugin API Category: Core Related to internal functionality Resolution: Abandoned PR has been abandoned by its author Stale Status: Waiting on Author Type: Enhancement Contributes features or other improvements to PocketMine-MP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants