[MEGA THREAD] Get Your Code Reviewed or Review Others

[rancid fern]

If you think your old code is garbage that means you've learned something new

[night knoll]

🥳!!!

[tidal basin]

Hello, does it seems optimized ? I check the ItemDisplay entities diff between old tick and actual tick and trigger a method while specifying the removed ItemDisplay```java
final LinkedList<ItemDisplay>[] entitiesMap = new LinkedList[]{new LinkedList<>()};
getScheduler().runTaskTimer(() -> {
LinkedList<ItemDisplay> actualEntitiesMap = new LinkedList<>();
for (World world : Bukkit.getWorlds()) {
actualEntitiesMap.addAll(world.getEntitiesByClass(ItemDisplay.class));
}

if(entitiesMap[0] == null) {
    entitiesMap[0] = actualEntitiesMap;
}

entitiesMap[0].removeAll(actualEntitiesMap);

for (ItemDisplay itemDisplay : entitiesMap[0]) {
    CustomBlockSuppressionListener.onCustomBlockDeath(itemDisplay);
}

entitiesMap[0] = actualEntitiesMap;

}, 1L, 1L);```

[rancid fern]

So what is the goal of that code

I missed the covo earlier

Why do you need to remove them like that

[tidal basin]

make an equivalent of EntityRemoveEvent for before 1.20.4, and just for Item displays

[tidal basin]

because I want to make custom blocks, I've got an internal list acting like cache instead of fetching every ItemDisplay and checking their PDC everytime

[rancid fern]

Are you planning on tracking all displays or just your own

[tidal basin]

just mine

I add an additional check ?

[rancid fern]

Then use weak references and a removeIf

[tidal basin]

weak references instead of this ugly [0] ?

[rancid fern]

I meant as part of the data type

Use a WeakHashSet

[tidal basin]

wdym by using removeIf ?

[tidal basin]

okay

[tidal basin]

I see what the method does

[rancid fern]

You can check isValid

[tidal basin]

but I don't see how can I use it with two lists

[rancid fern]

If it isn't valid anymore it should be removed

[tidal basin]

it doesn't exist sadge

[rancid fern]

You need to make one from a map

[tidal basin]

I see

[tidal basin]

okay thanks

[tidal basin]

hum

final Set<ItemDisplay> entitiesMap = Collections.newSetFromMap(
        new WeakHashMap<>()
);
getScheduler().runTaskTimer(() -> {
    Set<ItemDisplay> actualEntitiesMap = new HashSet<>();
    for (World world : Bukkit.getWorlds()) {
        actualEntitiesMap.addAll(world.getEntitiesByClass(ItemDisplay.class));
    }

    // If there
    if(entitiesMap == null) {
        entitiesMap = actualEntitiesMap;
    }

    // Do the diff on the old tick ItemDisplay entities
    entitiesMap.removeAll(actualEntitiesMap);

    // Trigger the method for every removed ItemDisplay
    for (ItemDisplay itemDisplay : entitiesMap) {
        CustomBlockSuppressionListener.onCustomBlockDeath(itemDisplay);
    }

    // Replace with the "new" old tick ItemDisplay entities
    entitiesMap = actualEntitiesMap;
}, 1L, 1L);```

I can't assign entitiesMap

because it's in a lambda

[rancid fern]

No need to

Also you can get rid of most of that code now

[tidal basin]

how does it works ?

I don't understand that correctly I think

[rancid fern]

Which part?

[tidal basin]

the fact that I don't need to assign entitiesMap and that I can get rid of most of the code

[rancid fern]

Can show you tmrow

Need to get some sleep

[tidal basin]

me too

thanks for your help anyway 😄 let's see that tomorrow

see ya 👋

[rancid fern]

Gn

[steady shard]

my system regarding player visibility to be world dependent

public interface Visibility {
     default VisibilitySetter getVisibilitySetter() {
          return new VisibilitySetter(this);
     }

     Collection<UUID> visiblePlayers();
}```

public class VisibilitySetter {
    private final Visibility visibility;

    public VisibilitySetter(Visibility visibility) {
        this.visibility = visibility;
    }

    public void setVisible(Player player) {
        for (Player onlinePlayer : Bukkit.getOnlinePlayers()) {
            boolean isInWorld = visibility.visiblePlayers().contains(onlinePlayer.getUniqueId());

            if (isInWorld) {
                // Show the player who joined the match to everyone else in the match and vice versa
                onlinePlayer.showPlayer(player);
                player.showPlayer(onlinePlayer);
            } else {
                // Hide the player who joined the match from everyone else
                onlinePlayer.hidePlayer(player);
                // Hide everyone else from the player who joined the match
                player.hidePlayer(onlinePlayer);
            }
        }
    }
}```

I have a class that is stored for each minigame match I create in my server and it is tied to a world:

@Getter
public class Match implements Visibility {
    private final World world;

    public BridgeMatch(int id, Map map, TeamConfiguration teamConfiguration) {
        String matchDirectory = MatchManager.MATCHES_DIRECTORY + File.separator + "match-" + id;

        WorldCreator creator = new WorldCreator(matchDirectory);
        ...

        world = creator.createWorld();
    }


    public void addPlayer(Player player) {
        ...
        player.teleport(map.getJoinSpawnPoint().toLocation(world))
        getVisibilitySetter().setVisible(player);
        ...
    }

   public void addSpectator(Player player) {
        ...
        player.teleport(map.getJoinSpawnPoint().toLocation(world))
        getVisibilitySetter().setVisible(player);
        ...
    }

    @Override
    public Collection<UUID> visiblePlayers() {
        // return both spectators and players
        Set<UUID> allPlayers = new HashSet<>(players);
        allPlayers.addAll(spectators);
        return allPlayers;
    }
}```

I then have a class that handles code regarding my lobby world

public class LobbyWorld implements Visibility {
    public static Location SPAWN;

    public void init() {
        SPAWN = new Location(Bukkit.getWorld("lobby"), 0.5D, 50.0D, 0.5D);
    }

    public void setup(Player player, boolean serverJoin) {
        ...
        player.teleport(SPAWN);
        ...
        getVisibilitySetter().setVisible(player);
        ...
    }

    @Override
    public Collection<UUID> visiblePlayers() {
        return SPAWN.getWorld().getPlayers().stream().map(Player::getUniqueId).collect(Collectors.toSet());
    }
}```

[honest silo]

i dont really see a point in your #getVisibilitySetter method if you never override it and it doesn't seem like it'll ever need to be overridden

and there also doesn't seem to be a way to hide players once they're visible?

well actually now that i look at it again there is a way to but it seems confusing and a bit misleading

i think a better name for VisibilitySetter#setVisible would be updateVisibility

[steady shard]

good idea

it works kinda like

when a player joins server

itll run the method in the lobby world whcih will teleport them there and then run the visibility shit

and then when a player is sent to a match itll run the addplayer method and visibility will be updated

and then if they do /lobby they will run the same lobby method and be sent to the lobby with the visibility updated

[honest silo]

some other things i noticed:

• why are visible players even exposed in the first place? the only place you'd really need to know which players are visible is when updating player visibility in worlds
• the entire VisibilitySetter class doesn't make too much sense. why is this a class that the Visibility interface exposes? if you're planning on making more variations, make it an interface instead and don't make getVisibilitySelector default
• i'm not sure what the point of the Visibility interface even is. it only provides which players are visible, no other functionality. it doesn't allow you to modify which players are visible or not (which shouldn't be exposed in the first place)
• players who aren't visible don't seem to even get updated on other player's visibility?
• if this is a per-world thing, why not directly tie visibility to a world, rather than making implementors handle that? it doesn't ever make sense to have another player visible on another world, since they're invisible anyways

[steady shard]

for some reason I just really didnt like doing new VisibilitySetter(this) each time I needed to use it

[honest silo]

i would just make it a private final field

which also means you don't have to create a new instance everytime you update visibility

[steady shard]

how would I tie visibility directly to a world

[honest silo]

have a getter for the world the visibility is handling

then only update visibility for players in that world

[steady shard]

so youre saying scrap the interface

[honest silo]

honestly yeah this seems too overcomplicated

[steady shard]

and use what a static helper method

[honest silo]

have a static method somewhere (probably VisibilitySetter) that updates visibility

yeah

[steady shard]

I wanted to avoid having to define who to make visible multiple times in a class

[honest silo]

but you won't have to

[steady shard]

but I guess I could just keep the method

[honest silo]

you can keep all your code in those implementing classes

just pass visible players into the static helper method

• the world

[steady shard]

public class VisibilitySetter {
    public static void updateVisibility(Player player, Collection<UUID> visiblePlayers) {
        for (Player onlinePlayer : Bukkit.getOnlinePlayers()) {
            boolean isInWorld = visiblePlayers.contains(onlinePlayer.getUniqueId());

            if (isInWorld) {
                // Show the player who joined the match to everyone else in the match and vice versa
                onlinePlayer.showPlayer(player);
                player.showPlayer(onlinePlayer);
            } else {
                // Hide the player who joined the match from everyone else
                onlinePlayer.hidePlayer(player);
                // Hide everyone else from the player who joined the match
                player.hidePlayer(onlinePlayer);
            }
        }
    }
}```

VisibilitySetter.updateVisibility(player, visiblePlayers());

[honest silo]

yeah that's literally the only change you need to make

[steady shard]

yeah ur right

thanks

[honest silo]

actually after i look at this wouldn't it make more sense as a updateVisibility(Player player, World world)

i mean it depends on what your specific use-case is

[steady shard]

yeah but I also have

[honest silo]

ig you would need a bool too

[steady shard]

    public Collection<UUID> visiblePlayers() {
        return Bukkit.getOnlinePlayers().stream()
                .filter(player -> player.getWorld().getName().startsWith(BridgeMatchManager.MATCHES_PRESET_DIRECTORY))
                .map(Entity::getUniqueId).collect(Collectors.toList());
    }```

this for a map editor world

where you can see everyone who is editing worlds

where it could be multiple different worlds

[honest silo]

yeah so doing it that way wouldn't work

[steady shard]

but you can still see them in tab even if theyre in a different world

so I think what I have now is fine

[honest silo]

yeah

[steady shard]

thanks

I have another class similar if you wanted to take a look

[honest silo]

sure i could take a quick look

bit busy tho so dont expect a huge analysis or anything

[steady shard]

of coursew

/**
 * The ObjectiveWorld interface defines methods for setting up and managing scoreboard objectives for players.
 * It is to be implemented by classes that represent different worlds in the game.
 */
public interface ObjectiveWorld {

    /**
     * Returns an instance of ObjectiveSetter associated with this ObjectiveWorld.
     *
     * @return a new ObjectiveSetter instance
     * @example In a class that implements ObjectiveWorld:
     * ObjectiveSetter objectiveSetter = getObjectiveSetter();
     */
    default ObjectiveSetter getObjectiveSetter() {
        return new ObjectiveSetter(this);
    }

    /**
     * Sets up the scoreboard objectives for a player who joins this world.
     *
     * @param player     the player for whom the objectives are being set up
     * @param scoreboard the scoreboard to which the objectives are being added
     */
    default void setupObjectives(Player player, Scoreboard scoreboard) {
    }

    /**
     * Sets the scores for the specified objectives in the world based on the player's score to show.
     *
     * @param objectives  the set of objectives for which scores are being set
     * @param playerScore the player whose scores are visible on the objectives
     */
    default void setObjectiveScores(Set<Objective> objectives, Player playerScore) {
    }
}```

