-
-
Notifications
You must be signed in to change notification settings - Fork 20
feat: use full minecraft version to get pack_format #472
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
base: main
Are you sure you want to change the base?
Conversation
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.
mostly approved, makes sense to me
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.
not sure if the logic for the file should be here, unsure where to put it ..
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.
yes, i didn't want to read the json every time the pack format is accessed
split_version(x.id): x.resource_pack_version | ||
for x in pack_format_registry | ||
if x.type == "release" | ||
} |
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.
i guess this file is eager instead of lazy. which is unlike most other files in beet. unsure how i feel about that
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.
i can change it to be lazy, like creating a cached function that return a object with all pack format registry, it would look like this :
class RegistryHolder(BaseModel):
pack_format_registry: list[PackFormatRegistry]
data_pack_format_registry: dict[Tuple[int, ...], int]
resource_pack_format_registry: dict[Tuple[int, ...], int]
@cache
def get_pack_format_registry() -> RegistryHolder:
"""Get the pack format registry."""
data = json.loads(
files("beet.resources").joinpath(f"pack_format_registry.json").read_text()
)
pack_format_registry: list[PackFormatRegistry] = []
for item in data:
pack_format_registry.append(PackFormatRegistry.model_validate(item))
data_pack_format_registry = {
split_version(x.id): x.data_pack_version
for x in pack_format_registry
if x.type == "release"
}
resource_pack_format_registry = {
split_version(x.id): x.resource_pack_version
for x in pack_format_registry
if x.type == "release"
}
return RegistryHolder(
pack_format_registry=pack_format_registry,
data_pack_format_registry=data_pack_format_registry,
resource_pack_format_registry=resource_pack_format_registry,
)
EDIT : copilot suggested this :
"""
Pack format registry resource from https://raw.githubusercontent.com/misode/mcmeta/refs/heads/summary/versions/data.json
"""
__all__ = [
"PackFormatRegistry",
"pack_format_registry",
"data_pack_format_registry",
"resource_pack_format_registry",
]
from importlib.resources import files
import json
from functools import lru_cache
from pydantic import BaseModel
from beet.core.utils import split_version
class PackFormatRegistry(BaseModel):
id: str
name: str
release_target: str | None
type: str
stable: bool
data_version: int
protocol_version: int
data_pack_version: int
resource_pack_version: int
build_time: str
release_time: str
sha1: str
@lru_cache(maxsize=1)
def pack_format_registry() -> list[PackFormatRegistry]:
"""Get the pack format registry, loaded lazily."""
data = json.loads(
files("beet.resources").joinpath("pack_format_registry.json").read_text()
)
return [PackFormatRegistry.model_validate(item) for item in data]
@lru_cache(maxsize=1)
def data_pack_format_registry() -> dict[tuple[int, ...], int]:
"""Get the data pack format registry, loaded lazily."""
return {
split_version(x.id): x.data_pack_version
for x in pack_format_registry()
if x.type == "release"
}
@lru_cache(maxsize=1)
def resource_pack_format_registry() -> dict[tuple[int, ...], int]:
"""Get the resource pack format registry, loaded lazily."""
return {
split_version(x.id): x.resource_pack_version
for x in pack_format_registry()
if x.type == "release"
}
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.
i also thinks it makes sense to pull the file remotely and cache it maybe? we do that for vanilla but that's also an optional thing versus this is something that people will use often.
thanks for providing both implementations
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.
But pulling it directly from misode/mcmeta makes it impossible to use beet offline, i like the idea to use misode/mcmeta file but we still needs to add a local registry as a backup strategy
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.
I don't personally like the idea of relying on misode/mcmeta
directly during runtime.
{ | ||
"id": "1.14", | ||
"name": "1.14", | ||
"release_target": "1.14", |
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.
Beet technically supported down to 1.13 before. I'm not sure if we care about this regression?
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.
I wasn't taking into account this, beet technically support MC 1.6 pack_format 1. Might be needed to hardcode even more values.
{ | ||
"id": "1.21.6", | ||
"name": "1.21.6", | ||
"release_target": null, |
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.
This is a pretty large file and has a lot of unused data. Do we want to try to generate this file (in github actions?) and preprocess it to only keep the relevant fields?
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.
Might be a good idea, i was taking this file because it was very convenient.
I was sorting informations i needed in python, but realistically we don't need :
- snapshots (as element in the list)
- release_target
- type
- stable
- build_time
- release_time
- sha1
"id": "25w21a", | ||
"name": "25w21a", | ||
"release_target": null, | ||
"type": "snapshot", |
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.
Is it necessary (or even a good idea) to support snapshots? Mecha and other beet features might not necessarily support these versions. Maybe we should just hardcode a list of pack formats for all the release versions?
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.
As discuss in others comments, the plan wasn't to support snapshot.
data_pack_format_registry = { | ||
split_version(x.id): x.data_pack_version | ||
for x in pack_format_registry | ||
if x.type == "release" |
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.
Was this tested? Does this work without any other changes? I am guessing the map created here will have tuples with mixed length 2 and 3? Before they were always tuples of length 2.
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.
The return type of split_version
was always Tuple[int, ...]
, so any tuple of any length. Also i tested this version against mecha, bolt and my repos and found no breaking changes.
@@ -1,6 +1,6 @@ | |||
{ | |||
"pack": { | |||
"pack_format": 9, | |||
"pack_format": 8, |
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.
This illustrates that this PR is a breaking change. Specifying minecraft: 1.18
in the config now specifies exactly 1.18
, not the last 1.18.x
. I know we discussed this a couple months ago in a google docs, but we'll need to be very careful merging this if people rely on this config.
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.
I don't think it's a big deal personally but i account for the issue.
Here's the ggdoc : https://docs.google.com/document/d/1m0niC2I80gMS_yRjQFP9PglhxztAs0b8O5y7XshXp-k/edit?tab=t.0#heading=h.qkfryuqiybl2
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.
Your last comment reminded me of the existence of this ggdoc, it would be a good idea to implement this in another PR.
{ | ||
"id": "1.21.6", | ||
"name": "1.21.6", | ||
"release_target": null, |
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.
Might be a good idea, i was taking this file because it was very convenient.
I was sorting informations i needed in python, but realistically we don't need :
- snapshots (as element in the list)
- release_target
- type
- stable
- build_time
- release_time
- sha1
"id": "25w21a", | ||
"name": "25w21a", | ||
"release_target": null, | ||
"type": "snapshot", |
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.
As discuss in others comments, the plan wasn't to support snapshot.
{ | ||
"id": "1.14", | ||
"name": "1.14", | ||
"release_target": "1.14", |
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.
I wasn't taking into account this, beet technically support MC 1.6 pack_format 1. Might be needed to hardcode even more values.
data_pack_format_registry = { | ||
split_version(x.id): x.data_pack_version | ||
for x in pack_format_registry | ||
if x.type == "release" |
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.
The return type of split_version
was always Tuple[int, ...]
, so any tuple of any length. Also i tested this version against mecha, bolt and my repos and found no breaking changes.
@@ -1,6 +1,6 @@ | |||
{ | |||
"pack": { | |||
"pack_format": 9, | |||
"pack_format": 8, |
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.
I don't think it's a big deal personally but i account for the issue.
Here's the ggdoc : https://docs.google.com/document/d/1m0niC2I80gMS_yRjQFP9PglhxztAs0b8O5y7XshXp-k/edit?tab=t.0#heading=h.qkfryuqiybl2
This PR fix #471 by creating a database of minecraft version in a resources folder (like what mecha does for command trees) and using it to get the pack_format.
Disadvantages :
This transform the two old classvar to properties, they act the same with an object of that class, but breaks few thing that were using the class itself.fixedThis PR comes with a PR for mecha with new command tree.