Add variant support for slugs & server version limitation #1356
Conversation
Pablete1234
force-pushed
the
add-version-requirements
branch
from
77caa04 to
e2a61a2
Compare
Signed-off-by: Pablo Herrera <[email protected]>
Pablete1234
force-pushed
the
add-version-requirements
branch
from
8812dba to
c7e3fc0
Compare
This patch adds the ability to prevent 1.13+ maps without a specified minimum version in `map.xml` from accidentally being loaded into legacy servers, causing undefined behavior including server crashes when cycled to. This is achieved by reading the data version (an integer increased with any Minecraft version, including snapshots [1]) from `level.dat` bundled with the map and inferring whether the map is a 1.13+ map. [1]: https://minecraft.wiki/w/Data_version Signed-off-by: TTtie <[email protected]>
Signed-off-by: Pablo Herrera <[email protected]>
There was a problem hiding this comment.
uh-oh! it blew up 😭
add'l issues/notes:
/map <map>lists variants not loaded due to version restrictions- it might be worthwhile to note somewhere in the docs that
max-server-version="1.8"means "limit to 1.8.0", not "limit to 1.8.8" (depending on how common the belief of "max-server-version="1.8"includes1.8.8servers" is going to get)
platform/platform-sportpaper/src/main/java/tc/oc/pgm/platform/sportpaper/SpMiscUtil.java
Outdated
Show resolved
Hide resolved
platform/platform-sportpaper/src/main/java/tc/oc/pgm/platform/sportpaper/SpMiscUtil.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Pablo Herrera <[email protected]>
|
Variants that can't load will now appear in red and have an explicit hover message |
Signed-off-by: Pablo Herrera <[email protected]>
|
There's one pending thing and it's unused node detection can mark the attributes on the root as unused for the variants. How exactly we tackle unused node detection in variants is something i want to give more thought and a more final solution later down the road, so it'll be ignored for now. |
Pablete1234
changed the title
| info.getId(), info, (m1, m2) -> m2.getVersion().isOlderThan(m1.getVersion()) ? m1 : m2); | ||
| // Only if from a supported version, add it to our library | ||
| if (info.isServerSupported()) { | ||
| maps.merge( |
There was a problem hiding this comment.
Will this register as a failure and should it? I feel like there would be some value in logging it specifically as a version incompatibility. I feel like people might be confused if we don't log anything.
There was a problem hiding this comment.
It will simply skip maps that are not for the current server version. It is intended as you may maintain a repo that contains both 1.8 and 1.21 maps, and simply when running in 1.8 the 1.21 maps are skipped and all is good
I'm not sure about logging as it'd likely lead to log-spam that is not really serving any use, as in, whoever set those maps knows what they're doing
Signed-off-by: Pablo Herrera <[email protected]> Signed-off-by: TTtie <[email protected]> Co-authored-by: TTtie <[email protected]>
The map itself can have
min-server-versionandmax-server-versionand individual variants can also override thisSlugs
Currently slugs are broken in variants, as one variant can't know the slug of other variants, since it's not part of the XML it sees (part of conditionals)
This PR fixes that by adding slug as an attribute in variants:
If a slug is defined in the variant, that slug is used, otherwise:
For variants
override=true: the slug defaults to the variant name slugifiedFor variants
override=false& no default<slug>: the slug will be the full variant name slugifiedFor variants
override=false& and default<slug>: the slug will bedefaultslug + "_" + variantIdThis aligns closely with what the behavior would usually be if it went thru the old rules, but won't always be the same