/**
 * The ObjectiveSetter class is responsible for managing and setting up scoreboard objectives for players.
 */
public class ObjectiveSetter {
    private static final Map<UUID, Scoreboard> scoreboards = new HashMap<>();

    private final ObjectiveWorld objectiveWorld;

    public ObjectiveSetter(ObjectiveWorld objectiveWorld) {
        this.objectiveWorld = objectiveWorld;
    }

    /**
     * Sets up a new scoreboard for the specified player and stores it in the scoreboards map.
     * This method should be called when a player joins the server.
     *
     * @param player the player for whom the scoreboard is being set up
     * @example When a player joins the server:
     * <p>
     * ObjectiveSetter.setup(player);
     */
    public static void addScoreboard(Player player) {
        player.setScoreboard(player.getServer().getScoreboardManager().getNewScoreboard());
        scoreboards.put(player.getUniqueId(), player.getScoreboard());
    }

    /**
     * Retrieves the scoreboard associated with the specified UUID.
     *
     * @param player the UUID of the player whose scoreboard is to be retrieved
     * @return the scoreboard associated with the UUID
     * @example Scoreboard scoreboard = ObjectiveSetter.getScoreboard(player.getUniqueId());
     */
    public static Scoreboard getScoreboard(UUID player) {
        return scoreboards.get(player);
    }

    /**
     * Removes the scoreboard associated with the specified UUID.
     * This method should be called when a player leaves the server.
     *
     * @param player the UUID of the player whose scoreboard is to be removed
     * @example When a player leaves the server:
     * <p>
     * ObjectiveSetter.removeScoreboard(player.getUniqueId());
     */
    public static void removeScoreboard(UUID player) {
        scoreboards.remove(player);
    }```

    /**
     * Sets up the objectives for the specified player by resetting old objectives and setting up new ones.
     *
     * @param player the player for whom the objectives are being set up
     * @example In a class that implements ObjectiveWorld:
     * <p>
     * getObjectiveSetter().setObjectives(player);
     */
    public void setObjectives(Player player) {
        Scoreboard scoreboard = scoreboards.get(player.getUniqueId());

        // When transferring worlds, reset all old objectives
        for (Objective objective : scoreboard.getObjectives()) {
            objective.unregister();
        }

        // And set up the new ones
        objectiveWorld.setupObjectives(player, scoreboard);
    }

    /**
     * Sets the scores on all the objectives for the specified world.
     *
     * @param world the world in which the scores are being set
     * @example In a class that implements ObjectiveWorld:
     * <p>
     * World world = Bukkit.getWorld("worldName");
     * getObjectiveSetter().setScores(world);
     */
    public void setScores(World world) {
        for (Player playerViewing : world.getPlayers()) {
            for (Player playerScore : world.getPlayers()) {
                objectiveWorld.setObjectiveScores(getScoreboard(playerViewing.getUniqueId()).getObjectives(), playerScore);
            }
        }
    }
}```

Match implements ObjectiveWorld and then
startMatch, rejoinPlayer, addSpectator

            getObjectiveSetter().setObjectives(player);
            getObjectiveSetter().setScores(world);

LobbyWorld implements ObjectiveWorld and then
setup:

        getObjectiveSetter().setObjectives(player);```

Match:

    @Override
    public void setupObjectives(Player player, Scoreboard scoreboard) {
        Objective nametag = scoreboard.registerNewObjective("nametag-health", Criterias.HEALTH);

        nametag.setDisplaySlot(DisplaySlot.BELOW_NAME);
        nametag.setDisplayName(ChatColor.RED + StringEscapeUtils.unescapeJava("❤"));

        Objective tab = scoreboard.getObjective("tab-health");

        if (tab == null) {
            tab = scoreboard.registerNewObjective("tab-health", Criterias.HEALTH);
        }

        tab.setDisplaySlot(DisplaySlot.PLAYER_LIST);
    }

    @Override
    public void setObjectiveScores(Set<Objective> objectives, Player playerScore) {
        for (Objective objective : objectives) {
            // all objectives are regarding player health
            objective.getScore(playerScore.getName()).setScore((int) playerScore.getHealth());
        }
    }```

but lobbyworld doesnt override anything

[honest silo]

when are the methods in ObjectiveWorld called?

and how does it get a Scoreboard to pass into setupObjectives?

[steady shard]

inside ObjectiveSetter,

    public void setObjectives(Player player) {
        Scoreboard scoreboard = scoreboards.get(player.getUniqueId());

        // When transferring worlds, reset all old objectives
        for (Objective objective : scoreboard.getObjectives()) {
            objective.unregister();
        }

        // And set up the new ones
        objectiveWorld.setupObjectives(player, scoreboard);
    }```

```java
    public void setScores(World world) {
        for (Player playerViewing : world.getPlayers()) {
            for (Player playerScore : world.getPlayers()) {
                objectiveWorld.setObjectiveScores(getScoreboard(playerViewing.getUniqueId()).getObjectives(), playerScore);
            }
        }
    }```

[steady shard]

Scoreboard scoreboard = scoreboards.get(player.getUniqueId()); based on the static map

when a player joins a server it will assign the player's scoreboard to a new one and save it in the static map

when they leave the server itll remove their scoreboard from the map

[honest silo]

wait so there isn't any outside code that calls any methods on ObjectiveWorld?

[steady shard]

Scoreboard scoreboard = scoreboards.get(player.getUniqueId()); is the same as Scoreboard scoreboard = player.getScoreboard();

no, only called within ObjectiveSetter

used as a sort of helper class for the interface

I may have misinterpreted the real usage of interfaces

[honest silo]

yeah interfaces should be used in cases where you want to hide implementation details

so an easy example is List<T>

[steady shard]

right

[honest silo]

you don't know what type of list it is, just that you can get, set, etc. on it

[steady shard]

so again this could all just be static

[honest silo]

which helps with being flexible, like a method that takes in a List rather than a specific type of list like ArrayList (assuming it doesn't need specific methods from ArrayList)

because you know you have that functionality and you know it will work no matter what type of list it is

[steady shard]

so right now I'm only using it as a sort of consistency between naming across classes

[honest silo]

this one is a bit harder because you need to update player scoreboards when they join the world

[steady shard]

and not really anythign else

[steady shard]

thats already a static method

    public static void addScoreboard(Player player) {
        player.setScoreboard(player.getServer().getScoreboardManager().getNewScoreboard());
        scoreboards.put(player.getUniqueId(), player.getScoreboard());
    }

infact I probably dont need that map at all

because itll just be tied to the player object as soon as I run player.setScoreboard

[honest silo]

yeah

[honest silo]

but you would have to update the objectives of that scoreboard when the player joins the world

[steady shard]

except do I make it static and have a runnable as the seocnd argument?

[honest silo]

you'd have to map world -> world manager

[steady shard]

im confused

[honest silo]

so my understanding rn is this
0. Player joins the server

1. Player gets assigned their own scoreboard
2. Player enters world
3. Player's scoreboard is cleared and its objectives are set to the specific ones of that world
4. Player leaves world and joins another
5. Repeat step 4

[steady shard]

correct

[honest silo]

what im referring to is step 4, how do you know what objectives to use?

you'd have to have some sort of way to map a world to one of your world manager classes

so you can call the necessary methods

[steady shard]

right

I mean the Match class already has a world field

and the LobbyWorld class has a static method getting its respective world which is always the same

[honest silo]

yeah so you'd just have to check against every class

and now you can use an interface if you want, something like:

public interface ScoreboardWorld {
  void updateObjectives(Player player);
}

make every world manager implement that

when a player joins a world, you get the corresponding ScoreboardWorld and call #updateObjectives

(after clearing their old scoreboard)

now the implementing classes add the necessary objectives to the player's scoreboard

[steady shard]

I think thats what I have

public interface ObjectiveWorld {
    
    /**
     * Sets up the scoreboard objectives for a player who joins this world.
     *
     * @param player     the player for whom the objectives are being set up
     * @param scoreboard the scoreboard to which the objectives are being added
     */
    default void updateObjectives(Player player, Scoreboard scoreboard) {
    }

    /**
     * Sets the scores for the specified objectives in the world based on the player's score to show.
     *
     * @param objectives  the set of objectives for which scores are being set
     * @param playerScore the player whose scores are visible on the objectives
     */
    default void updateScores(Set<Objective> objectives, Player playerScore) {
    }
}```

this is my interface

[honest silo]

right but you have way more functionality that isn't needed

[steady shard]

theyre default for worlds that dont actually have objectives so they dont need to impl it

[honest silo]

like the ObjectiveSetter class

[steady shard]

yeah

so you're saying

make it static like this?

    public static void setScores(ObjectiveWorld objectiveWorld, World world) {
        for (Player playerViewing : world.getPlayers()) {
            for (Player playerScore : world.getPlayers()) {
                Scoreboard scoreboard = playerViewing.getScoreboard();
                objectiveWorld.updateScores(scoreboard.getObjectives(), playerScore);
            }
        }
    }```

[honest silo]

remove the ObjectiveSetter class and basically combine the two methods in ObjectiveWorld

make the implementor decide how to manage the player's scoreboard

oh and remove the scoreboard arg since you can just get the scoreboard from the player

[steady shard]

scoreboard arg was so its easier to read than just passing 2 player arguments

because scoreboard arg is easily identifiable as the scoreboard thats changing and then the player must be the score thats showing

[steady shard]

I wanted to avoid calling

        Scoreboard scoreboard = player.getScoreboard();

        // When transferring worlds, reset all old objectives
        for (Objective objective : scoreboard.getObjectives()) {
            objective.unregister();
        }```
in each class that implements it though

[honest silo]

do that in your world change listener

only add objectives in that method

[steady shard]

interesting approach

[honest silo]

because the functionality of that method should just be "i am adding the necessary objectives and scores to that player's scoreboard"

[steady shard]

right

I also wanted to remove having to have this for loop in every implementation

        for (Player playerViewing : world.getPlayers()) {
            for (Player playerScore : world.getPlayers()) {
                Scoreboard scoreboard = playerViewing.getScoreboard();
                objectiveWorld.updateScores(scoreboard.getObjectives(), playerScore);
            }
        }```

but is it just something I have to deal with

[honest silo]

wait now im a bit confused, are you displaying the scoreboard of a different player to that player

like you're looping over each player twice

is it like just some sidebar that shows every player name or something?

im not sure if im understanding this fully

[steady shard]

the code with basically go from:

        getObjectiveSetter().setObjectives(player);
        getObjectiveSetter().setScores(world);```
to

