poorly disguised feature requests - #115
Conversation
# Conflicts: # src/main/java/org/jlortiz/playercollars/PlayerCollarsMod.java
Unpredictable tag/block/item ordering differences between the client and server meant that the server didn't always add what the client thought it was adding. This commit makes the server sync its version of the list to the client, rather than simply assuming that the client can calculate the same one itself.
Make it a little easier to specify pet-specific items
Also split PAWS_ALLOW_INTERACT into two (one for break, one for interact)
|
WOW
I strongly approve I'm just too lazy to fix it myself
Fucking brilliant omg Up to @sorgyfrog as always with forks but this is a massive step in the right direction. (Also, you get to tell people/hide from them at all costs that you worked on this mod!) |
|
I mean, if people see my pishock mod, I don't think this one is going to be much of a surprise :) I'm still going "yooo i want this thing" at random ideas, but I'm going to leave this PR as-is for now. you can check what else I've done here: ScoreUnder/PlayerCollars@score-poc...score |
|
This looks very good but I am kinda worried about the future of this project |
|
@sorgyfrog after playing this through I can say with no hesitation that this would be worthwhile to fork in. |
I'd also be willing to contribute towards a kotlin rewrite, as I find kotlin significantly nicer to work with |
|
it might be worthwhile to cherry pick commits & PR each feature individually to make it easier to review, though idk how much effort that would be on your end. |
solonovamax
left a comment
solonovamax
left a comment
There was a problem hiding this comment.
just some random comments on things
| import java.util.Optional; | ||
| import java.util.UUID; | ||
|
|
||
| public record OwnerComponent(UUID uuid, String name, Optional<UUID> owned, Optional<String> ownedName) { |
There was a problem hiding this comment.
honestly I think while you're at it, it might be worth it to just drop name & ownedName all together, as those can become out of date if the owner/owned player updates their username.
I think you'd want to do this using UserCache, but I'm not 100% sure
There was a problem hiding this comment.
Do we know for sure that the name is always present in the UserCache? this is otherwise required for tooltip rendering
There was a problem hiding this comment.
I thought UserCache had a way to fill out the profile, but looking into it a bit more it doesn't seem like it. however, a user only gets removed from the UserCache if they have not logged in for 2 months, but this is for servers. for clients, it doesn't seem like the user cache is used at all. though I'm also looking at 1.20.1 code.
If you need to look it up, you need to use either:
MinecraftSessionService#fillProfilePropertiesRealmsUtil#uuidToProfile
neither of these seem to use a file cache, however the second one uses a LoadingCache, so I'd recommend that one, though it has the overhead of doing a uuid -> string -> uuid conversion..
also, these both can make web requests, so you wouldn't want to use these on the main thread, maybe running it off-thread and falling back to just the uuid until it is retrieved.
| @@ -154,9 +175,12 @@ | |||
| public static final Item[] DOG_BOWL_ITEMS = new Item[DyeColor.values().length]; | |||
| public static final BlockEntityType<DogBowlBlock.DogBowlBlockEntity> DOG_BOWL_BLOCK_ENTITY; | |||
| public static final ItemGroup GROUP; | |||
There was a problem hiding this comment.
atp I think it's probably a good idea to just move all of these out into their own dedicated files
| /** | ||
| * Find all item slots containing collars currently equipped by the player. | ||
| */ | ||
| public static @NotNull List<SlotEntryReference> getEquippedCollars(@NotNull LivingEntity player) { | ||
| AccessoriesCapability cap = AccessoriesCapability.get(player); | ||
| if (cap == null) return Collections.emptyList(); | ||
|
|
||
| return cap.getEquipped(x -> x.isIn(PlayerCollarsMod.COLLAR_TAG)); | ||
| } | ||
|
|
||
| /** | ||
| * Get the {@link ItemStack} describing the collar worn by the player which is owned by the given owner. | ||
| * @return owned collar if present, or {@code null} | ||
| */ | ||
| public static @Nullable ItemStack getOwnedCollar(@NotNull LivingEntity player, @NotNull Entity owner) { | ||
| List<SlotEntryReference> equippedCollars = getEquippedCollars(player); | ||
| return PlayerCollarsMod.filterStacksByOwner(equippedCollars, owner.getUuid(), player.getUuid()); | ||
| } | ||
|
|
||
| /** | ||
| * Check if the player is a pet in the abstract. | ||
| * Pets are subject to pet rules (currently just the inability to pass through or manipulate invisible fences). | ||
| */ | ||
| public static boolean isPet(@NotNull LivingEntity entity) { | ||
| return !getEquippedCollars(entity).isEmpty(); | ||
| } | ||
|
|
||
| /** | ||
| * Check what level of ownership a given potential owner has over the player. | ||
| */ | ||
| public static @NotNull OwnershipLevel getOwnershipLevel(@NotNull LivingEntity player, @NotNull Entity owner) { | ||
| return getOwnershipLevel(player, getOwnedCollar(player, owner)); | ||
| } | ||
|
|
||
| /** | ||
| * Check what level of ownership the given collar represents on the player. | ||
| */ | ||
| public static @NotNull OwnershipLevel getOwnershipLevel(@NotNull LivingEntity player, @Nullable ItemStack collarStack) { | ||
| if (collarStack == null) return OwnershipLevel.NOT_OWNED; | ||
|
|
||
| OwnerComponent ownerComponent = collarStack.get(OWNER_COMPONENT_TYPE); | ||
| if (ownerComponent == null || !ownerComponent.isValidForPet(player.getUuid())) return OwnershipLevel.NOT_OWNED; | ||
| if (ownerComponent.isOwnedByContract()) return OwnershipLevel.OWNED_SIGNED; | ||
| return OwnershipLevel.OWNED; | ||
| } | ||
|
|
||
| /** | ||
| * Check if the player is owned by someone else. | ||
| * @return {@code true} if the player is an owned pet, but not owned by the given owner. | ||
| */ | ||
| public static boolean doesPetBelongToSomeoneElse(@NotNull LivingEntity player, @NotNull Entity owner) { | ||
| boolean ownedBySomeoneElse = false; | ||
| for (SlotEntryReference collar : getEquippedCollars(player)) { | ||
| OwnerComponent ownership = collar.stack().get(OWNER_COMPONENT_TYPE); | ||
| if (ownership == null) continue; | ||
| if (!ownership.isValidForPet(player.getUuid())) continue; | ||
| if (ownership.isOwnedBy(owner.getUuid())) return false; // We have a claim to ownership | ||
| ownedBySomeoneElse = true; | ||
| } | ||
| return ownedBySomeoneElse; | ||
| } | ||
|
|
||
| public static boolean renamePlayer(@NotNull LivingEntity player, @NotNull Entity owner, @NotNull Text text) { | ||
| ItemStack ownedCollar = getOwnedCollar(player, owner); | ||
| if (!getOwnershipLevel(player, ownedCollar).isOwned()) return false; | ||
|
|
||
| ownedCollar.set(NAME_TAG_COMPONENT_TYPE, text); | ||
| return true; | ||
| } | ||
|
|
||
| public static @Nullable Text getPlayerCustomName(@NotNull LivingEntity player) { | ||
| for (var collar : PlayerCollarsMod.getEquippedCollars(player)) { | ||
| var nameComponent = collar.stack().get(PlayerCollarsMod.NAME_TAG_COMPONENT_TYPE); | ||
| if (nameComponent != null) { | ||
| return nameComponent; | ||
| } | ||
| } | ||
| return null; | ||
| } |
| protected static @NotNull ItemStack makeUnenchantedItemStack(ItemStack stack) { | ||
| ItemStack is = stack.copy(); | ||
| is.remove(DataComponentTypes.ENCHANTMENT_GLINT_OVERRIDE); | ||
| is.remove(DataComponentTypes.ENCHANTMENTS); | ||
| return is; | ||
| } | ||
|
|
There was a problem hiding this comment.
I'd drop this entirely in favour of just overriding hasGlint for FootPawsItem, PawsItem, and CollarItem
| !leashplayers$holder.isAlive() | ||
| || !isAlive() | ||
| || isDisconnected() | ||
| !leashplayers$holder.isAlive() || !isAlive() || !PlayerCollarsMod.isPet(this) |
There was a problem hiding this comment.
it's implied that PlayerCollarsMod.isPet(this) is always true if leashplayers$holder, no?
There was a problem hiding this comment.
If the player slips their collar (i.e. takes it off from the accessories inventory screen) while leashed, their leash should break
| var oldHolder = getServerWorld().getPlayerByUuid(uuid); | ||
| if (isAlive() && oldHolder != null && oldHolder.isAlive()) { | ||
| leashplayers$attach(oldHolder); | ||
| } else { | ||
| leashplayers$drop(); | ||
| } |
There was a problem hiding this comment.
this can cause the leash to get attached to a player who is very far away, then causing the pet to teleport.
is that intentional?
There was a problem hiding this comment.
yes :) hehehe
just so the owner doesn't have to hang around the same spot waiting for the pet to log in if they're just being a little bratty with it.
then causing the pet to teleport
or the leash to break, depending on the game rule
| public void useOnEntity(ItemStack stack, PlayerEntity user, LivingEntity entity, Hand hand, CallbackInfoReturnable<ActionResult> cir) { | ||
| if (cir.getReturnValue() == ActionResult.PASS) { | ||
| Text text = stack.get(DataComponentTypes.CUSTOM_NAME); | ||
| if (text == null) return; | ||
| if (PlayerCollarsMod.renamePlayer(entity, user, text)) { | ||
| stack.decrement(1); | ||
| cir.setReturnValue(ActionResult.SUCCESS); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
perhaps for the nametag, additional consent could be required through showing a popup to the pet & having then accept/reject it?
| AttackEntityCallback.EVENT.register((PlayerEntity player, World world, Hand var3, Entity entity, @Nullable EntityHitResult var5) -> { | ||
| if (world.isClient) return ActionResult.PASS; | ||
| if (player.isSpectator()) return ActionResult.PASS; | ||
|
|
||
| ServerWorld sworld = (ServerWorld) world; | ||
| AccessoriesCapability cap = AccessoriesCapability.get(player); | ||
| if (cap != null && sworld.getGameRules().getBoolean(ALLOW_ATTACK_OWNER)) { | ||
| for (SlotEntryReference sr : cap.getEquipped((x) -> x.isIn(PlayerCollarsMod.COLLAR_TAG))) { | ||
| OwnerComponent owner = sr.stack().get(OWNER_COMPONENT_TYPE); | ||
| if (owner != null && owner.uuid().equals(entity.getUuid())) { | ||
| // Collared players are allowed to attack owners, but have 75% damage returned to them | ||
| player.sendMessage(Text.translatable("message.playercollars.no_attack_owner").formatted(Formatting.RED), true); | ||
| double f = player.getAttributeValue(EntityAttributes.ATTACK_DAMAGE); | ||
| f = (f - 1) * 0.75 + 1; | ||
| player.damage(sworld, player.getDamageSources().playerAttack(player), (float) Math.ceil(f)); | ||
| return ActionResult.PASS; | ||
| } | ||
| if (getOwnershipLevel(player, entity).isOwned()) { | ||
| // Collared players are allowed to attack owners, but have 75% damage returned to them | ||
| player.sendMessage(Text.translatable("message.playercollars.no_attack_owner").formatted(Formatting.RED), true); | ||
|
|
||
| if (!sworld.getGameRules().getBoolean(ALLOW_ATTACK_OWNER)) { | ||
| return ActionResult.FAIL; | ||
| } | ||
|
|
||
| double f = player.getAttributeValue(EntityAttributes.ATTACK_DAMAGE); | ||
| f = (f - 1) * 0.75 + 1; | ||
| player.damage(sworld, player.getDamageSources().playerAttack(player), (float) Math.ceil(f)); | ||
| return ActionResult.PASS; | ||
| } | ||
|
|
||
| if (entity instanceof LeashKnotEntity ke && blockLeashKnotBreak(sworld, player, ke)) return ActionResult.FAIL; | ||
| return ActionResult.PASS; | ||
| }); |
There was a problem hiding this comment.
tbh this should be moved out to a mixin simply because of the length, as it would make it a lot cleaner
tbh, if you want to cherry pick, most of the commits are split up somewhat nicely - this PR is only as large as it is because I don't want to throw a thousand changes at a repo that's going to conflict with it next time the maintainer with push access arrives, so this is more just here to say "i've done this and right now i'm hyperfixated enough that i'd do it again". |
4 participants
Contents:
Basically, I know this is probably going to conflict horrendously with #109 and if nothing else I'd like to see that get in first, but I want to put all of this on the table (as "realistic features/fixes that I have literally already written").
For the maintainer(s), rather than a full code review at this stage (unless??), I'd like to ask which of these suits the direction of the project so that I know what to keep once I inevitably have to rewrite half of this after #109 :)