-
Notifications
You must be signed in to change notification settings - Fork 5
Add local reverse-geocode database for better performance #87
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,7 @@ | ||
| package net.buildtheearth.buildteamtools.modules.navigation; | ||
|
|
||
| import com.alpsbte.alpslib.geo.rgc.RgcHandler; | ||
| import com.alpsbte.alpslib.utils.ChatHelper; | ||
| import lombok.Getter; | ||
| import net.buildtheearth.buildteamtools.BuildTeamTools; | ||
| import net.buildtheearth.buildteamtools.modules.Module; | ||
|
|
@@ -21,6 +23,15 @@ | |
| import net.buildtheearth.buildteamtools.utils.io.ConfigPaths; | ||
| import net.buildtheearth.buildteamtools.utils.io.ConfigUtil; | ||
| import org.bukkit.Bukkit; | ||
| import org.jetbrains.annotations.Nullable; | ||
|
|
||
| import java.io.File; | ||
| import java.io.FileOutputStream; | ||
| import java.net.URI; | ||
| import java.net.URL; | ||
| import java.nio.channels.Channels; | ||
| import java.nio.channels.FileChannel; | ||
| import java.nio.channels.ReadableByteChannel; | ||
|
|
||
| /** | ||
| * Manages all things related to universal tpll | ||
|
|
@@ -36,7 +47,9 @@ public class NavigationModule extends Module { | |
| private TpllComponent tpllComponent; | ||
| @Getter | ||
| private BluemapComponent bluemapComponent; | ||
|
|
||
| @Getter | ||
| @Nullable | ||
| private RgcHandler rgcHandler = null; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Quite sure that will not cause issues because we always read from main thread. also Rgc handler itself is not thread save |
||
|
|
||
| private static NavigationModule instance = null; | ||
|
|
||
|
|
@@ -60,9 +73,41 @@ public void enable() { | |
| navigatorComponent = new NavigatorComponent(); | ||
| tpllComponent = new TpllComponent(); | ||
|
|
||
| var navConfig = BuildTeamTools.getInstance().getConfig(ConfigUtil.NAVIGATION); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is deeply nested code plus a lot of if/elses, which makes it hard to read and expand in the future. It would be better to use a switch statement here and/or more seperate functions. |
||
|
|
||
| if (navConfig.getBoolean(ConfigPaths.Navigation.RGC_LOCAL_DB_ENABLED, false)) { | ||
| File rgcFile = BuildTeamTools.getInstance().getDataPath().resolve("modules/navigation").resolve(navConfig.getString(ConfigPaths.Navigation.RGC_LOCAL_DB_PATH, "bs.file")).toFile(); | ||
| ChatHelper.logDebug("Reverse Geocode local database support is enabled. Checking for local database file at: %s", rgcFile.getAbsolutePath()); | ||
| if (rgcFile.exists()) { | ||
| rgcHandler = new RgcHandler(rgcFile, BuildTeamTools.getInstance().getSLF4JLogger(), false); | ||
| } else { | ||
| BuildTeamTools.getInstance().getComponentLogger().info("Reverse Geocode local database is enabled but the file does not exist at the specified path, installing it from the configured url."); | ||
| Bukkit.getScheduler().runTaskAsynchronously(BuildTeamTools.getInstance(), () -> { | ||
| try { | ||
| if (!rgcFile.getParentFile().mkdirs()) { | ||
| BuildTeamTools.getInstance().getComponentLogger().warn("Failed to create parent directories for Reverse Geocode local database file. Make sure the plugin has the necessary permissions to create directories and files in the plugin data folder."); | ||
| } | ||
| URL url = URI.create(navConfig.getString(ConfigPaths.Navigation.RGC_LOCAL_DB_UPDATE_URL, "")).toURL(); | ||
| try (ReadableByteChannel readableByteChannel = Channels.newChannel(url.openStream())) { | ||
| try (FileOutputStream fileOutputStream = new FileOutputStream(rgcFile)) { | ||
| FileChannel fileChannel = fileOutputStream.getChannel(); | ||
| fileChannel.transferFrom(readableByteChannel, 0, Long.MAX_VALUE); | ||
| } | ||
| } | ||
| Bukkit.getScheduler().runTask(BuildTeamTools.getInstance(), () -> { | ||
| rgcHandler = new RgcHandler(rgcFile, BuildTeamTools.getInstance().getSLF4JLogger(), false); | ||
| BuildTeamTools.getInstance().getComponentLogger().info("Successfully downloaded Reverse Geocode local database and enabled local database support for Reverse Geocoding."); | ||
| }); | ||
| } catch (Exception e) { | ||
| BuildTeamTools.getInstance().getComponentLogger().error("Failed to download Reverse Geocode local database from the configured url, disabling local database support for Reverse Geocoding.", e); | ||
| navConfig.set(ConfigPaths.Navigation.RGC_LOCAL_DB_ENABLED, false); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // Check if BlueMap plugin is enabled and config allows BlueMap integration | ||
| boolean bluemapConfigEnabled = BuildTeamTools.getInstance().getConfig(ConfigUtil.NAVIGATION) | ||
| .getBoolean(ConfigPaths.Navigation.BLUEMAP_ENABLED, true); | ||
| boolean bluemapConfigEnabled = navConfig.getBoolean(ConfigPaths.Navigation.BLUEMAP_ENABLED, true); | ||
|
|
||
| if (Bukkit.getPluginManager().isPluginEnabled("BlueMap") && bluemapConfigEnabled) { | ||
| bluemapComponent = new BluemapComponent(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,16 @@ | ||
| package net.buildtheearth.buildteamtools.modules.network.api; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are using deeply nested code and a chain of if/elses here again. Use switch statements and/or seperate functions for this. |
||
|
|
||
| import com.alpsbte.alpslib.geo.AdminLevel; | ||
| import com.alpsbte.alpslib.utils.ChatHelper; | ||
| import net.buildtheearth.buildteamtools.BuildTeamTools; | ||
| import net.buildtheearth.buildteamtools.modules.navigation.NavigationModule; | ||
| import org.bukkit.Bukkit; | ||
| import org.jetbrains.annotations.NotNull; | ||
| import org.json.simple.JSONArray; | ||
| import org.json.simple.JSONObject; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.Objects; | ||
| import java.util.concurrent.CompletableFuture; | ||
|
|
||
| public class OpenStreetMapAPI extends API { | ||
|
|
@@ -16,6 +21,37 @@ | |
| */ | ||
| public static @NotNull CompletableFuture<String[]> getCountryFromLocationAsync(double @NotNull [] coordinates) { | ||
| CompletableFuture<String[]> future = new CompletableFuture<>(); | ||
|
|
||
| if (NavigationModule.getInstance().isEnabled() && NavigationModule.getInstance().getRgcHandler() != null) { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Quite sure that will not cause issues |
||
| ChatHelper.logDebug("Using custom file API to get country from location: %s, %s", coordinates[0], coordinates[1]); | ||
|
|
||
| // If we are not on main thread, schedule lookup on main thread and complete the outer future there | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Try to avoid comments (especially in the way you wrote them) like this as you should already be able to read from function names what it is supposed to do. You can of course still add a comment if something is cases where it is really necessary to do so to understand something, but here that is not the case (same for line 44). |
||
| if (!Bukkit.isPrimaryThread()) { | ||
| ChatHelper.logDebug("Not on main thread: scheduling RGC lookup on main thread..."); | ||
| Bukkit.getScheduler().runTask(BuildTeamTools.getInstance(), () -> { | ||
| try { | ||
| var rgcGeoLocation = NavigationModule.getInstance().getRgcHandler() | ||
| .locationFromCoordinates((float) coordinates[0], (float) coordinates[1]); | ||
| ChatHelper.logDebug("RGC lookup successful: %s", rgcGeoLocation); | ||
|
Check notice on line 35 in src/main/java/net/buildtheearth/buildteamtools/modules/network/api/OpenStreetMapAPI.java
|
||
| future.complete(new String[]{rgcGeoLocation.get(AdminLevel.COUNTRY), ""}); | ||
| } catch (Exception ex) { | ||
| future.completeExceptionally(ex); | ||
| } | ||
| }); | ||
| return future; | ||
| } | ||
|
|
||
| // We're on the main thread already — run synchronously | ||
| try { | ||
| var location = Objects.requireNonNull(NavigationModule.getInstance().getRgcHandler()).locationFromCoordinates((float) coordinates[0], (float) coordinates[1]); | ||
| ChatHelper.logDebug("RGC lookup successful: %s", location); | ||
|
Check notice on line 47 in src/main/java/net/buildtheearth/buildteamtools/modules/network/api/OpenStreetMapAPI.java
|
||
| future.complete(new String[]{location.get(AdminLevel.COUNTRY), ""}); | ||
| } catch (Exception ex) { | ||
| future.completeExceptionally(ex); | ||
| } | ||
| return future; | ||
| } | ||
|
|
||
| String url = "https://photon.komoot.io/reverse?lat=" + coordinates[0] + "&lon=" + coordinates[1] + "&lang=en"; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here it says "&lang=en". Did you check whether it always returns English names? Also it may be a good idea to expand the code like so that we can add more languages in the future, because of course not everyone in BTE speaks English. You do not have to implement this already, but there should at least be a class (or enum?) where you can define this easily.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about disputed areas on Earth? What names does it give back for that? This may not seem an issue, but people across the world - and thus, also in BTE - have different beliefs on what regions on Earth have what names and it may cause unnecessary discussions in-game or on our socials. I do not know the solution to this myself right now. Maybe give back multiple names for disputed areas? We could discuss this internally. |
||
|
|
||
| ChatHelper.logDebug("Requesting country from location: %s", url); | ||
|
|
||
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.
Try to avoid long method chains like this. You can use a helper or facade method for this instead.