    objectiveWorld.updateObjectives(player);
    for (Player playerViewing : world.getPlayers()) {
        for (Player playerScore : world.getPlayers()) {
            Scoreboard scoreboard = playerViewing.getScoreboard();
            objectiveWorld.updateScores(scoreboard.getObjectives(), playerScore);
        }
    }```

basically

each player will have their own scoreboard

[honest silo]

you should only be looping through each player once

right?

update the scoreboard of every player in that world

[steady shard]

when a player joins a match and then that match stats their scoreboard will register nametag-health and tab-health and assign their display slots
to account for the issue where itll default at 0, it will loop through every player in the world, and then get their scoreboard, and loop through every other player, and update the score

setScores(world) will loop through every scoreboard in the world and for each scoreboard itll loop through every player and set their score correctly

its the same as this I believe

        for (Scoreboard scoreboard : world.getPlayers().stream().map(Player::getScoreboard).distinct().toArray(Scoreboard[]::new)) {
            for (Player playerScore : world.getPlayers()) {
                objectiveWorld.updateScores(scoreboard.getObjectives(), playerScore);
            }
        }```

[honest silo]

this is what im thinking an implementation of ScoreboardWorld would be like

public class MyWorldManager implements ScoreboardWorld {
  private final World world;

  public MyWorldManager() {
    this.world = Bukkit.getWorld("my cool world");
  }

  @Override
  public void updateObjectives(Player player) {
    Scoreboard scoreboard = player.getScoreboard();

    // psuedo code
    Objective objective = scoreboard.addObjective("kills: " + getKills(player));
    objective.setScore(0);
  }

  public void playerKill(Player killer) {
    // psuedo code
    addKill(killer);
    killer.getScoreboard().reset();

    updateObjectives(killer);
  }
}

so then now you could have a listener on world change that gets this manager, resets the player's scoreboard, and calls #updateObjectives on that player

and then ig when they leave you reset their scoreboard

[steady shard]

mroeso health objectives automatically update when health is changed but when they are initially registered they start at 0

[honest silo]

whenever a player kills another player in that world, call #playerKill which will update the killer's scoreboard

[honest silo]

same idea with health updates, have a listener that listens to when a player gets damaged or whatever and call the corresponding method on the world manager that updates the scoreboard displaying the health of that player

[steady shard]

dont need to do that because it automatically updates in server code

[honest silo]

then that's even easier

[steady shard]

only time i need to set the objective score is when I register it

thats the only time it doesnt update

[honest silo]

oh right because you're using the actual score of the objective

rather thanusing the name

[steady shard]

when a player gets damaged the score will fix itself from being at 0

but when its first initialised, the tablist will show everyones health at 0

what I could do is when a players scoreboard is initialised, it will loop through every player in the world and set the score of that player to their health

instead of double looping

[honest silo]

or you could just set the score to the current player's health in the #updateObjectives method

and since you're showing everyone's scores it jsut propagates through all the players in the world

updating their scoreboard to reflect the new player that joined

[steady shard]

so it turns into this

    @Override
    public void updateObjectives(Player player, Scoreboard scoreboard) {
        Objective nametag = scoreboard.registerNewObjective("nametag-health", Criterias.HEALTH);

        nametag.setDisplaySlot(DisplaySlot.BELOW_NAME);
        nametag.setDisplayName(ChatColor.RED + StringEscapeUtils.unescapeJava("❤"));

        Objective tab = scoreboard.getObjective("tab-health");

        if (tab == null) {
            tab = scoreboard.registerNewObjective("tab-health", Criterias.HEALTH);
        }

        tab.setDisplaySlot(DisplaySlot.PLAYER_LIST);
        
        for (Player playerScore : world.getPlayers()) {
            nametag.getScore(playerScore.getName()).setScore((int) playerScore.getHealth());
            tab.getScore(playerScore.getName()).setScore((int) playerScore.getHealth());
        }
    }```

and no need for set score method at all

[honest silo]

yeah exactly

[steady shard]

yeah correct

[honest silo]

this is now much more intuitive too since you aren't hiding code inside ObjectiveSetter

[steady shard]

problem is on 1.8 they dont have WorldChangeEvent

[honest silo]

though i am still a bit confused why you need both a player and a scoreboard argument

[steady shard]

I dont I realised

[honest silo]

1.8 :pepe_hands:

[steady shard]

yea

[honest silo]

dont make me hit you with the

?1.8

[rich fog]

https://howoldisminecraft188.today/

[steady shard]

10th time this week

[honest silo]

lmao

[steady shard]

wait I cant find worldchangevent on javadocs anywhere

[honest silo]

not sure if that's the actual name

[steady shard]

oh its playerchangedworldevent

[honest silo]

yeah there we go

[steady shard]

oh sweet it exists on 1.8

    @EventHandler
    public void onWorldChange(PlayerChangedWorldEvent event) {
        Scoreboard scoreboard = event.getPlayer().getScoreboard();

        // When transferring worlds, reset all old objectives
        for (Objective objective : scoreboard.getObjectives()) {
            objective.unregister();
        }
    }```

even better

registerObjectives(player);

turned into this

[honest silo]

yeah it's much cleaner imo

[steady shard]

    @Override
    public void registerObjectives(Player player) {
        Scoreboard scoreboard = player.getScoreboard();
        
        Objective nametag = scoreboard.registerNewObjective("nametag-health", Criterias.HEALTH);

        nametag.setDisplaySlot(DisplaySlot.BELOW_NAME);
        nametag.setDisplayName(ChatColor.RED + StringEscapeUtils.unescapeJava("❤"));

        Objective tab = scoreboard.getObjective("tab-health");

        if (tab == null) {
            tab = scoreboard.registerNewObjective("tab-health", Criterias.HEALTH);
        }

        tab.setDisplaySlot(DisplaySlot.PLAYER_LIST);

        for (Player playerScore : world.getPlayers()) {
            nametag.getScore(playerScore.getName()).setScore((int) playerScore.getHealth());
            tab.getScore(playerScore.getName()).setScore((int) playerScore.getHealth());
        }
    }```

no I agree

and then for classes that dont even override the method because they have no objectives

I can just remove the interface implementation

cus its handled in the listener anyway

[honest silo]

yeah

[steady shard]

perfect

wait could I turn visibility into a interface again to then implement into worldchangevent

so then I wont even need to call anything regarding update visibility itll all be handled automatically as its handled in the world change event

but I would have to map world to visibility implementation somewhere

[honest silo]

just make your world managers implement both visibility and objective

then you can just use the same system for both

and yeah depending on what your use-case is you could do the same for visibility

[steady shard]

unless I stored the world id inside of the visibility interface and did some janky check every class that implemented visibility and if the world thats overriden matches the world change then run the code

[honest silo]

lowkey might be easiest to do an if chain

[steady shard]

yeah but I wanted to stay dynamic because if I'm manually defining it in the world change event whats the difference from manually using the static method in the actual class

[honest silo]

if (MyWorldManager.inWorld(player)) {
  // get the manager and do something with it
} else if (MyOtherWorldManager.inWorld(player)) {
  // same ehre
} else if (MyThirdWorldManager.inWorld(player)) {
  // also here
} //...

[steady shard]

at startup I could just get a list of all classes that implement visibility

[honest silo]

yeah i mean you could just make a hashmap that maps world uuid -> manager

[steady shard]

true

[honest silo]

nah that needs reflection bs you dont wanna do that

[honest silo]

you'd just have to make a WorldManager interface that includes the methods you need

[steady shard]

so like map.get(eventworlduuid) if not null then run setvisible(worldmanager.visibleplayers)

[honest silo]

or you could just not care and do a Map<UUID, Object>

and do an instanceof check

[honest silo]

yeah exactly

[steady shard]

why not Map<UUID, Visibility>

[honest silo]

if its not null and it implements visibility

[steady shard]

I already use reflection for my listener loading and my own command system so

[honest silo]

because then you'd have 2 maps, one for visibility and one for objectives

[steady shard]

and it runs in the ctor so theres no real performance loss

[honest silo]

yeah but besides some specific use-cases it's usually not recommended to use reflection

[steady shard]

I dont need a map for objectives

[honest silo]

oh right dont you already have a system for that?

[steady shard]

cus the code will always be

        for (Objective objective : scoreboard.getObjectives()) {
            objective.unregister();
        }```
where as in visibility the code will change based on what the interface implements

[honest silo]

in that it should just be fine to do a Map<UUID, Visibility>

[steady shard]

surely I can use reflection tho

[honest silo]

reflection is extremely unreadable and undebuggable

[steady shard]

this is my current reflection usage

        // command reflection loading
        Reflections reflections = new Reflections(Command.class.getPackage().getName());

        commandClasses = reflections.getSubTypesOf(Command.class);

        subCommandTypes = reflections.getTypesAnnotatedWith(SubCommandInfo.class);

        Field bukkitCommandMap;
        try {
            bukkitCommandMap = Bukkit.getServer().getClass().getDeclaredField("commandMap");
        } catch (NoSuchFieldException e) {
            throw new RuntimeException(e);
        }

        bukkitCommandMap.setAccessible(true);

        try {
            commandMap = (CommandMap) bukkitCommandMap.get(Bukkit.getServer());
        } catch (IllegalAccessException e) {
            throw new RuntimeException(e);
        }

        initCommands();

        // listener reflection loading
        Reflections listenerReflections = new Reflections(
                ChatListener.class.getPackage().getName(),
                VoidCheckListener.class.getPackage().getName()
        );
        listenerClasses = listenerReflections.getSubTypesOf(Listener.class);```

[honest silo]

yeah i do that too

i come from a js background so ive engrained in my mind the usage of require(...) inside a loop

[steady shard]

wtf is require

[honest silo]

import but

you can use it anywhere

[steady shard]

interesting

[honest silo]

but doing that in java doing that leads to unreadable and unpredictable code

like registering all listeners in a package or something

[steady shard]

honestly I think id personally prefer reflection over a pre defined hashmap

[honest silo]

well we all have our preferences ig

[steady shard]

wait should I compare world with uuid

or name

[honest silo]

i will say there is a reason why literally like no other language has reflection or anything similar

[steady shard]

I came from c# so I love reflection tbh

[honest silo]

i dont think it matters since 2 worlds cant have the same name anyways

but uuid takes up less memory so

why not

[steady shard]

oh really

interesting

[honest silo]

takes up less memory in some cases*

i guess i should say

because a uuid is basically just a 64 bit (?) integer

if a string name is more than like 8 characters it'll take up more memory

[steady shard]

I see

[honest silo]

depends on what characters because utf-8 is weird like that

[steady shard]

well thank u for the advice its been really helpful

[honest silo]

yeah np

its an absolute pain in the butt to learn like inheritance and stuff especially in java but once you do you'll be able to write much better code and in much less time

in terms of maintainability and scalability

[steady shard]

        // visibility
        Set<WorldManager> worldManagers = XinCraftPlugin.get().getWorldManagers();
        UUID worldTo = event.getPlayer().getWorld().getUID();
        Player player = event.getPlayer();
        
        for (WorldManager worldManager : worldManagers) {
            if (worldManager.getWorldUUID().equals(worldTo)) {
                // handle visibility for the player
                for (Player onlinePlayer : Bukkit.getOnlinePlayers()) {
                    boolean isInWorld = worldManager.visiblePlayers().contains(onlinePlayer.getUniqueId());

                    if (isInWorld) {
                        // Show the player who joined the match to everyone else in the match and vice versa
                        onlinePlayer.showPlayer(player);
                        player.showPlayer(onlinePlayer);
                    } else {
                        // Hide the player who joined the match from everyone else
                        onlinePlayer.hidePlayer(player);
                        // Hide everyone else from the player who joined the match
                        player.hidePlayer(onlinePlayer);
                    }
                }
            }
        }```

inside the world change event

realised every world manager will handle visibility so no need to split them

public interface WorldManager {
    UUID getWorldUUID();

