Feature: Subpack loading

[onebeastchris]

Implements #4939

Potential usages:

• different textures/animations depending on platform to e.g. account for different performance levels
• shipping one pack for different servers

Further, this also adds setters for contentKey and the subpackName to the ResourcePack interface. These would, imo, also benefit remote resource pack - those values cannot be read directly from the resource pack, so it makes sense to allow these to be modifies after reading the pack.

[letsgoawaydev]

For what ever reason, it seems like the settings object is capable of having other elements like a server form, but it doesnt actually do anything. So I think that this is good atm

[onebeastchris]

huh, interesting. We could also parse those, but given that they're non-functional, parsing just type + text seems fine

[letsgoawaydev]

huh, interesting. We could also parse those, but given that they're non-functional, parsing just type + text seems fine

yeah probably doesnt need to be parsed in terms of minimums and maximums because majority of packs wouldn't have it anyway

[letsgoawaydev]

also, maybe this should be api/2.4.2 ?

[onebeastchris]

That's unlikely - we want to get api/2.4.2 done as soon as possible (which would currently just include #4957), all other current API PRs will likely be pushed to a separate release. The priority is to get 1.21.20 support out before it officially releases, we don't want to end up in a situation where API PR's cause further holdups.

[Konicai]

Link to https://learn.microsoft.com/en-us/minecraft/creator/documents/utilizingsubpacks

[Konicai]

Refer as this subpack instead of the subpack, and as required in other methods

[Konicai]

Suggested change

	* Gets the memory tier of the subpack.
	* One memory tier requires 0.25 GB of free memory
	* that a device must have to run a sub-pack.
	*
	* @return the memory tier
	*/
	@Nullable
	Float memoryTier();
	* Gets the memory tier of this Subpack, representing how much RAM a device must have to run it.
	* Each memory tier requires 0.25 GB of RAM. For example, a memory tier of 0 is no requirement,
	* and a memory tier of 4 requires 1GB of RAM.
	*
	* @return the memory tier
	*/
	@Nullable
	Float memoryTier();

It seems like it is based off of device specs, not the active amount of free memory? Also it seems like the memory tier is an integer? Otherwise comment on the discrepancy with MS docs

[onebeastchris]

I copied the values from the GeyserMC/packconverter generated stuff; in there it's a float

The memory thing is explicitly explained on the ms docs, where 1 "memory tier unit" requires 0.25 GB of storage.

From my testing, it seems like this value does not even matter for our usecase, as the user can't change which subpack to use on their own (that can only be set when the pack is loaded on the client, and not while in a world)

[Konicai]

I copied the values from the GeyserMC/packconverter generated stuff; in there it's a float

ok.

The memory thing is explicitly explained on the ms docs, where 1 "memory tier unit" requires 0.25 GB of storage.

I mean it doesn't use the word "unit", it just says "each tier represents .25 GB". I've never interpreted the words "unit" or "tier" as fractional. Can you share the pack you've tested with and I'll try it with the client directly?

[Konicai]

Considering that the content key is static with respect to the resource pack data source, I'm under the impression this shouldn't be allowed to change.

The PackCodec, responsible for reading the resource pack manifest, is also responsible for determining the resource pack content key.

[onebeastchris]

I'm aware of the current pack codec impl that tries to resolve a key file - but I don't see a reason why a user couldn't provide the key themselves :p

Although yeah, we could probably also make sure it can only be set once

[Konicai]

The addition of the two mutations seems dangerous. The first thought is that we don't want to be mutating packs that are in the Geyser registry just because of the way we are sending them to the client.

More importantly, packs need to be thread safe because they are shared by sessions, if I understand correctly?

We could add additions to SessionLoadResourcePacksEvent, without need for deprecations:

public abstract @NonNull List<Entry> entries();
public abstract boolean register(@NonNull ResourcePack resourcePack, @NonNull String subpack);

public interface Entry {
    @NonNull ResourcePack resourcePack();
    @NonNull String subpack();
}

[onebeastchris]

Personally, I'd like to avoid adding multiple register methods - they'd have to be duplicated for the eventual GeyserDefineResourcePacks event

[Konicai]

Then make Entry a separate class and use that for any register methods that take any more parameters than just ResourcePack. Or something like that.

[onebeastchris]

Going to close this - Konicai pointed out vital flaws with this implementation (thanks konicai!), so i've began working on a different approach. master...onebeastchris:Geyser:subpacks-rewrite is the current idea; it will probably change further.