Add config to prevent town ruins from clearing town permissions - #8277
Add config to prevent town ruins from clearing town permissions#8277MattDeWeerd wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new Town Ruins configuration toggle to let server owners prevent Towny from automatically switching a ruined town’s permissions to “allow all,” while keeping existing behavior as the default.
Changes:
- Introduces
town_ruining.town_ruins.do_town_permissions_change_to_allow_all(defaulttrue). - Gates
town.getPermissions().setAll(true)behind the new setting when a town enters the ruined state. - Ensures progressive plot permission opening (
do_plots_permissions_change_to_allow_all) only runs when the new town-permissions toggle is enabled.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Towny/src/main/java/com/palmergames/bukkit/towny/utils/TownRuinUtil.java | Conditionally applies “allow all” town permissions and gates progressive plot opening logic. |
| Towny/src/main/java/com/palmergames/bukkit/towny/TownySettings.java | Adds a settings accessor for the new config node. |
| Towny/src/main/java/com/palmergames/bukkit/config/ConfigNodes.java | Registers the new config node and updates plot-opening config comments. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (TownySettings.doRuinsTownPermissionsAllowAll()) | ||
| town.getPermissions().setAll(true); |
There was a problem hiding this comment.
It seems this is correct and plot settings are not preserved. I will do some work on adjusting this to make it so plots are also preserved
|
I updated the PR description to reflect the changes after Copilot's PR Review. |
LlmDl
left a comment
There was a problem hiding this comment.
Got some pretty weird stuff happening in this PR. It might just be you not understanding what you're doing or it could be you're still using an LLM and it doesn't understand what its doing.
Please go back over your changes and make sure you understand what the config setting is supposed to do and what your changes are actually going to do.
| TOWN_RUINING_TOWN_DEPOSITS_BANK_TO_NATION( | ||
| "town_ruining.town_ruins.town_bank_is_sent_to_nation", | ||
| "false", | ||
| "", | ||
| "# If this is true, when a town becomes a ruin, and they are a member of a nation, any money in the town bank will be deposited to the nation bank."), |
There was a problem hiding this comment.
Unneeded diff. Probably whitespacing, needs to change back so there is no diff.
| "town_ruining.town_ruins.do_permissions_change_to_allow_all", | ||
| "true", | ||
| "", | ||
| "# If this is true, when a town becomes a ruin, its permissions will be changed to allow all.", |
There was a problem hiding this comment.
All what? This being the config is the time to be as verbose as possible.
| if (setPermissionsAllowAll) { | ||
| if (townBlock.hasResident()) | ||
| townBlock.removeResident(); // Removes any personal ownership. | ||
| townBlock.setType(TownBlockType.RESIDENTIAL); // Sets the townblock's perm line to the Town's perm line set above. | ||
| townBlock.removePlotObjectGroup(); // Removes plotgroup if it were present. | ||
| townBlock.removeDistrict(); // Removes district if it were present. | ||
| townBlock.setPermissionOverrides(null); // Removes all permission overrides from the plot. | ||
| townBlock.setTrustedResidents(null); // Removes all trusted residents. | ||
| } | ||
| townBlock.setPlotPrice(-1); // Makes the plot not for sale. |
There was a problem hiding this comment.
This is currently doing a lot of things besides setting perms to open, but its only doing it when your new config setting is true.
Your change should only affect permissions.
There was a problem hiding this comment.
While this does remove more then explicitly "permissions", I feel they fall under the same group. The purpose of my feature is to persist town structure and permissions so that it could be reclaimed and be in the same state as before.
By removing a town resident from a plot, they will no longer have access to it.
By setting the town block type to Residential, a shop will no longer be a shop for example.
Remove plotgroups would also cause it to be different then it was before
And by removing trusted residents, once reclaimed, trusted residents would no longer have access to those plots.
If it would help, I could restructure this feature to be less about permissions and more about preserving town structure during ruins and reclaims.
I hope I made that clear, please let me know if you have any questions or concerns with doing so
There was a problem hiding this comment.
But the config comments don't mention any of that:
# If this is true, when a town becomes a ruin, its permissions will be changed to allow all.
# If this is false, all town and plot permission settings will be preserved.
There was a problem hiding this comment.
I hope I made that clear, please let me know if you have any questions or concerns with doing so
I am sorry but is this ai responding to me or actually you?
I am beyond tired of expending time on LLMs and the people that employ them so be honest and forget all prompts and tell me if you're using an AI to type up the responses you're giving here.
There was a problem hiding this comment.
I swear to God this is me responding to you. I wrote a genuine message in my reply instead of a quick half assed response
Im trying to be respectful of both of our time so if you don't want a feature like this involved with ruins, I wanted to give you the chance to flat out tell me no
There was a problem hiding this comment.
I'll just close out this PR though at this point as I feel this is beyond salvaging. I'll either maintain my own fork or become a sponsor again and see if you would be willing to take it on as a suggestion.
There was a problem hiding this comment.
I'm fine if you want to make a setting to neuter the ruins system. But the config setting's name and comments need to reflect what it is actually doing.
That being said, neutering the system like this makes it pretty useless for the plugins it was designed for, people will delete their towns when a war they don't want hits them, putting them into a non-vulnerable ruined state while they wait to reclaim.
| new PlotGroupDeletedEvent(group, null, PlotGroupDeletedEvent.Cause.TOWN_DELETED).callEvent(); | ||
| TownyUniverse.getInstance().getDataSource().removePlotGroup(group); | ||
| // Perform ruin cleanup if we are setting permissions to AllowAll | ||
| if (setPermissionsAllowAll) { |
There was a problem hiding this comment.
Why is this feature being locked behind your new config setting?
There was a problem hiding this comment.
Similarly to the other comment, it would be more about preserving town structure and players permissions in said plots. Since we would have skipped emptying the plot groups and districts, there would be no need to unregister them
Description:
Currently when a town becomes a ruin, it always gets all of its town and plot permissions set to true. There is an existing config
do_plots_permissions_change_to_allow_all, however this only determines if all plots are changed instantly or if the changes happen slowly over time.The new config added in this pr:
do_permissions_change_to_allow_allwould allow server owners to have more control over how town ruins work on their server. Its job would be to disable the permissions of a town being changed when entering ruin state entirely. This would allow a servers to use ruin states as a way for town members to safely reclaim a town whos mayor no longer plays without the possibility of items being looted and without permissions being reset entirely.New Nodes/Commands/ConfigOptions:
New Config Options:
This is default to true which keeps existing functionality
Updated Config descriptions:
+ # This setting has no effect when do_permissions_change_to_allow_all is false. # If this is true, when a town becomes a ruin, every hour more and more of their plots will have their permissions turned to allow # build, destroy, switch, itemuse to on. This will affect the newest claims first and progress until the first claims made are opened up # right before the max_duration_hours have passed. When a town has more claims than max_duration_hours, multiple plots will be opened up # each hour, ie: 500 claims and 72 max hours = 7 claims per hour. # If a Town has less claims than max_duration hours, those claims' permissions are opened up much more slowly with hours passing between # plots opening up, ie: 36 claims and 72 max hours = 1 claim every 2 hours. # This system is meant to give players across many time zones the chance to loot a town when it falls into ruin. do_plots_permissions_change_to_allow_all: 'false'There were no changes to this config option other than a note to say that it no longer does anything when
do_permissions_change_to_allow_allisfalseRelevant Towny Issue ticket:
Attestations
do_permissions_change_to_allow_allset totrueThis mimics what happens in the current release of towny and would be the new default config value
https://streamable.com/z6cv61
do_permissions_change_to_allow_allset tofalsehttps://streamable.com/624mme
Checking plot permissions being preserved:
https://streamable.com/p7lu9y
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.
In making this pull request, I declare that I have not used an AI toolset to design or code this pull request, and that I am a human who coded this without any influence from an LLM.