    Collection<UUID> visiblePlayers();
}```

LobbyWorld:

    @Override
    public UUID getWorldUUID() {
        return SPAWN.getWorld().getUID();
    }

    @Override
    public Collection<UUID> visiblePlayers() {
        return SPAWN.getWorld().getPlayers().stream().map(Player::getUniqueId).collect(Collectors.toSet());
    }```

also no need for reflection idk what I was thinking:
on enable:
worldManagers.add(lobbyWorld);
match ctor:
world = creator.createWorld();
XinCraftPlugin.get().getWorldManagers().add(this);

[honest silo]

looks good to me

only thing i'll say is that you could use a map for worlds instead of a set for faster look-up times but its not like you'll have 500 worlds anyways so it really doesn't matter

[steady shard]

is looping through a list to check for uuid the same performance wise as having a map of uuid to worldmanager and running get() and then if not null

ah yeah sweet

fuck I gotta remember to remove the world from the list once the world is unloaded

[honest silo]

its like how doing a brute-force search of something in an array list is faster than in a hashset in small sizes because hashing takes longer than just checking each element in the array list

[honest silo]

oh yeah memory leak oops

[steady shard]

okay nice ty I'm really happy with it now

[honest silo]

yeah it's looking pretty nice

[steady shard]

and worldManagers is just a private final Set<WorldManager> worldManagers = new HashSet<>();

[honest silo]

yeah

in your main class

or a WorldManagerManager if you're feeling really spicy

[steady shard]

could I make it static

[hasty lotus]

Unrelated, but I think you can do this with Guava without having to import Reflections dependency

ClassPath

[honest silo]

nah that would be static abuse

[steady shard]

I don’t understand the bad thing about making stuff static

if it’s per server why can’t it be static

[honest silo]

There are plenty of articles online about this but I’ll just give the quick rundown:

• not the purpose of static
• global state = bad
• code unpredictability
• very untestable
• does not fair well with server reloads (though that isn’t supported so may or may not be applicable in your case)
• good practice
• di/singleton patterns are much more flexible

The only places you should have static fields is in immutable final constants

Unless it’s a special case

Oh and also less code coupling

[steady shard]

so it’s better to have plugin.instance.whatever

it just feels so ugly

when there’s only ever one plugin instanfe

[dense leaf]

@round fossil dm advertising?

[steady shard]

it gives me these ugly warnings everywhere

[tidal basin]

@rancid fern I'm ready !

[rancid fern]

Alr give me a moment

[dense leaf]

ready for what

[night knoll]

so i did this base for managing just UUIDs and booleans
Is there anything else i could add / optimize? except adding maybe some comments and making method names more legible

public class Manager {
    private final Map<UUID, Boolean> map = new HashMap<>();
    public void set(UUID u, boolean b){
        map.put(u, b);
    }
    public Map<UUID, Boolean> get(){
        return map;
    }
    public boolean get(Player p){
        return map.get(p.getUniqueId());
    }
    public boolean get(UUID u){
        return map.get(u);
    }
}

also ```java
public static Manager getManager(){
return manager;
}

[sly jackal]

wtf does the boolean mean

[night knoll]

wdym with that?

[dense leaf]

what

[night knoll]

do you mean why im storing a boolean?

[sly jackal]

yes

i assume it has some meaning

[night knoll]

oh shit i could do List<UUID>

[sly jackal]

like is the player hot or whatever

[dense leaf]

I didn't read the "the" at first and thought you meant "wtf does boolean mean" and got confused lmfao

[sly jackal]

lmao

[night knoll]

💀

[sly jackal]

would be something I ask though

but

whats the point of the manager

[dense leaf]

I mean tbf, what does the word actually mean

[night knoll]

who knows

[sly jackal]

its from some guy named boolean who invented boolean algebra probably

[night knoll]

we're jjust used to the word "boolean"

[dense leaf]

John Boolean

[night knoll]

????

[sly jackal]

exactly

i was close enough

[dense leaf]

Nah it was John Boolean

[sly jackal]

so bool is the best way to write it anyways

[dense leaf]

nah John Bool invented those

[sly jackal]

but stop dodging my question, whats the point of the manager?

[dense leaf]

Yeah I mean the class name really isn't descriptive

[night knoll]

sorry lol was trying to write that and also in the Boolean name thing

so i'm creating a placeholderapi expansion with some 🤮 api and im adding the player to the list whenever the api fires an event

[sly jackal]

wdym player list

how is that set up

[night knoll]

player to the list lol

mb

[sly jackal]

also a better translation would be using a set of players, not a list

and then what does the list do

[night knoll]

when placeholder is parsed is checks if the player UUID is on the list, if it is, the placeholder returns "yes", else, returns "no"

[sly jackal]

i see

and this logic is going in the manager?

[night knoll]

nope, the manager just has the list and i change the list only when an event is fired

[sly jackal]

so its a wrapper around a list?

[night knoll]

i think so

[desert ore]

With a Map<UUID, Boolean> you usually want a Set, not a List

[night knoll]

lol

[dense leaf]

idk why but I never thought of abstracting managers ```kt
abstract class Manager<A, B> {
private val inner = mutableMapOf<A, B>()
val entries get() = inner.entries

operator fun get(a: A) = inner[a]
operator fun set(a: A, b: B) { inner[a] = b }

}

object CooldownManager : Manager<UUID, Int>()

[sly jackal]

thats good to not thing about

because its borderline useless

[dense leaf]

DRY moment

[night knoll]

💀

[sly jackal]

just extend a map?

[dense leaf]

I mean, that would be exposing the mutable map directly, which is the manager

which you should not be doing

[sly jackal]

what are you on about

[night knoll]

what are you guys even talking about

im so confused

[sly jackal]

this is the same thing

whats the actual functional difference

[dense leaf]

You shouldn't be exposing mutable collections

rather immutable ones

and provide functions to modify the inner mutable one

[sly jackal]

why

whats the difference

your goal is to put stuff in the map

who cares how it got there

[night knoll]

public class Manager {
    private final Set<UUID> list = new HashSet<>();
    public void set(UUID u, boolean b){
        if (b){
            list.add(u);
            return;
        }
        list.remove(u);
    }
    public boolean get(UUID u){
        return list.contains(u);
    }
}```smth like that?

[sly jackal]

the Map is already the abstraction

[rancid fern]

So basically you can just use an iterator like the removeIf method does:

    Set<TextDisplay> trackedDisplays = Collections.newSetFromMap(new WeakHashMap<>());
    getScheduler().runTaskTimer(() -> {
        Iterator<DisplayEntity> iterator = trackedDisplays.iterator();
        while (iterator.hasNext()) {
            DisplayEntity nextDisplayEntity = iterator.next();
            if (!nextDisplayEntity.isValid()) {
                iterator.remove();
                CustomBlockSuppressionListener.onCustomBlockDeath(nextDisplayEntity);
            }
        }
    }, 1, 1);

feel free to invert that if statement and continue instead

[sly jackal]

a map is only mutable because you can set values in it using the functions

just wrapping around doesnt change anything except adding another layer on the cake of abstract paintings

like how is a Map implemented

pretty sure it uses the same kind of idea that your Manager does

so this manager is just an implementation of a map

[dense leaf]

I mean that eliminates the point of a manager, a manager is just an encapsulation over a map

[sly jackal]

exactly

[night knoll]

does anyone have a Chain of Responsibility code example?

[sly jackal]

you dont need a manager when you dont have anything to manage

[dense leaf]

"java chain of responsibility pattern example"

[night knoll]

alr ty and sorry

[sly jackal]

use a collection if you want to keep things in a collection

if there is no additional logic past the CRUD stuff, why bother writing something for it

[tidal basin]

for sure, thanks 😄

[tidal basin]

how should I add values to trackedDisplays ?

I've done it when I spawn a display, but it does nothing

wait

I might just be dumb

[pine grail]

10/10 best code I've seen, O(0) performance both in time and space complexity, completely bug free, extremely polished, well thought out and honestly state-of-the-art techniques at play here

[bronze crag]

https://pastebin.com/TtzCzMbT

*mojang thing*

[random yacht]

Might be nice to make those strings constants at the top of the file in case the endpoints ever change

And just for the sake of readability

I would also consider precompiling the pattern in that last .replaceAll() call. It's a bit lengthy and will probably be used relatively frequently enough to where precompiling is just an overall good idea. Matcher has a replaceAll() method which accomplishes the same thing, only you can reuse a Pattern constant

private static final Pattern PATTERN_UUID = Pattern.compile("(\\w{8})(\\w{4})(\\w{4})(\\w{4})(\\w{12})");

private static String getString(String jsonElement, String request) throws IOException {
    try (BufferedReader bufferReader = new BufferedReader(new InputStreamReader(new URL(request).openStream()))) {
        String foundElement = (((JsonObject) JSON_PARSER.parse(bufferReader))
                .get(jsonElement))
                .toString()
                .replace("\"", ""); // Also change this to 'replace' from 'replaceAll', the latter uses RegEx which you don't need

        return PATTERN_UUID.matcher(foundElement).replaceAll("$1-$2-$3-$4-$5");
    }
}

I believe that's the same?

[hasty lotus]

Code review : https://github.com/conclube/DeluxeAsyncJoinLeaveMessage

[round fossil]

yo you cant do that

stop

lol

[dense leaf]

10/10 commit message

[random yacht]

Thank God for all those events

Now I can have 7 different events for players joining with 6 priorities for each. That's 42 different stages!

Ignore the fact I can't math and wrote 38

[dense leaf]

Use this at hypixel

[dense leaf]

I think the only bad thing about this is that there is some badly formatted code

https://github.com/conclube/DeluxeAsyncJoinLeaveMessage/blob/59824aa714a7f6663a35b77dd695d88a3b8ef30a/src/main/java/me/conclure/wtf/FinalProUltimateDeluxeAsyncChatJoinMessagePluginImplFactoryAbstractCommonMainProviderBootstrapExecutorHandler.java#L77 Like, the { should have a space before

That's about it hto

[random yacht]

I like how the name and description are so long that it pushes your face off the embed

[woeful pasture]

no jspecify support

[late smelt]

Someone take this, remove the background and mirror it

Perfect reaction image

[alpine anvil]

@dense leaf wake up

also reminder to do that thing

[dense leaf]

I did that like 5h ago

[alpine anvil]

i left my idscord open and didnt see the ping

i forgot to turn my pc off this morning

and had this channel open

[dense leaf]

It's beautiful

[dense leaf]

well theyre 2cm

[late smelt]

That’s not quite what I meant

But I like it

[alpine anvil]

tyty

[random yacht]

Holy shit I took a quick glance at this and thought I had to ban you for a second

Thought you were a porn bot

[alpine anvil]

what do you think that is

i think you need to turn ur computer off for a bit if you think that looks like something

[dense leaf]

what 😭😭

[random yacht]

It's very vaginal in shape lol

At a quick glance

[hybrid osprey]

omegalul

[steady shard]

💀

[late smelt]

That’s clearly a dog cyclops choco

Gosh

Get your mind out of the gutter

[alpine anvil]

He never found the cliborous

[steady shard]

how can I optimise my arbitrary small structure placer

@Getter
@NoArgsConstructor
public class Cage {
    public CageBlock[][][] cageBlocks;
    @JsonIgnore
    private CageBlock[][][] cache;

    private String name;

    private int width;
    private int height;
    private int depth;

    private int yPosition;

    @JsonCreator
    public Cage(@JsonProperty("name") String name, @JsonProperty("yposition") int yPosition, @JsonProperty("width") int width, @JsonProperty("height") int height, @JsonProperty("depth") int depth) {
        cageBlocks = new CageBlock[width][height][depth];
        this.name = name;
        this.yPosition = yPosition;
        this.width = width;
        this.height = height;
        this.depth = depth;
    }


    public int size() {
        return cageBlocks.length;
    }

    public void cache(Location centreOfCage) {
        cache = new CageBlock[width][height][depth];

        Location position = centreOfCage.clone().subtract((double) width / 2, 0, (double) depth / 2);

        for (int y = 0; y < height; y++) {
            for (int z = 0; z < depth; z++) {
                for (int x = 0; x < width; x++) {
                    Block cageBlock = position.getWorld().getBlockAt(
                            position.getBlockX() + x,
                            position.getBlockY() + y,
                            position.getBlockZ() + z);
                    cache[x][y][z] = new CageBlock(cageBlock.getType(), cageBlock.getState().getData());
                }
            }
        }
    }

    public void generate(Location centreOfCage) {
        Location position = centreOfCage.clone().subtract((double) width / 2, 0, (double) depth / 2);

        boolean isFlipped = centreOfCage.getYaw() == -180;

        for (int y = 0; y < height; y++) {
            for (int z = 0; z < depth; z++) {
                for (int x = 0; x < width; x++) {
                    CageBlock cageBlock = cageBlocks[x][y][z];
                    if (cageBlock != null) {
                        int zPosition = isFlipped ? z : (depth - 1 - z);
                        setBlock(position, y, zPosition, x, cageBlock);
                    }
                }
            }
        }
    }

    public void resetToCache(Location centreOfCage) {
        Location position = centreOfCage.clone().subtract((double) width / 2, 0, (double) depth / 2);

        for (int y = 0; y < height; y++) {
            for (int z = 0; z < depth; z++) {
                for (int x = 0; x < width; x++) {
                    CageBlock cageBlock = cache[x][y][z];
                    if (cageBlock != null) {
                        setBlock(position, y, z, x, cageBlock);
                    }
                }
            }
        }
    }

    private void setBlock(Location position, int y, int z, int x, CageBlock cageBlock) {
        Block block = position.getWorld().getBlockAt(
                position.getBlockX() + x,
                position.getBlockY() + y,
                position.getBlockZ() + z
        );

        block.setType(cageBlock.getType(), false);

        BlockState state = block.getState();
        state.setData(cageBlock.getMaterialData());

        if (!state.update(true, false)) {
            throw new RuntimeException("failed to update block at " + block.getLocation());
        }
    }```

