Skip to content
This repository was archived by the owner on Nov 25, 2019. It is now read-only.
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,10 @@ mutation.type.tools = Tools
mutation.type.tools.desc = enchanted diamond tools
mutation.type.apocalypse = Apocalypse
mutation.type.apocalypse.desc = mob spawning chaos
mutation.type.teamchest = Team Chest
mutation.type.teamchest.desc = access a shared team chest
mutation.type.teamchest.item_name = Team Chest
mutation.type.teamchest.item_lore = Right click to access an inventory shared by the team.

tnt.license.info.alreadyHas = You have a TNT license. You can surrender your license by typing {0}
tnt.license.info.doesNotHave = You do not have a TNT license. You can request one by typing {0}
Expand Down
3 changes: 2 additions & 1 deletion PGM/src/main/java/tc/oc/pgm/mutation/Mutation.java
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ public enum Mutation {
BREAD (BreadMutation.class, Material.BREAD),
BOAT (BoatMutation.class, Material.BOAT, false),
TOOLS (ToolsMutation.class, Material.DIAMOND_PICKAXE),
APOCALYPSE (ApocalypseMutation.class, Material.NETHER_STAR);
APOCALYPSE (ApocalypseMutation.class, Material.NETHER_STAR),
TEAMCHEST (TeamChestMutation.class, Material.ENDER_CHEST);

public static final String TYPE_KEY = "mutation.type.";
public static final String DESCRIPTION_KEY = ".desc";
Expand Down
135 changes: 135 additions & 0 deletions PGM/src/main/java/tc/oc/pgm/mutation/types/kit/TeamChestMutation.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
package tc.oc.pgm.mutation.types.kit;

import org.bukkit.ChatColor;
import org.bukkit.Material;
import org.bukkit.entity.Player;
import org.bukkit.event.EventHandler;
import org.bukkit.event.EventPriority;
import org.bukkit.event.block.Action;
import org.bukkit.event.inventory.InventoryAction;
import org.bukkit.event.inventory.InventoryClickEvent;
import org.bukkit.event.inventory.InventoryType;
import org.bukkit.event.player.PlayerInteractEvent;
import org.bukkit.inventory.EquipmentSlot;
import org.bukkit.inventory.Inventory;
import org.bukkit.inventory.ItemStack;
import tc.oc.commons.bukkit.inventory.Slot;
import tc.oc.commons.bukkit.item.ItemBuilder;
import tc.oc.pgm.PGMTranslations;
import tc.oc.pgm.kits.ItemKit;
import tc.oc.pgm.kits.Kit;
import tc.oc.pgm.kits.SlotItemKit;
import tc.oc.pgm.match.Match;
import tc.oc.pgm.match.MatchPlayer;
import tc.oc.pgm.match.Party;
import tc.oc.pgm.mutation.types.KitMutation;
import tc.oc.pgm.wool.WoolMatchModule;

import java.util.*;

Choose a reason for hiding this comment

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

No wildcard imports please


public class TeamChestMutation extends KitMutation {
final static int SLOT_ID = 17; // Top right
final static Material TOOL_TYPE = Material.ENDER_CHEST;

Choose a reason for hiding this comment

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

Do we need this extra field?

Copy link
Author

Choose a reason for hiding this comment

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

Because it is used six times, I believe so. So in the future it may be easy to change it if needed.

final static String ITEM_NAME_KEY = "mutation.type.teamchest.item_name";

Choose a reason for hiding this comment

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

The translation strings don't need to be statically defined

final static String ITEM_LORE_KEY = "mutation.type.teamchest.item_lore";
final static int CHEST_SIZE = 27;

final Map<Party, Inventory> teamChests = new WeakHashMap<>();

Choose a reason for hiding this comment

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

Even though this is weak, you want to clear this disable()


final Optional<WoolMatchModule> oWmm;

Choose a reason for hiding this comment

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

Just call it wools or optWools, oWmm is ambiguous.


public TeamChestMutation(Match match) {
super(match, false);
oWmm = Optional.ofNullable(match().getMatchModule(WoolMatchModule.class));

Choose a reason for hiding this comment

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

Match::getMatchModule is an @nullable. Is there a need for an Optional?

Choose a reason for hiding this comment

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

If you want the optional use match().module(...)

for (Party party : match().getParties()) {

Choose a reason for hiding this comment

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

Mutation modules are special, because they can be enabled or disabled on the fly. So this logic should go into

@Override
public boolean enable() {
    super.enable();
    for (Party party : match().getParties()) {
         // Rest of implementation
    }
}

You will also have to make sure you implement disable() too.

if (party.isParticipatingType()) {
// Could the chest title be localized properly?

Choose a reason for hiding this comment

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

Yes it can, but you'll have to create an inventory view each time a player wishes to view it. See NavigatorInterface.

Choose a reason for hiding this comment

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

Make this a TODO

teamChests.put(party, match().getServer().createInventory(null, CHEST_SIZE));
}
}
}

@Override
public void kits(MatchPlayer player, List<Kit> kits) {
super.kits(player, kits);
kits.add(getKitForPlayer(player));
}

// Open shared inventory instead of placing the chest
@EventHandler(priority = EventPriority.HIGHEST)

Choose a reason for hiding this comment

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

Add to the annotation ignoreCancelled = true

public void onChestUse(PlayerInteractEvent event) {

Choose a reason for hiding this comment

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

Rule of thumb for player events, very early in your event block you want to convert to a MatchPlayer and depending on the event a Participant.

MatchPlayer player = match.participant(event.getPlayer());
if(!player.isPresent() ||
   !(event.getAction() == Action.RIGHT_CLICK_AIR || event.getAction() == Action.RIGHT_CLICK_BLOCK) ||
   event.getItem() == null || event.getItem().getType() != Material.ENDER_CHEST) {
       return;
}

if (event.getItem() == null) return;

if (!(event.getAction() == Action.RIGHT_CLICK_AIR || event.getAction() == Action.RIGHT_CLICK_BLOCK)) return;
if (event.getItem().getType() != TOOL_TYPE) return;

Player bukkitPlayer = event.getPlayer();
if (bukkitPlayer == null) return;

Choose a reason for hiding this comment

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

Will this actually happen?

Copy link
Author

@DirkyJerky DirkyJerky Jun 9, 2018

Choose a reason for hiding this comment

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

I don't know. I've seen some weird things with Bukkit events returning null when they shouldn't be, better safe than sorry.

Optional<Inventory> oTeamInventory = getTeamsInventory(bukkitPlayer);

Choose a reason for hiding this comment

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

If you want to prefix your optional variables use opt(...), so optTeamInv etc..


oTeamInventory.ifPresent(teamInventory -> {
event.setCancelled(true);
// If the item is in the off-hand slot, it wont get put back visually for the player without this.
if(event.getHand() == EquipmentSlot.OFF_HAND) event.getActor().updateInventory();
bukkitPlayer.openInventory(teamInventory);

Choose a reason for hiding this comment

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

Unnecessary newline

});
}

@EventHandler(priority = EventPriority.LOWEST)
public void onInventoryClick(InventoryClickEvent event) {
if (event.getCurrentItem() == null) return;

// No putting evil items (ender chest, possibly wool) into the chest
Optional<Inventory> teamChest = getTeamsInventory(event.getActor());
if (teamChest.filter(teamInventory -> {
return teamInventory.equals(event.getView().getTopInventory());

Choose a reason for hiding this comment

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

Single line lambdas don't need the {} this whole block can be simplified:

if(getTeamsInventory(event.getActor()).filter(i -> i.equals(event.getView().getTopInventory())).isPresent() &&
   isBlacklistedItem(event.getCurrentItem()) {
      event.setCancelled(true);
      return;
}

}).isPresent() &&
isItemEvil(event.getCurrentItem())) {
event.setCancelled(true);
return;
}

// If normal right click, in their inventory, on the chest, then open shared inventory.
getTeamsInventory(event.getActor()).ifPresent(teamInventory -> {

Choose a reason for hiding this comment

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

DRY, can we set a variable for event.getActor()?

if (event.getInventory().getType() == InventoryType.CRAFTING &&
event.getCurrentItem().getType() == TOOL_TYPE &&
event.getAction() == InventoryAction.PICKUP_HALF) {
event.setCancelled(true);
// This resets their mouse position annoyingly, but without it items can get stuck in places.
event.getActor().closeInventory();

Choose a reason for hiding this comment

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

Since you use event.getActor() several times, you want to store that variable.

// Prevent visual inconsistencies, specifically off-hand slot
if (event.getSlotType() == InventoryType.SlotType.QUICKBAR) {
event.getActor().updateInventory();
}
event.getActor().openInventory(teamInventory);
}
});
}

private boolean isItemEvil(ItemStack item) {

Choose a reason for hiding this comment

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

isBlacklistedItem

if(item.getType() == TOOL_TYPE) return true;
if(oWmm.filter(wmm -> wmm.isObjectiveWool(item)).isPresent()) return true;

Choose a reason for hiding this comment

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

Be careful here. If isObjectiveWool is false, you are still returning true. Your method can also be simplified.

private boolean isBlacklistedItem(ItemStack item) {
   return item.getType() == Material.ENDER_CHEST || wools.map(w -> w.isObjectiveWool(item)).orElse(false);
}


return false;
}

private Optional<Inventory> getTeamsInventory(Player bukkitPlayer) {

Choose a reason for hiding this comment

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

For our needs, would an @nullable possibly be better?

Choose a reason for hiding this comment

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

Can we take in a MatchPlayer instead of a bukkit Player object?

Choose a reason for hiding this comment

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

Optional's are preferred over null values

Choose a reason for hiding this comment

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

Use Optionals if it makes for cleaner code, if it makes things messier then you don't need to.

MatchPlayer player = match().getPlayer(bukkitPlayer);
Party team = player.getParty();

if (!team.isParticipating()) return Optional.empty();

Choose a reason for hiding this comment

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

Instead of a team check, you can check the player.
MatchPlayer player = match().participant(bukkitPlayer)

If they aren't participating, it will be optional absent.


return Optional.of(teamChests.get(team));

Choose a reason for hiding this comment

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

Can teamChests.get(team) be null? If so you want Optional.ofNullable or else a runtime error will be thrown.

Copy link
Author

@DirkyJerky DirkyJerky Jun 9, 2018

Choose a reason for hiding this comment

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

"""
I don't think you need to throw an exception if a chest isn't found, can't think of any cases where that should happen.
"""
--- Shiny

}

private Kit getKitForPlayer(MatchPlayer player) {

Choose a reason for hiding this comment

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

PGMTranslations is legacy. See RenderedItemBuilder.

ItemStack stack = new ItemBuilder(item(TOOL_TYPE))
.name(ChatColor.DARK_PURPLE + PGMTranslations.t(ITEM_NAME_KEY, player))
.lore(ChatColor.DARK_AQUA + PGMTranslations.t(ITEM_LORE_KEY, player))
.get();

ItemKit kit = new SlotItemKit(stack, Slot.Player.forIndex(SLOT_ID));

Choose a reason for hiding this comment

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

You don't need the static variable but add a type comment here for what the 17 means.

return kit;
}
}
2 changes: 1 addition & 1 deletion PGM/src/main/java/tc/oc/pgm/wool/WoolMatchModule.java
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ public class WoolMatchModule extends MatchModule implements Listener {
.collect(Collectors.toImmutableList());
}

private boolean isObjectiveWool(ItemStack stack) {
public boolean isObjectiveWool(ItemStack stack) {
if(stack.getType() == Material.WOOL) {
for(MonumentWool wool : this.wools) {
if(wool.getDefinition().isObjectiveWool(stack)) return true;
Expand Down