IntellectualSites / FastAsyncWorldEdit

Blazingly fast world manipulation for artists, builders and everyone else: https://www.spigotmc.org/resources/13932/
Other
584 stars 207 forks source link

//regen Does not function with negative custom seeds #1410

Closed encode42 closed 2 years ago

encode42 commented 2 years ago

Server Implementation

Purpur, Paper

Server Version

1.17.1

Describe the bug

Changing Spigot or Purpur's custom seeds (seed-igloo, seed-mansion, buried_treasure, etc.) to a negative number will cause an error when using the //regen command.

To Reproduce

  1. Set a custom seed from Spgiot or Purpur to a negative value, I mainly tested with seed-igloo
  2. Start/restart the server
  3. Select a region and run //regen

Expected behaviour

An errorless regeneration of my selection

Screenshots / Videos

No response

Error log (if applicable)

https://mclo.gs/kSSH8BN, https://paste.gg/p/anonymous/b095501b6ac74ae2bed99e7fcec970c0

Fawe Debugpaste

https://athion.net/ISPaster/paste/view/fbabcf12a8d84cb7b4a16ddf31175257

Fawe Version

FastAsyncWorldEdit version 1.17-393;806ea14

Checklist

Anything else?

This may apply to Paper's custom seed options as well, though I have not tested such. Purpur uses -1 as the "default" seed value, which makes this a common error for users of Purpur.

NotMyFault commented 2 years ago

Can't replicate on Spigot or Paper.

encode42 commented 2 years ago

Was able to reproduce on a fresh Paper server (1.17.1 build 378) with the configuration attached below.

It's a completely fresh Paper instance with the latest FAWE build. Started the server to generate the world and config files, made seed-igloo and seed-mansion negative by simply adding - before the auto-filled seed, restarted the server, and tried to run the regen command.

https://cloud.backlight.games/s/LX4Qs4pdnrFEbS3

SirYwell commented 2 years ago

Minecraft itself does not allow negative seeds there, spigot does not care about it and forces it in anyways. But that leads to a configuration state, that can not be read again. I'm not sure if we want to deal with that.

BillyGalbreath commented 2 years ago

Minecraft itself does not allow negative seeds there, spigot does not care about it and forces it in anyways. But that leads to a configuration state, that can not be read again. I'm not sure if we want to deal with that.

Negative seeds have always worked with Minecraft, and you can clearly see it works fine with regular WorldEdit's //regen command using negative seeds in spigot.yml. This issue only exists with FAWE, and can be replicated by using any negative seed in any configuration file from any Bukkit fork (spigot.yml, paper.yml, or purpur.yml)

SirYwell commented 2 years ago

I don't understand what's so difficult about looking at the minecraft code for some people, but the CODEC in StructureFeatureConfiguration clearly expects the salt (which is the "seed" for the structures) to be non-negative. And that's where it currently fails. With this patch:

diff --git a/worldedit-bukkit/adapters/adapter-1_17_1/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_17_R1_2/regen/PaperweightRegen.java b/worldedit-bukkit/adapters/adapter-1_17_1/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_17_R1_2/regen/PaperweightRegen.java
--- a/worldedit-bukkit/adapters/adapter-1_17_1/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_17_R1_2/regen/PaperweightRegen.java    (revision 6f33c5223d206b5fa7b2c68f88762c6d0d0272c9)
+++ b/worldedit-bukkit/adapters/adapter-1_17_1/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_17_R1_2/regen/PaperweightRegen.java    (date 1636700185360)
@@ -231,8 +231,13 @@
                         seed,
                         new HashSet<>()
                 )))
-                .result()
-                .orElseThrow(() -> new IllegalStateException("Unable to map GeneratorOptions"));
+                .get()
+                .map(
+                        l -> l,
+                        error -> {
+                            throw new IllegalStateException("unable to map GeneratorOptions: " + error.message());
+                        }
+                );
         LevelSettings newWorldSettings = new LevelSettings(
                 "worldeditregentempworld",
                 originalWorldData.settings.gameType(),

We even get the message unable to map GeneratorOptions: Value must be non-negative: -14357618; Value must be non-negative: -14357618; Value must be non-negative: -14357618 (idk what Mojang was doing there that it appears three times).

That said, I'm not against finding a workaround for this, but I'm not sure if it should be allowed by spigot in the first place - a configuration, that can't be encoded and decoded again feels wrong to me.

NotMyFault commented 2 years ago

I don't understand what's so difficult about looking at the minecraft code for some people, but the CODEC in StructureFeatureConfiguration clearly expects the salt (which is the "seed" for the structures) to be non-negative. And that's where it currently fails. With this patch:

diff --git a/worldedit-bukkit/adapters/adapter-1_17_1/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_17_R1_2/regen/PaperweightRegen.java b/worldedit-bukkit/adapters/adapter-1_17_1/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_17_R1_2/regen/PaperweightRegen.java
--- a/worldedit-bukkit/adapters/adapter-1_17_1/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_17_R1_2/regen/PaperweightRegen.java  (revision 6f33c5223d206b5fa7b2c68f88762c6d0d0272c9)
+++ b/worldedit-bukkit/adapters/adapter-1_17_1/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_17_R1_2/regen/PaperweightRegen.java  (date 1636700185360)
@@ -231,8 +231,13 @@
                         seed,
                         new HashSet<>()
                 )))
-                .result()
-                .orElseThrow(() -> new IllegalStateException("Unable to map GeneratorOptions"));
+                .get()
+                .map(
+                        l -> l,
+                        error -> {
+                            throw new IllegalStateException("unable to map GeneratorOptions: " + error.message());
+                        }
+                );
         LevelSettings newWorldSettings = new LevelSettings(
                 "worldeditregentempworld",
                 originalWorldData.settings.gameType(),

We even get the message unable to map GeneratorOptions: Value must be non-negative: -14357618; Value must be non-negative: -14357618; Value must be non-negative: -14357618 (idk what Mojang was doing there that it appears three times).

That said, I'm not against finding a workaround for this, but I'm not sure if it should be allowed by spigot in the first place - a configuration, that can't be encoded and decoded again feels wrong to me.

Do you have anything specific in mind yet? I'm not opposed to adding workaround, but interfacing with a solution the game doesn't allow feels wrong after all.