    public void saveToJson() {
        ObjectMapper objectMapper = new ObjectMapper();
        try {
            objectMapper.writeValue(new File(XinCraftPlugin.get().getDataFolder() + File.separator + "cages" + File.separator + name + ".json"), this);
        } catch (IOException e) {
            throw new RuntimeException(e);
        }
    }

    public static Cage readJson(String name) {
        try {
            return new ObjectMapper().readValue(new File(XinCraftPlugin.get().getDataFolder() + File.separator + "cages" + File.separator + name.replace("_", "-") + ".json"), Cage.class);
        } catch (IOException e) {
            throw new RuntimeException(e);
        }
    }
}```

at the start of a match it will get the cage that the player has equipped with cacheTeamCage running on each team (only ever 2 teams)

cacheTeamCage:

    private void cacheTeamCage(BridgeTeam bridgeTeam) {
        ...

        cages.put(bridgeTeam, net.xincraft.systems.match.cage.Cage.readJson(cageName));

        System.out.println("Cage cached for " + bridgeTeam + ": " + cages.get(bridgeTeam).getName());

        Location cageLocation = map.getTeamSpawnpoints().get(bridgeTeam).toTeamSpawn(world, bridgeTeam)
                .add(0, 5 - cages.get(bridgeTeam).getYPosition(), 0);

        cages.get(bridgeTeam).cache(cageLocation);
    }```

cages is just a map of team to cage

the position of the cage will never change per match, and the type of cage will never change per match

so at the start of the match I read the json and save it to a map
then I cache the existing blocks that are going to be replaced by my cage into a CageBlock[][][]

then I cage the team, which runsgenerate

and then after a 5 second timer, I run resetToCache which functions similar to worldedit's undo

CageBlock is just:

@Getter
@NoArgsConstructor
public class CageBlock {
    private Material type;
    private byte data;

    public CageBlock(Material type, MaterialData state) {
        this.type = type;
        this.data = state.getData(); // this fucking sucks, but I can't see a way around it
    }

    @Override
    public String toString() {
        return type.name().toCharArray()[0] + "";
    }

    @JsonIgnore public MaterialData getMaterialData() {
        return new MaterialData(type, data); // this fucking sucks, but I can't see a way around it
    }
}```

[hybrid osprey]

You can use Gson instead of writing these save and load methods

formatted strings instead of " + "

Records

And i dont see the point of CageBlock - if not a thin wrapper for Block

i dont see the point of having a cache

[sharp epoch]

creating a instance of ObjectMapper for each write 💀

[steady shard]

cache is just a saved array of what the blocks used to be before I generated the cage

write is never used unless testing purposes so doesn’t really matter

will move to gson tho I hate this lib

[steady shard]

yeah literally just a thin wrapper

[sharp epoch]

there's also Moshi too, if you don't like gson

[steady shard]

I believe it just lags the server a tiny bit when running generate

or resettocache

I believe due to all the light updates

[hybrid osprey]

performance wise its probably fine

code review wise, well

theres work to be done

[sharp epoch]

https://www.reddit.com/r/androiddev/comments/684flw/comment/dgx3gpm/ jesus christ Moshi is already 7 and some years old

what the fuck do you mean 2017 was 8 years ago reddit 😭

[steady shard]

is there a way to like

not call light updates or something

[sharp epoch]

just use FAWE/WE

they figured out all of this so you don't have to

block placing en masse is the one thing you want to depend on WE for

[steady shard]

that’s my only limitation

can’t use world edit

[pine grail]

LGTM. Merged.

[pine grail]

I click "scroll to recent messages" and this is now in the middle of my screen

[pine grail]

use 1D arrays over 3D arrays, [] is better than [][] which is better than [][][] for both memory and performance

also instead of using .toCharArray()[index] use .charAt(index)

[pine grail]

you can also pre-compute the material data if it will never change

[steady shard]

java 8 no records

[pine grail]

yeah realized

[steady shard]

and i would know when to change coordinate with width height depth?

[steady shard]

so precompute it into a Block array?

[pine grail]

not quite:

@Getter
@NoArgsConstructor
public class CageBlock {
    private final MaterialData data;

    public CageBlock(Material type, MaterialData state) {
        this.data = new MaterialData(type, state.getData());
    }

    @Override
    public String toString() {
        return String.valueOf(type.name().charAt(0));
    }

    @JsonIgnore public MaterialData getMaterialData() {
        return this.data;
    }
}

[pine grail]

basically:

int cageIndex = x * (height * width) + y * width + z;

[desert ore]

Maths

[pine grail]

you can get the coords back from the index, too:

int rem = index % (height * width);

int x = index / (height * width);
int y = rem / width;
int z = rem % width;

[steady shard]

I see

thank you

[pine grail]

np, you should really not use 3D arrays, search up java's memory model

basically it ends up using a ton more ram

and slows down the program because ram isn't contiguous

[desert ore]

I used to work with something similar before

Can't quite remember why and where tho

[steady shard]

would I set cageBlocks = new CageBlock[width * height * depth];?

because I got an ArrayIndexOutOfBounds

I think its linking the index of the array to the position in the world maybe

unless I'm stupid

    public void cache(Location centreOfCage) {
        cache = new CageBlock[]{}; // new CageBlock[width * height * depth];

        Location position = centreOfCage.clone().subtract((double) width / 2, 0, (double) depth / 2);

        for (int y = 0; y < height; y++) {
            for (int z = 0; z < depth; z++) {
                for (int x = 0; x < width; x++) {
                    Block cageBlock = position.getWorld().getBlockAt(
                            position.getBlockX() + x,
                            position.getBlockY() + y,
                            position.getBlockZ() + z);

                    int index = x * (height * width) + y * width + z;
                    cache[index] = new CageBlock(cageBlock.getType(), cageBlock.getState().getData());
                    System.out.println("Index: " + index + " x: " + x + " y: " + y + " z: " + z);
                }
            }
        }
    }```

java.lang.ArrayIndexOutOfBoundsException: 0
        at net.xincraft.systems.match.cage.Cage.cache(Cage.java:75) ~[?:?]

which is this here

[pine grail]

okay so you're making a zero-size array

you need to do

cache = new CageBlock[width * height * depth];

[steady shard]

I tried it with that and it did an out of bounds for like 2200 or something

[pine grail]

okay now we're getting somewhere

[steady shard]

one sec let me reproduce

[pine grail]

can you print width, height, depth and the multiplications of each

so like

width = 0, height = 0, depth = 0, width * height * depth = 0

[steady shard]

yeah one sec

I had to convert all my old cage json types to the new type

dunno if that could of fucked anything up

[pine grail]

nah it's fine

[steady shard]

