Skip to content
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

Dynamic nation range issue #7668

Closed
MrToirol opened this issue Nov 9, 2024 · 7 comments
Closed

Dynamic nation range issue #7668

MrToirol opened this issue Nov 9, 2024 · 7 comments
Assignees
Milestone

Comments

@MrToirol
Copy link

MrToirol commented Nov 9, 2024

What steps will reproduce the problem?

(This issue is only relevant if the GNATION_SETTINGS_NATION_PROXIMITY_TO_OTHER_NATION_TOWNS config node is set.)

  1. Using the dynamic nation range, create two towns outside of the "capital range".
  2. Delete the chain of towns linking them to the "capital range". (i.e. the towns that allowed them to join in the first place by expanding the nation range)
  3. These towns will never get kicked from the nation, even during newdays.

Looking at the code, it appears that:
The ProximityUtil.closeEnoughToOtherNationTowns method only requires 1 town (which we'll name "B") in the nation to be close enough to town "A". Therefore, even if town B itself is out of range, neither will be kicked, because they will satisfy each other's requirements.
This means that a nation can have 2+ towns completely detached from its "core" nation range and they will not be kicked.

What is the expected output?

  • The towns get kicked on newday.
  • No other town can join the nation using towns A and B's dynamic range (not that important as it would get kicked next newday anyway).

Towny version

0.100.4.2

Server version

1.20.4

Please use Pastebin.com to link the following files

I cannot access those files as I am just a player, however there is no error or permission issue as it appears to be an issue in the code itself.

I can however provide the following config values:
GNATION_SETTINGS_NATION_PROXIMITY_TO_OTHER_NATION_TOWNS = 1000
GNATION_SETTINGS_NATION_PROXIMITY_TO_CAPITAL = 3500
GNATION_SETTINGS_NATION_PROXIMITY_TO_CAPITAL_CAP = 0

@MrToirol MrToirol added the bug label Nov 9, 2024
@LlmDl
Copy link
Member

LlmDl commented Nov 9, 2024

I have looked over the code and I'm not sure about your theory about B and A towns not being close enough to each other.

I'll probably get around to testing this with some smaller numbers though.

@MrToirol
Copy link
Author

MrToirol commented Nov 9, 2024

Sorry let me reword that.
If both towns are out of range of the nation (dynamic range included) but less than 1000 blocks away from each other, they will not get kicked.

Here is a visual example of the issue

@LlmDl LlmDl self-assigned this Nov 10, 2024
@LlmDl LlmDl added this to the 0.101.0.0 milestone Nov 10, 2024
@LlmDl LlmDl closed this as completed in f74b75c Nov 10, 2024
@MrToirol
Copy link
Author

Hello again,
I tested the new release (0.100.4.12) which includes the fix for this issue.
I used the following config values :

nation_proximity_to_capital_city: '15.0'
nation_proximity_to_other_nation_towns: '5.0'
absolute_distance_from_capital: '0.0'

However, I was still able to replicate the issue by creating a chain of towns and then deleting one of them, as shown in the image below:
image
Even after a newday the towns on the east side weren't kicked. (Note that I could not plant a town at this distance before creating the chain.)

I tried moving the homeblock of every town and triggering newdays, but the towns would not get kicked.

Furthermore, I cannot move the capital's homeblock. The command does not output anything, and there are no server logs or errors.

Thank you for your time

@LlmDl
Copy link
Member

LlmDl commented Nov 11, 2024

I will probably have to add something into the order the towns are searched in, going from closest to farthest. The new code works fine as when its not checking far towns first I guess.

@LlmDl
Copy link
Member

LlmDl commented Nov 12, 2024

I believe this sorts it out: https://github.com/TownyAdvanced/Towny/actions/runs/11801038567/artifacts/2177235259 if you wouldn't mind testing it on your end. It is working properly on mine.

@MrToirol
Copy link
Author

I tried different scenarios, everything is working as expected.
Thanks for resolving this so quickly!

@LlmDl
Copy link
Member

LlmDl commented Nov 12, 2024

If you like my work I'd urge you to become a sponsor, and you're welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants