add mount/dismount vehicles/animals flags #426
Conversation
There was a problem hiding this comment.
Before getting into the semantics: please maintain syntactical social distancing with your braces. there's lots of missing spaces.
So, to address each major point:
-
No problems here. Obviously even the docs thought ride should apply to vehicles.
-
This seems generally ok though I struggle to see why you'd want separate mount/dismount flags. Even in your use-case it seems a bit shaky - if you can't dismount your minecart in the tunnel, why should you need to mount one? Unless you somehow get knocked out I guess? On one hand, extra granularity isn't bad, but on the other it can lead to bloat not just for the code but for people trying to mentally manage a large amount of flags. I think this is probably ok here, I just always try to evaluate whether the benefit of adding flags is as a whole more useful than not.
-
Agree with this. It's technically a behavioral breaking change but there precedent: d37f015 (I explain some motives there with chest-access).
-
This one is a bit weird. Generally I'd prefer it if "more specific" overrides "more general" but at the same time, it's a hard rule that
denyoverridesallow. This dichotomy also manifests in theuse/interactflags. Additionally, if nothing breaks, admins won't revise their flags, meaning this isn't just some temporary backstop but will define the behavior until WG gets a major rewrite (lol). Not really sure how to handle this best in the end. This is the kind of thing I'd rather have feedback on from people who actually use these flags.
| @@ -414,7 +415,15 @@ public void onUseEntity(UseEntityEvent event) { | |||
| } | |||
| /* Ridden on use */ | |||
| } else if (Entities.isRiddenOnUse(entity)) { | |||
| canUse = query.testBuild(BukkitAdapter.adapt(target), associable, combine(event, Flags.RIDE, Flags.INTERACT)); | |||
| StateFlag flagToCheck; | |||
| if (entity instanceof Tameable){ | |||
There was a problem hiding this comment.
pigs can be ridden but aren't tameable, what happens here? maybe check the Animal interface?
There was a problem hiding this comment.
Good catch - I admit I was only thinking of horses while working on this. I'll check the Animal interface instead.
| Player player = (Player) exited; | ||
| LocalPlayer localPlayer = WorldGuardPlugin.inst().wrapPlayer(player); | ||
| StateFlag flagToCheck; | ||
| if (vehicle instanceof Tameable){ |
|
I was interested in seeing dismount and mount flags separated. My usecase is minecart departure / arrival zones being separate, and attempting to dissuade people from riding the system backwards just to screw over the system / redstone. Once someone hops in the minecart, they are trapped behind a glass barrier currently, and getting out will cause them to get trapped, and probably flood the subway with empty carts. The glass barrier is there to stop them from being able to push the cart down the subway, without spamming excess carts (costing them iron). |
This PR aims to address inconsistencies and unexpected surprises with the current
rideregion flag, while maintaining backward compatibility.Prevents players from dismounting both vehicles and animals in protected regions where
rideis deniedPreviously, in regions with the
rideflag set todeny, players would be prevented from dismounting their animals inside because they cannot get back on. However, this does not apply to vehicles (e.g. minecarts), despite the documentation stating therideflag applies to both vehicles and animals. This can lead to other problems such as abandoned minecarts in rail systems clogging up the system because they cannot get back on after they exit.More granular flags for dismounting/mounting vehicles/animals
My proposed solution is to separate the animals and vehicles flags entirely, as well as decouple mounting and dismounting permissions. Examples of use:
A metro system would have two global regions: one for tunnels, and one for station platforms. Tunnels would set
dismount-vehiclestoDENYto prevent players from exiting while the cart is in transit between stations, while the platforms will havemount-vehiclesanddismount-vehiclesset toALLOW.A adventure map / pvp arena that requires players to remain mounted in their horses at all times while within the region would set
dismount-animalstoDENYwithin the arena grounds, while setting a separate staging area (the entrance) to allow them to mount and dismount freely before entering the grounds.Don't let
interactdetermine mounting/dismounting permissionsCurrently, if the
interactflag (meant for buttons and doors) is set toALLOW, users will still be able to mount/dismount from minecarts. Since we already haveride(and with my PR, more granular permissions targeting vehicles specifically), I suggest removing theinteractflag from the permissions query for vehicle entry and exit.Backward compatibility with the existing
rideflagIf this granular permissions is favourable to you, I suggest deprecating the
rideflag. For backwards compatibility, my implementation lets the existingrideflag override my new flags:dismount-vehiclesis set toDENYbutrideis set toALLOW, allow dismount;mount-vehiclesis set toDENYbutrideis set toALLOW, allow mounting.This gives server admins time to revise their region flags without their players suddenly being unable to mount or dismount vehicles or animals after upgrading.