[22:28:44 INFO]: Cage cached for BLUE: egg
[22:28:44 INFO]: Width: 15 Height: 13 Depth: 10 All: 1950
[22:28:44 INFO]: Width: 15 Height: 13 Depth: 10 All: 1950
[22:28:44 INFO]: Width: 15 Height: 13 Depth: 10 All: 1950
[22:28:44 INFO]: Width: 15 Height: 13 Depth: 10 All: 1950
[22:28:44 INFO]: Width: 15 Height: 13 Depth: 10 All: 1950
[22:28:44 INFO]: Width: 15 Height: 13 Depth: 10 All: 1950
[22:28:44 INFO]: Width: 15 Height: 13 Depth: 10 All: 1950
[22:28:44 INFO]: Width: 15 Height: 13 Depth: 10 All: 1950
[22:28:44 INFO]: Width: 15 Height: 13 Depth: 10 All: 1950
[22:28:44 INFO]: Width: 15 Height: 13 Depth: 10 All: 1950
[22:28:44 INFO]: Width: 15 Height: 13 Depth: 10 All: 1950
[22:28:44 WARN]: [XinCraft] Task #29 for XinCraft v0.2.3 generated an exception
java.lang.ArrayIndexOutOfBoundsException: 1950

    public void cache(Location centreOfCage) {
        cache = new CageBlock[width * height * depth];

        Location position = centreOfCage.clone().subtract((double) width / 2, 0, (double) depth / 2);

        for (int y = 0; y < height; y++) {
            for (int z = 0; z < depth; z++) {
                for (int x = 0; x < width; x++) {
                    Block cageBlock = position.getWorld().getBlockAt(
                            position.getBlockX() + x,
                            position.getBlockY() + y,
                            position.getBlockZ() + z);

                    int index = x * (height * width) + y * width + z;

                    System.out.println("Width: " + width + " Height: " + height + " Depth: " + depth + " All: " + (width * height * depth));

                    cache[index] = new CageBlock(cageBlock.getType(), cageBlock.getState().getData());
                    //System.out.println("Index: " + index + " x: " + x + " y: " + y + " z: " + z + " width: ");
                }
            }
        }
    }```

[pine grail]

what's width height and depth

15 each?

[steady shard]

all printout

so 1950

[pine grail]

ah I'm dumb

lol

well my honest suggestion

new CageBlock[width * height * depth + 1];

[steady shard]

ill give a shot

I added more logging too

[22:32:02 INFO]: Cage cached for BLUE: egg
[22:32:02 INFO]: Width: 15 Height: 13 Depth: 10 All: 1950
[22:32:02 INFO]: Index: 0 x: 0 y: 0 z: 0
[22:32:02 INFO]: Index: 195 x: 1 y: 0 z: 0
[22:32:02 INFO]: Index: 390 x: 2 y: 0 z: 0
[22:32:02 INFO]: Index: 585 x: 3 y: 0 z: 0
[22:32:02 INFO]: Index: 780 x: 4 y: 0 z: 0
[22:32:02 INFO]: Index: 975 x: 5 y: 0 z: 0
[22:32:02 INFO]: Index: 1170 x: 6 y: 0 z: 0
[22:32:02 INFO]: Index: 1365 x: 7 y: 0 z: 0
[22:32:02 INFO]: Index: 1560 x: 8 y: 0 z: 0
[22:32:02 INFO]: Index: 1755 x: 9 y: 0 z: 0
[22:32:02 INFO]: Index: 1950 x: 10 y: 0 z: 0
[22:32:02 WARN]: [XinCraft] Task #30 for XinCraft v0.2.3 generated an exception
java.lang.ArrayIndexOutOfBoundsException: 2145

[pine grail]

hm wait wait

[steady shard]

looks like its not starting at index 0 and climbing up

[pine grail]

oh I'm dumb

int index = (y * depth * width) + (z * width) + x;

lol

I forgor how to maths

[steady shard]

I hate this code because I cannot read it

like my brain genuinely cant comprehend this sort of stuff

it makes me so mad

[pine grail]

okay so lemme explain

a 3D array

is just a cube

right

[steady shard]

yea

[pine grail]

if the cube is 10m high, 10m wide and 10m long

[steady shard]

btw it worked but its fucked as lol

[pine grail]

😭

[steady shard]

let me show u

supposed to be

[pine grail]

okay idea

switch the y and z

[steady shard]

props to you tho its so much faster

[pine grail]

yeah, always remember

[] is way faster than [][]

and [][] is WAY faster than [][][]

the slowness grows exponentially the more you nest

[steady shard]

[][][][][][]

[pine grail]

lol

[steady shard]

imagine trying to comprehend a 5d array

[pine grail]

lol

[steady shard]

look its different but still fucked

[pine grail]

can you move a bit away so I can see it 😭

[steady shard]

yeah

look theres easily a chance that I fucked up the conversion

[pine grail]

where are you placing

[steady shard]

                // Convert the old cageBlocks array
                CageBlock[] newCageBlocks = new CageBlock[oldCage.getWidth() * oldCage.getHeight() * oldCage.getDepth()];
                for (int x = 0; x < oldCage.getWidth(); x++) {
                    for (int y = 0; y < oldCage.getHeight(); y++) {
                        for (int z = 0; z < oldCage.getDepth(); z++) {
                            int index = x * (oldCage.getHeight() * oldCage.getDepth()) + y * oldCage.getDepth() + z;
                            newCageBlocks[index] = oldCage.getCageBlocks()[x][y][z];
                        }
                    }
                }

                // Create a new Cage object with the converted cageBlocks array
                NewCage newCage = new NewCage(oldCage.getName(), oldCage.getYPosition(), oldCage.getWidth(), oldCage.getHeight(), oldCage.getDepth());
                newCage.cageBlocks = newCageBlocks;```

conversion code ^

[steady shard]

wdym

[pine grail]

so [][][] is still there 😭

okay basically, what is conversion

[steady shard]

nah I just used two different classes

[pine grail]

ah okay

[steady shard]

and read from old class and then converted and then saved as new class

[pine grail]

where are you placing the blocks

[steady shard]

above a predetermined position based on the map

for example

{"BLUE":{"x":0,"y":99,"z":-29},"RED":{"x":0,"y":99,"z":28}}

[pine grail]

no like

what is the code

you're using

[steady shard]

oh lol

    public void generate(Location centreOfCage) {
        Location position = centreOfCage.clone().subtract((double) width / 2, 0, (double) depth / 2);

        boolean isFlipped = centreOfCage.getYaw() == -180;

        for (int y = 0; y < height; y++) {
            for (int z = 0; z < depth; z++) {
                for (int x = 0; x < width; x++) {
                    int index = getIndex(y, z, x);
                    CageBlock cageBlock = cageBlocks[index];
                    if (cageBlock != null) {
                        int zPosition = isFlipped ? z : (depth - 1 - z);
                        setBlock(position, y, zPosition, x, cageBlock);
                    }
                }
            }
        }
    }```

[pine grail]

okay so, what is getIndex

[steady shard]

location thats being parsed as the argument:
Location cageLocation = map.getTeamSpawnpoints().get(bridgeTeam).toTeamSpawn(world, bridgeTeam) .add(0, 5 - cages.get(bridgeTeam).getYPosition(), 0);

    private int getIndex(int y, int z, int x) {
        return (z * depth * width) + (y * width) + x;
    }```

[pine grail]

oh yeah

[novel meteor]

(what the heck is y z x)

[pine grail]

I see the issue

[pine grail]

it makes sense to me

anyways

[steady shard]

it seems like the cache method is fine but the generate method isnt

[pine grail]

use the formula in getIndex for every place you're doing something similar

[steady shard]

its possible I fucked up conversion

[pine grail]

they all need to be the exact same

[steady shard]

pretty sure I am

[pine grail]

nop, conversion uses x y z order and getIndex uses z y x order

[steady shard]

wha

so change the setBlock

[pine grail]

x * (oldCage.getHeight() * oldCage.getDepth()) + y * oldCage.getDepth() + z; is in conversion

replace it with

z * (oldCage.getDepth() * oldCage.getWidth()) + y * oldCage.getWidth() + x;

wait

no

[steady shard]

ah

[pine grail]

they're completely diff

what is this 😭

[steady shard]

idk bro again I cannot comprehend it so

[novel meteor]

why tf are you mixing xyz and zyx and yzx

that's just confusing everyone here

[pine grail]

do this

[pine grail]

they forgot to replace something, it's okay dw

[pine grail]

@steady shard

[steady shard]

alright

one sec

I see I see now

sorry lol

out of bounds

Caused by: java.lang.ArrayIndexOutOfBoundsException: 315
        at net.xincraft.systems.match.cage.CageConverter.convertOldCages(CageConverter.java:38) ~[?:?]

                // Convert the old cageBlocks array
                CageBlock[] newCageBlocks = new CageBlock[oldCage.getWidth() * oldCage.getHeight() * oldCage.getDepth()];
                for (int x = 0; x < oldCage.getWidth(); x++) {
                    for (int y = 0; y < oldCage.getHeight(); y++) {
                        for (int z = 0; z < oldCage.getDepth(); z++) {
                            int index = z * (oldCage.getDepth() * oldCage.getWidth()) + y * oldCage.getWidth() + x;
                            newCageBlocks[index] = oldCage.getCageBlocks()[x][y][z];
                        }
                    }
                }```

[pine grail]

hm I see

okay just replace z and y

in all places you use this formula

[steady shard]

so

    private int getIndex(int y, int z, int x) {
        return (y * depth * width) + (z * width) + x;
    }```

[pine grail]

yus

and

y * (oldCage.getDepth() * oldCage.getWidth()) + z * oldCage.getWidth() + x;

[steady shard]

wait wtf wheres height

[pine grail]

you don't really need it

[steady shard]

interesting

holy shit it worked

nice shit

[pine grail]

show

[steady shard]

lets see if it feels faster

it does too

[pine grail]

W

yeah just rule of thumb

1D arrays

are the goat

also lemme explain how it works

imagine you slice up a cube

first you do it in floors

so say you have a 10x10x10 cube

you have 10 floors

each floor has width * depth blocks inside, right?

the formula for the area of a plane is that ^

[steady shard]

yea

[pine grail]

by doing y * width * depth

you skip y floors

[steady shard]

right

[pine grail]

so y = 0 would mean first floor, etc

[steady shard]

so if y is 0 then youre on the first

yea

[pine grail]

then

split each floor into rows

you get 10 rows

each row is width length

so the same exact thing applies

z = 0 means the first row

z = 1 is the second

etc

then x is just the specific column in the row

[steady shard]

interesting

yeah that does make sense

[pine grail]

floor_size = width * height
floor = y * floor_size

row_size = width
row = z * row_size

column = x

selected = floor + row + column

same thing applies in 2D

you just remove floor

and it becomes like

y * width + x

