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

Add entity placing and breaking flags (Fixes WORLDGUARD-4080) #417

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Phoenix616
Copy link
Contributor

This implements two new flags (entity-place and entity-destroy) which allow specifying which entities can be placed or destroyed in a region even though the player does not have build rights. This resolves issue 4080.

It also displays the actual entity name when placing or destroying is cancelled instead of a general "things", fixes a comment and removes an unnecessary check for drops (which is handled before already)

@Joo200
Copy link
Collaborator

Joo200 commented Oct 15, 2019

I'm using self written plugins to override the worldguard stuff:

  • Make your custom worldguard flag and create a listener for "SpawnEntityEvent"
  • set the priority from the listener to "LOWEST" or "LOW"
  • use event.setAllowed(true); to override WorldGuard things.

Works perfectly fine for me for block place, block break, pvp and more

@Phoenix616
Copy link
Contributor Author

I realise that this can be done in an external plugin too (but so can many other things already provided by WG itself) Also doing that in an external plugin the way you describe it would introduce a good amount of unnecessary additional logic while providing it directly in the plugin itself keeps the necessary checks to a minimum.

Also adding a new flag was the suggested solution to the issue so I just did that. 😉

@wizjany
Copy link
Collaborator

wizjany commented Oct 15, 2019

so, few random thoughts:
first, while what joo says works, those events specifically state that they aren't intended to be publicly consumed and may change at any time. that said there isn't any "api-condoned" way to override WG protections atm, so this is often how it's done.
second, override flags are a relatively often requested feature, so if they are going to be added to WG I'd like them to be complete, consistent, and cover needs correctly. at the moment, I'm not really sure about the implementation of these flags. I think it would be much better to have "entity-place/destroy" mirroring "block-place/break" as state flags to handle the entity aspect of "build" protection. then, 4 more flags: "override-block/entity-place/break(destroy)" which take sets of blocktypes/entitytypes that override the state flag. this allows more granular control over who these flags apply to, since I would guess (though this would probably require market research) that these are often used where players are members of the region, but are only allowed to place certain blocks/entities (farm/mine type setups). in this setup, the state flag would be denied, and the set flag would allow (override) certain block/entity manipulation for just members.
there is a flip side here, which is that some people may want to allow most blocks to be placed (or any other change) but block specific ones. in this case, it would be much more useful for the override flag to also be able to deny placement, not just override allow it. this is currently an issue with the deny-spawn flag as there are more and more entities added to MC - there's no succinct way to block just hostile mobs, or allow only friendly ones. i've considered solutions like making an EntityTypeMask sort of thing and allowing for tags. I'm not really sure what the correct solution to override flags is atm, but this PR is not yet it.

@Phoenix616
Copy link
Contributor Author

Well tbh. I don't like the current solution either, mainly because it has limitations, some of which you pointed out already. But it works so it's enough for my future use-case for now: allow spawn egg usage in regions that have build (not mob spawning) blocked for event usage. An alternative to this approach could be to mirror the vehicle more closely and directly add an "allow spawnegg usage" flag. (but I took the opportunity of making them more general than just spawneggs)

My main issues with the current flag system which I noticed again when trying to add that are also similar to yours: The inability to properly specify the overrides to actually be able to specify each situation. And I don't think the solution can be to just add more flags to override existing ones as afaik. it would still not be possible to specify all possible combinations e.g. for members and non-members, and don't let me get into allowing specific mobs to override/bypass certain flags.

I feel like the whole flag system is missing a component that allows for the flag to know the context in which it is queried and to be able to respond to them in different ways which results in this quite unwieldy override system. An approach that is more similar to LuckPerms's and Sponge's context might be better suited in the long run.

@wizjany
Copy link
Collaborator

wizjany commented Oct 15, 2019

yea, that's essentially what sk was doing with rule lists. essentially you could make a flexible blacklist-entry-like rule that could be applied to regions. flags were just to be implemented as pre-written rules.

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.

3 participants