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

Reworking protection system (test-event & move-location check) #545

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

RedstoneFuture
Copy link

@RedstoneFuture RedstoneFuture commented Jul 1, 2024

Description

With this PR I would like to suggest a different approach to checking building rights. Querying all rights plugins is currently inperformant and not completely correct due to the different scenarios (e.g. misc flags like block-build on WorldGuard; custom flags for WorldGuard & PlotSquared via add-ons; or custom features via add-ons for BentoBox). At the end, we are interested in whether the player is allowed to build at the corresponding location or not, right? So, I used a test event to check whether the player is allowed to build at the current Armorstand position and whether he is allowed to build at the target location of a Move event (if performed).

I can see you had a lot of work to support the large number of protection plugins, but the current test seems to work well. The new check may be more correct for some cases, as it simply reflects the test result and does not refer to the theoretical processes of the individual restriction plugins.

Some other notes

By movements in the Y-achse the "lower block" is checked here. This is the same behavior as normal building protections (e.g. with WorldGuard).

The only weak point of this concept is that logging systems log the test-event. For example, it's logged by CoreProtect on location without AIR, by default. I don't see any good way to get around this.

grafik

Once the restriction has been tested for a while, we can minimize the /protections folder. Only the permission check of asedit.ignoreProtection.<plugin> would then be necessary. (An OP check is not necessary, as the permission is already queried).

/src/main/java/io/github/rypofalem/armorstandeditor/protections/WorldGuardProtection.java#L48-L49

        if (player.isOp()) return true;
        if (player.hasPermission("asedit.ignoreProtection.worldGuard")) return true;

I would like to point out that I am primarily looking for the best and safest option. In this case, it also seems to be the simplest solution. Provided it works equally well with all supported plugins.


[CORE] Changes

Changes to the core of the plugin - Performance Fixes, Bug Fixes, New Features, New Permission Nodes, New Config Options etc.

  • Replacing the current permission check with creating of a BlockPlaceEvent test event (performance and restriction improvements)
  • Adding canEdit check for Move and Reverse-Move to prevent armorstands from leaving the region

[CI] Changes

Changes relating to the Continuous Integration of other Plugin APIs, Github Workflows, Issue Templates etc.


[DOC] Changes

Changes relating to plugin Documentation - See the Wiki for more info


[MISC] Changes

Changes that does not fit in the above list


Dev-Test

Tested with:

  • PaperMC 1.20.6
  • the changes based of ArmorStandEditor-Reborn v. 1.20.6-46.2
  • PlotSquared v. 7.3.8 and WorldGuard v. 7.0.9 for X, Y and Z movements of armorstands

I am not familiar with all of the supported protection systems. Please feel free to report any errors you find with other plugins.

  • I have tested this pull request for defects on a server.

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 ArmorStandEditor Project Owners have the copyright to use and modify my contribution under the ArmorStandEditor License for perpetuity.

player.getActiveItem(), player, false, player.getActiveItemHand());

// Checking build permission by test-event. (The block is generally not placed via 'callEvent()' method.)
Bukkit.getServer().getPluginManager().callEvent(placeEvent);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might need to do something here for Folia users, since I dont think calling events directly like that works in their API... Something to check and investigate I think?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Since you also support Folia, I'll have to find out about this first. I am not familiar with it.

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

Successfully merging this pull request may close these issues.

2 participants