(you don't have z in 2D yk)

[steady shard]

    private int getIndex(int y, int z, int x) {
        // slice a cube into floors
        // each floor is width * depth in length, y=0 is the first floor, y=1 is the second floor, etc.
        int floor = (y * depth * width);
        // each row is width in length, z=0 is the first row, z=1 is the second row, etc.
        int row = (z * width);
        // x is the column in the row
        return floor + row + x;
    }```

this checks out yea

[pine grail]

yup

that's actually a great way to remember

you should now try to remove the old class

and straight up remove the conversion

[steady shard]

yeah will do

its still a little bit noticibly laggy when it initially runs

cache and then generate straight after

as both happen when the game starts

[pine grail]

issogei

you can do it :D

[steady shard]

actually maybe its not

[pine grail]

wha

[steady shard]

might just be me thinking its lagging

should I benchmark?

[23:12:31 INFO]: [XinCraft] Cached and generated cages for each team (took 49ms)

I'm thinking like

I could just cache like 20x20x20 at the start of the map load

for both sides

and have it not dependent on the cage size

[spring reef]

do the calculations async and send the block changes sync

[jagged rock]

Worker threads + chunk snapshotting + fast block placement

[dense leaf]

https://github.com/mcbrawls/inject/

I'm really not a fan of how I did the platform thing, are there any suggestions to that?
and general stuff too ofc

ready to get roasted

||-# spigot support might come soon idk 😭||

[round fossil]

Rad on Java is wild

[dense leaf]

I moved this to java yeah

[round fossil]

fair, what ru not fond of?

I don’t see the inherent issue really

[dense leaf]

wdym

[round fossil]

I mean, not a fan of?

Did you have another vision of how itd turn out?

[dense leaf]

I don't like how you have to do it on the instance

[dense leaf]

no I just haven't done this before 😭

[round fossil]

hmm, I assume you wanna provide library users the option to code platform agnostically against your library

[dense leaf]

I guess that'd be nice

[round fossil]

did I get that right?

just having the platform agnostic interfaces should be fine

[dense leaf]

With Kotlin you can at least fun InjectPlatform.Companion.registerPaper(injector: Injector)

[round fossil]

how you decide to provide the implementations don’t really matter too much

Yea fairs

I mean I can say that factory is nice since you decouple creation/obtaining the service implementation from the instantiation/instancing itself

But like who cares

[alpine anvil]

who emoted first

cowoconcluwube or ike

[pine grail]

conclube

I don't have nitro

[dense leaf]

[round fossil]

Yo yo lets not jump to conclusions

[alpine anvil]

ha

conclusions

new name fr

[round fossil]

😭

[junior holly]

https://tenor.com/view/birthday-happy-birthday-to-you-gif-11633040543978983038

[alpine anvil]

https://tenor.com/view/you-gif-25833251

current you

[woeful pasture]

based server

[woeful pasture]

https://github.com/Y2Kwastaken/MetaTrace

have at it ladies and gents

this is the first actual project I've posted here in a bit xD

[sharp epoch]

was Y2K_ taken too

[alpine anvil]

nah it ent

hes just weird

and a dream fan

[sharp epoch]

lmfao, I was about to say, you might be mistaken as a dream fan now lol

[woeful pasture]

shoulda known dream would get big smh

what an oversight

[sharp epoch]

wastaken used to be pretty normal before dream

[woeful pasture]

yeah I used it cuz I know others who did 🤷‍♂️ figured yeah works for me

but dream has to ruin everything 😡

[dense leaf]

you know you can change your gh handle right

[woeful pasture]

yeah I don't want to though

[sharp epoch]

and tbf I never understood why it is in past tense

it'd make more sense if it was <username>istaken but I guess it doesn't look as pretty

one might make the argument that it is in the past in the sense of it was taken when I tried using the handle but eh

then again istaken would feel like they're purposefully saying they're married in their handle lmao

guess there's no good way to phrase it

[woeful pasture]

I'm going to release my revolutionary bukkit competitor if no one reviews this code 😄

[sharp epoch]

Gson 🤮

moshi 🔛🔝

[woeful pasture]

I was debating do I support different json libraries?

and have different modules

[sharp epoch]

that sounds too overengineered, and it wouldn't really help the end user of the library because most if not all bukkit users will stick with Gson as that's what is bundled in the platform

I'd rather see people start adopting moshi but that's not going to happen anytime soon ig, since gson just works

[woeful pasture]

yeah true, but this is generic, I just happened to use GSON because its bundled with minecraft wich I use for my server api

[rancid fern]

You can use Configurate

It has support for both Jackson and GSON

as well as other formats if you want that

[sharp epoch]

eh, I don't love Configurate. It is a schema validator disguised as a configuration library

[rancid fern]

It works great

[dense leaf]

never had issues with it

[sharp epoch]

my issue with it is mostly the cultural perception, people believe that a configuration library should be able to handle dynamic values but Configurate is oriented towards static configuration for validation purposes, so dynamism is kind of rough with it

[woeful pasture]

configurate isn't really valuable for my library though since I only really need to parse and write json

[alpine anvil]

change it

go

[sharp epoch]

is there a reason you're implementing JsonSerialize/Deserialize separatedly instead of just implementing TypeAdapter

[woeful pasture]

they're different things

TypeAdapter is lower level, faster, but harder to implement

it handles like a stream versus an easy object based adapter

[sharp epoch]

so why go for the slower option

[dense leaf]

noticed anything else I should change?

[woeful pasture]

its what I'm used to and I prefer not to handle json deserialization in a stream like format tbh

[sharp epoch]

tbh, those classes don't seem to need a custom adapter for it

it doesn't look like you're doing anything more than just excluding fields, which could be done by marking the fields as transient in the class definition

[woeful pasture]

if you don't use a custom adapter you delegate to reflection

you could delegate to Unsafe, but that's on its way out

[alpine anvil]

gson has some built in de/serialization

[woeful pasture]

to me it just makes more sense to instead of using the default reflective deserializer/serializer to just implement one

[sharp epoch]

it really doesn't, there's nothing wrong with reflection, it is an instrospection tool

the bulk of the operation is going to be eaten by the IO costs of reading/writing to a file anyway, I don't believe performance is a worry here considering these files aren't going to be read more than once per plugin/mod

there could be the odd tool that tries to use your library for parsing plugin/mod metadata in bulk, but we're not there yet so I don't particularly think it is worth worrying about. Unless you want to specifically target that, then the best thing to do would be to actually benchmark whether the reflective approach is considerably slower for such situation, if it isn't then you can freely chip away at the line count for free

the way you use Gson leads me to believe you'd be more suited to use Jackson lol

[woeful pasture]

I just use GSON because its bundled

I mean Jackson is too, but its not exposed

[sharp epoch]

was Cuvee the previous name of this, that's what the usage uses lol

the utility methods that get metadata from the internet should either use a single HttpClient, or warn in the javadoc that it creates a HttpClient per request. Those are not free

[woeful pasture]

yeah lol I forgot to rename it

[woeful pasture]

will do smart idea

[late smelt]

If Conclube wasn’t a femboy he wouldn’t be buying people cat ears

[dense leaf]

why not

[night knoll]

XD

[night knoll]

bout to send that

oh alr sent

[steady shard]

what is chunk snapshotting and fast block placement

[dense leaf]

okay can I at least get some general feedback for this

[alpine anvil]

@woeful pasture

[dense leaf]

damn bro

[alpine anvil]

He's gonna go ham on it fr

[woeful pasture]

I gave him asm advice earlier

[dense leaf]

yeah no fuck spigot support for now

[alpine anvil]

Review it anyway

[woeful pasture]

okay I will after I'm done with homeowrk

[woeful pasture]

Chunk#getChunkSnapshot

?workdistro

[rich fog]

https://www.spigotmc.org/threads/guide-on-workload-distribution-or-how-to-handle-heavy-splittable-tasks.409003/

[woeful pasture]

Example
https://github.com/PineappleDevelopmentGroup/PineappleEnvoys/blob/master/src/main/kotlin/sh/miles/pineappleenvoys/util/ChunkSnapshotRetriever.kt

https://github.com/PineappleDevelopmentGroup/PineappleEnvoys/blob/master/src/main/kotlin/sh/miles/pineappleenvoys/configuration/EnvoyConfiguration.kt#L40

[woeful pasture]

looks good to me

[dense leaf]

miles approved

when we getting that as a sticker

[woeful pasture]

I could go full CMarco, but I won't

[dense leaf]

oh god what did he do

[woeful pasture]

do you not remember when he went around reviewing repositories?

and giving them ratings?

[dense leaf]

uh no

[woeful pasture]

L

[late smelt]

He did it for plugins too

Like, the resource pages

[steady shard]

what is ServerThreadSupplier

[woeful pasture]

https://github.com/PineappleDevelopmentGroup/Pineapple/tree/dev/pineapple-bundle/pineapple-core/src/main/java/sh/miles/pineapple/task/work

this is my implementation

[steady shard]

I dont actually understand what this is doing

could I use this async method instead of the other one?

[sharp epoch]

why so many abstractions

Y2K you have abstraction disease

it's when you can't help yourself but create more abstractions

[junior holly]

D: and miles was yelling at me for abstracting

[woeful pasture]

The design itself is rather bad to be completely honest

I haven't gotten around to reworking it

The suppliers themselves shouldn't be children of the other

I could do just a Object, but I don't really like the idea of having to do instanceof checks

I could also do their is always a return but in some cases its a Unit or something too

[sharp epoch]

for that one specifically, I was thinking you could just remove it all lol

I'm going to be very blunt here, but that just looks like a half-assed scheduler on top of the bukkit scheduler

[steady shard]

I've transformed it into this

    public void cageAndCacheTeamsAsync() {
        // Cache cages asynchronously
        CompletableFuture<Void> redTeamCacheFuture = CompletableFuture.runAsync(() -> cacheTeamCage(BridgeTeam.RED));
        CompletableFuture<Void> blueTeamCacheFuture = CompletableFuture.runAsync(() -> cacheTeamCage(BridgeTeam.BLUE));

        CompletableFuture<Void> allCachesCompleted = CompletableFuture.allOf(redTeamCacheFuture, blueTeamCacheFuture);

        allCachesCompleted.thenRun(() -> {
            System.out.println("All cages cached, now running generation code");
            // Generate cages asynchronously
            CompletableFuture<Void> redTeamCageFuture = CompletableFuture.runAsync(() -> {
                Location redCageLocation = map.getTeamSpawnpoints().get(BridgeTeam.RED).toTeamSpawn(world, BridgeTeam.RED)
                        .add(0, 5 - cages.get(BridgeTeam.RED).getYPosition(), 0);
                System.out.println("Generating red cage at " + redCageLocation);
                cages.get(BridgeTeam.RED).generate(redCageLocation);
                System.out.println("Red cage generated");
            });

            CompletableFuture<Void> blueTeamCageFuture = CompletableFuture.runAsync(() -> {
                Location blueCageLocation = map.getTeamSpawnpoints().get(BridgeTeam.BLUE).toTeamSpawn(world, BridgeTeam.BLUE)
                        .add(0, 5 - cages.get(BridgeTeam.BLUE).getYPosition(), 0);
                System.out.println("Generating blue cage at " + blueCageLocation);
                cages.get(BridgeTeam.BLUE).generate(blueCageLocation);
                System.out.println("Blue cage generated");
            });```

            CompletableFuture<Void> allCagesGenerated = CompletableFuture.allOf(redTeamCageFuture, blueTeamCageFuture);

            allCagesGenerated.thenRun(() -> {
                System.out.println("All cages generated, now sending players to cages");
                Bukkit.getScheduler().runTask(XinCraftPlugin.get(), () -> {
                    for (BridgeTeam bridgeTeam : BridgeTeam.values()) {
                        for (Player player : world.getPlayers().stream().filter(p ->
                                players.contains(p.getUniqueId()) &&
                                        teams.get(p.getUniqueId()).equals(bridgeTeam)).collect(Collectors.toList())) {
                            player.setGameMode(GameMode.ADVENTURE);
                            respawn(player, true);

                            List<Player> players = new ArrayList<>();

                            for (Player player_ : worldPlayersOf(this.players)) {
                                if (teams.get(player_.getUniqueId()) == bridgeTeam) {
                                    players.add(player_);
                                }
                            }

                            Location cageLocation = map.getTeamSpawnpoints().get(bridgeTeam).toTeamSpawn(world, bridgeTeam)
                                    .add(0, 5 - cages.get(bridgeTeam).getYPosition(), 0);
                            cageCountdowns.put(bridgeTeam, new CageCountdown(this, null, players, bridgeTeam, cageLocation));
                        }
                    }
                });
            }).exceptionally(throwable -> {
                throw new RuntimeException("Failed to generate cages", throwable);
            });
        }).exceptionally(throwable -> {
            throw new RuntimeException("Failed to cache cages", throwable);
        });
    }```

and then to ensure the actual block setting happens on main thread I just schedule it

    private void setBlock(Location position, int y, int z, int x, CageBlock cageBlock) {
        Bukkit.getScheduler().runTask(XinCraftPlugin.get(), () -> {
            Block block = position.getWorld().getBlockAt(
                    position.getBlockX() + x,
                    position.getBlockY() + y,
                    position.getBlockZ() + z
            );

            block.setType(cageBlock.getType(), false);

            BlockState state = block.getState();
            state.setData(cageBlock.getMaterialData());

            if (!state.update(true, false)) {
                throw new RuntimeException("failed to update block at " + block.getLocation());
            }
        });
    }```

