✨ Extensive rework of the tpll command

[Zoriot]

This PR improves the robustness of the /tpll command and the handling of linked worlds. The primary focus was on making height calculation height map based where possible (no api calls) + non blocking, linking dynamic and improving height calculation accuracy across different world configurations. Tpll now supports proper target selectors.
Tpll forward now is handled outside of Tpll via PlayerCommandPreprocessEvent - we'll maybe remove it completly in the future.

Key Changes

• Dynamic Linked Worlds: Refactored the linked worlds logic to fetch configurations dynamically, removing hardcoded dependencies and improving flexibility.
• Enhanced Teleportation Logic: Updated the teleportation sequence to better handle height fetching, ensuring players land correctly when moving between linked worlds.
• Improved Configuration Handling: Streamlined how the plugin reads and applies settings related to world generation and coordinate transformation.
• Switched to Brigadier: So we can eloborate better parsing capabilities. We'll maybe switch to a Command Framework in the future.

Related Commits

• Improved player join handling and linked world configuration.
• Refined height fetching and teleportation logic.
• Dynamic configuration fetching for linked worlds.

Closes DEV-0045.

[SmylerMC]

The review is incomplete, I haven't had time to go through everything and another one will have to follow. I am publishing this one to give some first feedback.

Besides the code comments, I have also noticed some inconsistent indentation in multiple places that should probably be fixed.

[SmylerMC]

Trivia, non-blocking:

projection.fromGeo is eccentric and expects lon, lat

Yes... For the trivia, this is inherited from the original Terra121, which originally had a single projection which was (x, z) |-> (long := x / 100000, lat := z / 100000), hence the argument order.

[Zoriot]

Can this be changed? Or what will the long term thing? Because i don't think we should inherent a eccentric non documented Method like that forever

[SmylerMC]

Style; potentially blocking:
Indentation seems broken here.

[SmylerMC]

Bug, blocking:

Offsets are already embedded in the projection, they shouldn't be added again.
See:

TerraPlusMinus/src/main/java/de/btegermany/terraplusminus/gen/RealWorldGenerator.java

Line 82 in 58fd13b

GeographicProjection projection = new OffsetProjectionTransform(

.

[SmylerMC]

Technical debt, blocking:

Do not use TerraConnector. It was copied blindly from Sledgehammer and is really nonsensical in this context. If you look at the implementation, it will project back the geo coordinates what you just projected to Minecraft coordinates and lookup the elevation dataset directly, bypassing any chunk generator logic. This could be a huge limitation in the future (e.g. what if SmylerMC/terraminusminus#17 gets merged and the generator no longer uses the default dataset pipeline?).

Something like this would make a lot more sense and already handles all offset calculations:

private static final Random dummyRandom =  new Random();  // To be used in places that require a random but we know it doesn't matter
[...]
height = (double) terraGenerator.getBaseHeight(tpWorld, dummyRandom, (int) round(x), (int) round(z), HeightMap.WORLD_SURFACE);

World#getHighestBlockAt(int, int, HeightMap) might even potentially suffice as the javadoc for ChunkGenerator#getBaseHeight(...) hints that it may be calling it.

Possible improvement, non-blocking

Note that none of this gets rid of another fundamental issue here: this may block the server thread until HTTP requests happen to get the height data. A better approach would be to have public methods in the generator that can return a CompletableFuture so we can do truly async operations.

[SmylerMC]

Two bugs, one blocking, one non-blocking, and a few style suggestions that should not block the merge.

There are a few other things that trouble me in the TpllCommand class (but I feel are out of scope for this PR):

• there are many magic strings for confiugration entries, permissions, etc.
• the linked-world and geo-fencing logic should be extracted to other places

Since I think this was the main goal, as there been any benchmarking of the difference in performance yet? I do think it should be better now but would be curious on the details.

Also the code clean-ups and parsing improvements coming from switching to Brigadier and Terra-- are a huge glow-up and would justify merging this by themselves.

[SmylerMC]

Suggestion, non-blocking:

Since this is sent to client, I would suggest using a less technical wording (e.g. "This is not a Terraplusmins world").

[SmylerMC]

Bug, blocking:

This fails to parse the following commands:

• /tpll 02°49'52"N131°47'03"E 1000
• /tpll 02°49'52''N 131°47'03''E 1000
• /tpll 2.831111s 131.784167w 1000

In fact, anything that has a height specified is very likely to cause parseVerbatimCoordinates(args) to throws NumberFormatException.

I would suggest something along the lines of the following instead:

        args = args.trim();  // I think Brigadier takes care of that, but unsure
        try {
            // First, try parsing as if the whole arg is a latitude and longitude string,
            // letting 'parseVerbatimCoordinate' figure out how its formated.
            // If that succeeds, then the user did not specify an altitude,
            // and we will figure it out later by ourselves.
            latLng = CoordinateParseUtils.parseVerbatimCoordinates(args);
        } catch (NumberFormatException ignored) { }

        if (latLng == null) {
            // Then, if latLng is still null at this point, the whole arg was not a valid latitude longitude string.
            // So, we remove the last word assuming it is the altitude and attempt to parse it as such.
            // Then we can try parsing the rest as latitude + longitude.
            // If that still fails, then there is no hope and the command was invalid.
            String[] argParts = args.split("\\s+(?=\\S*)$");  // Split on the last group of whitespaces
            if (argParts.length == 2) {
                String latLongArg = argParts[0];
                String altitudeArg = argParts[1];
                try {
                    // The order is important here becase we use latLng's nullness to detect parsing failures
                    height = Double.parseDouble(altitudeArg);
                    latLng = CoordinateParseUtils.parseVerbatimCoordinates(latLongArg);
                } catch (NumberFormatException ignored) { }
            }
        }
        
        if (latLng == null) {
            // All parsing attempts failed :(
            sendUsageMessage(sender);
            return;
        }

[Zoriot]

Shouldn't that be part of the library? Sounds quite tedious to have to implement it in every implementation. (Like Plot-System & Satellite)

For now Ill make the changes - if you agree Ill make a issue for it.

[SmylerMC]

Your current index-based approach is very rigid and incompatible with many formats found in the wild. E.g. searching the web for "latitude longitude Paris", I find this kind of results : 48° 51' 23.81" N 2° 21' 7.999" E . I get similar formats for Los-Angeles (34° 3' 8.0460'' N 118° 14' 37.2588'' W).

These work fine in Terra++, I think it would be great if they worked as well in Terra+-.

[Zoriot]

It now alligns with T++ & seems to be fine. Thanks for noticing that

[SmylerMC]

Style non-blocking:
This execute(...) method is quite long and intricate and it might be worth it to extract some of its logic to smaller methods.

[SmylerMC]

Bug, non-blocking because it is widespread throughout the plugin:
While Terraplusminus.config is used throughout the plugin, it is inherently broken and I think we should avoid using it and aim to have it replaced with Plugin#getConfig() everywhere.

This is because reloading the plugin's configuration will cause Plugin#getConfig() to return a different object, so using the static Terraplusminus.config might return an outdated config. This can be verified in the BukkitAPI source code for JavaPlugin#reloadConfig().

[SmylerMC]

This will fail silently if the sender is not a player. It would be best to let the sender know they need to provide a target.

[Zoriot]

I get the minecraft default message:

Invalid name or UUID

Because it cannot access this command path, only the other one, and this fails because of the first argument. Should I add another path for a proper error message, or is this fine?