-
-
Notifications
You must be signed in to change notification settings - Fork 356
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
Distance option to prevent new towns to block old towns expansion. #7364
base: master
Are you sure you want to change the base?
Distance option to prevent new towns to block old towns expansion. #7364
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.
I was able to do a bit of a review, found some changes that should be made. I should be able to do a second review after the requested changes are made. Overall it looks decent.
Towny/src/main/java/com/palmergames/bukkit/towny/object/TownyWorld.java
Outdated
Show resolved
Hide resolved
Towny/src/main/java/com/palmergames/bukkit/towny/object/TownyWorld.java
Outdated
Show resolved
Hide resolved
Towny/src/main/java/com/palmergames/bukkit/towny/object/TownyWorld.java
Outdated
Show resolved
Hide resolved
Towny/src/main/java/com/palmergames/bukkit/towny/object/TownyWorld.java
Outdated
Show resolved
Hide resolved
Towny/src/main/java/com/palmergames/bukkit/config/ConfigNodes.java
Outdated
Show resolved
Hide resolved
Towny/src/main/java/com/palmergames/bukkit/config/ConfigNodes.java
Outdated
Show resolved
Hide resolved
Towny/src/main/java/com/palmergames/bukkit/config/ConfigNodes.java
Outdated
Show resolved
Hide resolved
…java Co-authored-by: LlmDl <[email protected]>
…java Co-authored-by: LlmDl <[email protected]>
…java Co-authored-by: LlmDl <[email protected]>
- Inverse the logic of the predicate to isIgnorableTown. - Move public methods upper than the private one. - Move predicate test - Remove a non necessary anymore ==null test.
…thub.com:HydrolienF/Towny into min_plot_distance_from_town_plot_for_newest_town
Thanks for reviewing step by step and helping me doing a better pull request. |
are blocked. Extracts a few methods to keep things more readable.
I've gotten this about to the point that I'm happy with it. There's a few things I don't really like. Namely, that we're essentially running the distance checks two times every time we test the distance. We are also currently testing the distance rules about twice every time there is claiming already. Once when the claiming selection is made, and then (somewhat redundantly,) checking it again in the proximity rules. So we're basically going from testing distance twice to testing it four times with this PR. @Warriorrrr what are you thoughts? |
Something that we can do to avoid calculating distance multiple time is add a new function private static boolean worldCoordNotTooCloseToOtherTowns(Town town, WorldCoord worldCoord) {
TownyWorld townyWorld = worldCoord.getTownyWorld();
return townyWorld != null
&& townyWorld.getMinDistanceFromOtherTownsPlots(worldCoord, town) >= TownySettings.getMinDistanceFromTownPlotblocks()
&& townyWorld.getMinDistanceFromOtherOlderTownsPlots(worldCoord, town) >= TownySettings.getMinDistanceFromOlderTownPlotblocks();
} whould become private static boolean worldCoordNotTooCloseToOtherTowns(Town town, WorldCoord worldCoord) {
TownyWorld townyWorld = worldCoord.getTownyWorld();
return townyWorld != null
&& townyWorld.notTooCloseToOtherTowns(worldCoord, town);
} And we will add a new function in public boolean notTooCloseToOtherTowns(Coord key, Town homeTown) {
final Map<Integer, Predicate<Town>> minDistances = Map.of(TownySettings.getMinDistanceFromTownPlotblocks(), t -> false, TownySettings.getMinDistanceFromOlderTownPlotblocks(), t -> t.getRegistered() > homeTown.getRegistered());
final int keyX = key.getX();
final int keyZ = key.getZ();
for(Town town : getTowns().values()){
if (homeTown != null && townSkippedByProximityFilter(town, homeTown, t -> false)) // No skip with the predicate here because it will be done later by the map.
continue;
for (TownBlock b : town.getTownBlocks()) {
if (!b.getWorld().equals(this)) continue;
final int tbX = b.getX();
final int tbZ = b.getZ();
if (keyX == tbX && keyZ == tbZ)
continue;
final double distSqr = MathUtil.distanceSquared((double) tbX - keyX, (double) tbZ - keyZ);
// If there is a town that is too close, return false.
if (minDistances.entrySet().stream().anyMatch(entry -> distSqr >= entry.getKey() && !entry.getValue().test(town)))
return false;
}
}
return true;
} I see this change having 3 advantages:
If @LlmDl let me know what you think about it. If you think it's a great way to do it, I can update the pull request. |
I think the direction you're planning is the way to go yes, what would be really nice is if we had test coverage applied to the distance/proximity code. Then we'd know if something breaks. |
Ok, then I will try to do a distance check function with a single for loop, link all current distance check to that function. |
Update local branch
I'm struggling with test part. Any advice on how to claim area with MockBukkit ? |
You don't want to claim stuff, you just want to test the variables work, using values you know will work/not work and passing them into the ProximityUtil methods that would decide if something works or doesn't work. |
I don't understand how to test |
For something like that you just need to make the object. Create a town and an townblock, using their constructors, then add the townblock to the town (using the same method used in the TownClaim task ie: Afterwards use the town and townblock to do the test. |
It's not working for now because townBlock.setTown(...) try to use the pugins, witch is null.
Hi @LlmDl MockBukkit.getOrCreateMock();
TownySettings.loadDefaultConfig(); This is the error: java.lang.IllegalStateException: Attempted to use getPlugin() while the plugin is null, are you shading Towny? If you do not understand this message, join the Towny discord using https://discord.com/invite/gnpVs5m and ask for support.
at com.palmergames.bukkit.towny.Towny.getPlugin(Towny.java:764)
at com.palmergames.bukkit.towny.TownyUniverse.(TownyUniverse.java:100)
at com.palmergames.bukkit.towny.TownyUniverse.getInstance(TownyUniverse.java:106)
at com.palmergames.bukkit.towny.object.TownBlock.setTown(TownBlock.java:79)
at com.palmergames.bukkit.towny.object.TownBlock.setTown(TownBlock.java:68) |
I am away on vacation for a couple more days and I'll look at this after I return. My laptop which I took with me has gotten stuck in a boot loop, and I have only got my phone (which is useless for viewing code on.) |
Description:
Add a new config option to prevent town encasement a bit better for older town.
The idea is that if a new town is created close to an older town, the old town will be able to claim more land between the 2 town.
New town could have choose an other location while the old town can't move. So it's unfair for the old town if it get claim blocked by a new town.
New Nodes/Commands/ConfigOptions:
New config option
min_plot_distance_from_older_town_plot
.The option is disabled by default and can be enabled by making
min_plot_distance_from_older_town_plot
>min_plot_distance_from_town_plot
Relevant Towny Issue ticket:
It is the feature that we talk about in #7361
By making this pull request, I represent that I have the right to waive copyright and related rights to my contribution, and agree that all copyright and related rights in my contributions are waived, and I acknowledge that the TownyAdvanced organization has the copyright to use and modify my contribution under the Towny License for perpetuity.