so I believe all the calculations here are running async:

    public void generate(Location centreOfCage) {
        Location position = centreOfCage.clone().subtract((double) width / 2, 0, (double) depth / 2);

        boolean isFlipped = centreOfCage.getYaw() == -180;

        for (int y = 0; y < height; y++) {
            for (int z = 0; z < depth; z++) {
                for (int x = 0; x < width; x++) {
                    int index = getIndex(y, z, x);
                    CageBlock cageBlock = cageBlocks[index];
                    if (cageBlock != null) {
                        int zPosition = isFlipped ? z : (depth - 1 - z);
                        setBlock(position, y, zPosition, x, cageBlock);
                    }
                }
            }
        }
    }

    public void resetToCache(Location centreOfCage) {
        Location position = centreOfCage.clone().subtract((double) width / 2, 0, (double) depth / 2);

        for (int y = 0; y < height; y++) {
            for (int z = 0; z < depth; z++) {
                for (int x = 0; x < width; x++) {
                    int index = getIndex(y, z, x);
                    CageBlock cageBlock = cache[index];
                    if (cageBlock != null) {
                        setBlock(position, y, z, x, cageBlock);
                    }
                }
            }
        }
    }```

[sharp epoch]

that's way too much code to review on discord, put it on github or some other code hosting platform

[steady shard]

it doesnt actually work

let me fix that first

https://paste.md-5.net/nolawodeqi.cpp

[alpine anvil]

honestly he does tho

[sharp epoch]

I said github specifically because it is hard to review this piece of code in isolation to whatever else you might be doing

it'd be better to get the whole picture before going on a rant about the things you're doing here specifically

[steady shard]

I'll upload the match code to github but we dont want the whole thing open source

[sharp epoch]

if it is supposed to be a closed source plugin then I guess it's fine if you don't share it, I'm going to ask a lot of questions to understand the scope here though, if that's alright with you

[steady shard]

of course

https://github.com/XinCraft/codereview/tree/main

heres a bit more

if that helps

I've only implemented this async stuff during the end of the startMatch method in BridgeMatch

you'll want to look at
cageAndCacheTeamsAsync, cacheTeamCageAsync, and the Cage class

[sharp epoch]

there's way too much happening on that Match class

[steady shard]

i know I need to abstract it real bad

[woeful pasture]

Hey Anti Abstraction Senior here, Iagree with the JavierFlores guy

[steady shard]

I dont even know where to start when abstracting this

[steady shard]

anyway any feedback on the cage async generation

[sharp epoch]

yeah no, I tried to read the code but there's just way too much happening there lol

[sharp epoch]

one thing I can tell you is, you should stay back from the whole async thing

[steady shard]

you reckon

[sharp epoch]

mostly because you don't seem to understand the implications there, there's a lot of data races happening in that code

[steady shard]

alright

and honestly how woudl I even start to abstract this code

becuase I absolutely hate it

[sharp epoch]

well, Match seems to be a game loop, so center around that idea

aside from a game loop, it is also doing team management (that is, adding teams, removing teams, adding parties to teams etc)

so that part you can completely separate

[steady shard]

like use a TeamManager of some sorts

and handle all that code in there

and then seperate all the cage stuff

[sharp epoch]

yes, the BridgeMatch class should ultimately only be handling the game events, which are start match, end match, start countdown, etc

[steady shard]

so would I break away parts like

onKill, onDeath, etc

[sharp epoch]

I mean, BridgeMatch does seem to be handling some listener information, however I don't particularly see why it has to handle notification of anything other than the game events

[steady shard]

maybe like MatchListener, cus rn its just a listener that will find the player's match and then handle

whereas I could just create a new listener instance per match class

[sharp epoch]

that's a huge ass plugin

[steady shard]

when I get the chance going to split away a main plugin, there match plugin, then lobby plugin, and then bridge match plugin

[sharp epoch]

if you are willing to go for a big refactor, I recommend you to go on paper and think about the class structure before chipping away at these classes

[steady shard]

we're using pandaspigot atm which is a paper fork

[sharp epoch]

I say paper because that's what I'm used to however any UML diagram thing will do, or a whiteboard

[steady shard]

oh

literally paper

ahaha

yeah okay

[sharp epoch]

lmao

[sharp epoch]

yeah, I figured it was an older version by looking at some of the code there using older APIs

[steady shard]

honestly it used to be much much worse

I only just completed a full refactor of the match system

but it was definitely rushed

[sharp epoch]

it isn't that bad I believe, it is just that I don't think the design is in a good state right now in order to reasonably make anything out of the algorithm

[steady shard]

my end goal is to have a match plugin where I can easily just develop any minigame onto it just like bridge

[sharp epoch]

your issue in question is the fact that your gamemode/minigame requires placing a lot of blocks, which is the part of concern, however we can't really look at it in isolation since it's hard to understand what is the relationship with cages and the bridge match to think about optimizing it

[steady shard]

cages would only be apart of the bridge part of the plugin

but I agree that the bridge match class is ridiculously large

[sharp epoch]

if you abstract away the concepts enough, it could be doable, but you got to think of the gamemodes/minigames as a whole and ask the question "what constitutes a Match? How will this be used by the gamemode, what if other gamemodes were to use it?"

then again, as far I can see you are working with completely custom plugins and there already seems to be a lot of framework-ish plugins in your codebase (or so I imagine from the database players and all of that), so you also have to think on how to adapt these already existing systems into that

it may seem mind-boggling, but it really just takes sitting down and thinking about the relationships

[hasty lotus]

Hi, I have this huge class that is nearly 500 lines of code https://github.com/PulseBeat02/MurderRun/blob/main/src/main/java/io/github/pulsebeat02/murderrun/game/player/PlayerManager.java what can I do to help shorten it? Because I feel like making another class doesn't make sense because all the methods are manipulating all players as a whole

[night knoll]

hi

[hasty lotus]

hey

i finished my plugin btw pretty much

[night knoll]

oh cool

[steady shard]

@sharp epoch I don't know if I'm thinking correctly but I can have an BridgeMatch class that contains HitManager, CageManager, GoalManager, TeamManager

and then take away everything regarding those classes

[random yacht]

I’ll take a look in a bit here

You might honestly be fine though

[terse spoke]

I don’t know but some ideas:

For murderers and survivors, a parent class for those so you don’t need several methods doing the same thing (broadcasting) and seperating them with just an enum type.

Also in my opinion, you don’t need to cache everything in a minigame plugin because the performance improvement you’ll get from that over quering the entire list is negligible in a plugin that’s not made to hold so many players

Besides that, not really much to change, could split the caching to a seperate handler

[random yacht]

I was also going to say that I'm unsure how necessary those cache collections are

+1 for some interfaces. Good suggestion. MessageReceiver or something that both Survivor and GamePlayer could override

[terse spoke]

Exactly, minigames are likely running at most 50 players, and that small performance benefit from caching isn’t worth those 50 players imo

RAM usage is likely a proportionally higher result than the performance benefit than you’d get from it

[terse spoke]

^^

[random yacht]

Oh I'm sorry, Survivor and Killer both extend GamePlayer. Then you don't even need interfaces at all. Just a base method on GamePlayer

See brief LSP discussion in #help-development message

Applies to more than just collections and maps :)

[terse spoke]

Also if you want to save lines

private Collection<Survivor> cachedAliveSurvivors;
private Collection<Survivor> cachedSurvivors;

to

private Collection<Survivor> cachedAliveSurvivors, cachedSurvivors;

xD

[pine grail]

don't

it's not worth it

[novel meteor]

Why not just remove newlines altogether :)
I'm personally not a fan of doing this

[random yacht]

It's weird. There's like a time and a place to do that. I personally like doing it on occasion for very simple x, y, z variables, but yeah. Situational for sure

[novel meteor]

I 100% agree.

[gilded gate]

someone save me from this shit https://paste.md-5.net/asuhudijuq.cs

this should let specifi items to be sat on a gui

by putting it or shift clicking it

[rancid fern]

InventoryHolder and opening inventory directly in the event 🔫

[gilded gate]

😦

any better ways?

[late smelt]

?gui

[rich fog]

https://www.spigotmc.org/threads/a-modern-approach-to-inventory-guis.594005/

[late smelt]

And delay the open a tick

[gilded gate]

how can I save items in the gui?

should I serialize it?

[hasty lotus]

Oh shit interfaces and abstractions good point. And also I agree with you that the caching unnecessary the more I think about it

[hasty lotus]

Oh I originally had it for Survivor out of continence so I didn’t have to cash but I will use the most basic form yeah

[steady shard]

https://github.com/XinCraft/codereview

how can I further reduce this large as file and improve my match system

https://github.com/XinCraft/codereview/blob/main/match/match/BridgeMatch.java

@sharp epoch i did a lot of work to it

[night knoll]

public class SkyblockJavaPlugin extends JavaPlugin {
    public static SkyblockJavaPlugin instancia;
    public static SkillAndStatsManager skillAndStatsManager;
    public static String bungeeName = "";

    public static boolean sendPlayerToServer(Player player, String server) {
        try {
            ByteArrayOutputStream b = new ByteArrayOutputStream();
            DataOutputStream out = new DataOutputStream(b);
            out.writeUTF("Connect");
            out.writeUTF(server);
            player.sendPluginMessage(instancia, "BungeeCord", b.toByteArray());
            b.close();
            out.close();
        }
        catch (Exception e) {
            instancia.getLogger().info(ChatColor.RED + "Excepción conectándose a " + server);
        }
        return true;
    }

    @Override
    public void onEnable(){
        getServer().getMessenger().registerOutgoingPluginChannel(this, "BungeeCord");
        instancia = this;
        skillAndStatsManager = new SkillAndStatsManager(this);
    }

    public static SkyblockJavaPlugin getInstance(){
        return instancia;
    }

    public static SkillAndStatsManager getSkillAndStatsManager(){
        return skillAndStatsManager;
    }
}

simple code but i wanna make sure everything is okay
also i will update bungeeName on the onEnable so don't worry bout it

[night knoll]

instancia 🤌

[night knoll]

do you atleast know what that means

Yeah

then

hablás español? xd

Idk spanish

alr

Still running away from duolingo brid

lmao

[sharp epoch]

use a try with resources for the streams

[night knoll]

what 💀