1.14.4 My block cooldown code stops working after one use

Discussion in 'Spigot Plugin Development' started by Nahmay, Jan 18, 2020.

  1. So I have been working on a plugin simply for a cooldown for block breaking and placing. The coding process had been majorly successful until literally one point. If I am not actively trying to place another block after the cooldown is over, then the timer does not restart. Thus, with no timer to prevent you from going crazy with block placement -- the player (being me) has the ability to place as many blocks as they want to. From what it looks like to me, there should be no issue with my code and I am at a loss after many days of looking at it. If anyone has any ideas, I would greatly appreciate it. I'm not asking you to write a ton of code for me, but to steer me in the right direction.
    Code (Text):
    package redonkulous.nahmay.BlockCooldown;

    import java.util.HashMap;
    import java.util.UUID;
    import java.util.logging.Logger;

    import org.bukkit.Bukkit;
    import org.bukkit.Material;
    import org.bukkit.entity.Player;
    import org.bukkit.event.EventHandler;
    import org.bukkit.event.Listener;
    import org.bukkit.event.block.BlockPlaceEvent;
    import net.md_5.bungee.api.ChatColor;

    public class BlockPlace implements Listener {
        Player player;
        private int cooldowntime = 30;
        private HashMap<UUID, Long> cooldownMap = new HashMap<UUID, Long>();
       
        public BlockPlace(CooldownMain plugin) {
            plugin.getServer().getPluginManager().registerEvents(this, plugin);
        }

        @EventHandler
        public void blockPlace(BlockPlaceEvent e) {
            player = e.getPlayer();

            if (e.getBlock().getType() == Material.BEDROCK) {
                if (!player.hasPermission("cooldown.bypass")) {
                    player.sendMessage("You cannot place bedrock");
                    e.setCancelled(true);
                } else {

                }
            }
        }

        @EventHandler
        public void onBlockPlace(BlockPlaceEvent event) {
            final Player player = event.getPlayer();
            Logger log = Bukkit.getLogger();
            if (!player.hasPermission("cooldown.bypass")) {
                if (cooldownMap.containsKey(player.getUniqueId())) {
                    long secondsleft = ((cooldownMap.get(player.getUniqueId()) / 1000) + cooldowntime)
                            - (System.currentTimeMillis() / 1000);
                    if (secondsleft > 0) {
                        player.sendMessage(ChatColor.RED + "You are on cooldown currently for " + secondsleft);
                        log.info(player + " is on cooldown");
                        event.setCancelled(true);
                    }
                    if (secondsleft == 0) {
                        cooldownMap.remove(player.getUniqueId());
                    }

                } else {
                    player.sendMessage(ChatColor.GREEN + "You now have a cooldown of " + cooldowntime + " seconds");
                    cooldownMap.put(player.getUniqueId(), System.currentTimeMillis());
                    long secondsleft = ((cooldownMap.get(player.getUniqueId()) / 1000) + cooldowntime)
                            - (System.currentTimeMillis() / 1000);
                    if (secondsleft == 0) {
                        cooldownMap.remove(player.getUniqueId());
                    }
                }
            } else {
                log.info(player + " is bypassing cooldown.");
            }

        }

    }
     
  2. I suspect that it has to do with the way you check if the time is over. You use
    Code (Text):
    if(secondsleft == 0) { ... }
    but that will only fire if the countdown has reached that exact time. If the player doesn't run the code when the secondsleft is exactly 0, but is run later, the time remaining will be negative. My suggestion would be to use
    Code (Text):
    if(secondsleft <= 0) { ... }
     
    • Like Like x 1
  3. You're calculating the secondsleft wrong, you should remove time when the cooldown is expected to end from the current time.
    The correct way would be:
    Code (Java):
    long secondsleft = (System.currentTimeMillis() / 1000) - ((cooldownMap.get(player.getUniqueId()) / 1000) + cooldowntime);
    I'd also recommend checking if secondsleft <= 0 instead of secondsleft == 0, otherwise it would only work if they place a block at exactly 0 seconds after cooldown
     
    #3 Rayo, Jan 20, 2020
    Last edited: Jan 20, 2020
  4. Thank you to the both of you! I will test them both out immediately.
     
  5. It worked after the if statement for secondsleft was changed to <= 0, the calculation fix broke the code again. But thank you both!
     
  6. I don't know if you have fixed this issue yet or not but, there are a few issues I would like to point out or change.

    First you have two different handlers for the same event. Both of them check (!player.hasPermission("cooldown.bypass")).
    These two could conflict with each other, but in this case doesn't look like they will. You should merge the code into a single event handler none the less.

    Second you check to see if the player is in the cooldownMap. If they are not, you attempt to remove them. If they are not, you will not be able to remove them and you will get an error.

    Third I would structure your map differently and simply save the last time a player placed a block, and then check if more time has passed than cooldowntime since they last placed a block.

    Forth, and I see this all the time, you have a bunch of unsessary memory declarations. There is no need to have additional memory space created to store a reference to player, when the event already has that stored for you. It's kind of like saying Event myEvent = event. Why? It is already there.

    This might be a lot to take in so I cleaned up your code and show you exactly how I would handle the cooldown (I hope you learn from this):
    Code (Java):
        private int cooldowntime = 30;
        private HashMap<UUID, Long> cooldownMap = new HashMap<UUID, Long>();
     
        public BlockPlace(CooldownMain plugin) {
            plugin.getServer().getPluginManager().registerEvents(this, plugin);
        }
     
        @EventHandler
        public void blockPlace(BlockPlaceEvent e) {
            // This if statement merges your first event handler.
            // This makes it so that only players with the "cooldown.bypass" can place bedrock period.
            // I am not sure if this is your intent, but how you previously written it.
            if (e.getBlock().getType() == Material.BEDROCK && !e.getPlayer().hasPermission("cooldown.bypass")) {
                e.getPlayer().sendMessage("You cannot place bedrock");
                e.setCancelled(true);
            }
            else if (!e.getPlayer().hasPermission("cooldown.bypass")) {
                if (!cooldownMap.containsKey(e.getPlayer().getUniqueId())) {
                    // If the player is not in the map, add them and set the time they last place a
                    // block to now.
                    // If they are not in the map that means that they do not have a cool down we
                    // need to worry about and skip the cool down checks.
                    cooldownMap.put(e.getPlayer().getUniqueId(), System.currentTimeMillis());
                    e.getPlayer().sendMessage(ChatColor.GREEN + "You now have a cooldown of " + cooldowntime + " seconds");
                } else if (System.currentTimeMillis() - cooldownMap.get(e.getPlayer().getUniqueId()) >= cooldowntime * 1000) {
                    // If enough time has passed, let them place the block, and update the last time they placed one.
                    cooldownMap.put(e.getPlayer().getUniqueId(), System.currentTimeMillis());
                } else {
                    // They player has a cooldown and should not be allowed to place a block.
                    e.getPlayer().sendMessage(ChatColor.RED + "You are on cooldown currently for " + (cooldownMap.get(e.getPlayer().getUniqueId()) + (cooldowntime * 1000) - System.currentTimeMillis()) / 1000);
                    Bukkit.getServer().getLogger().info(e.getPlayer() + " is on cooldown");
                    e.setCancelled(true);
                    }
            } else {
                Bukkit.getServer().getLogger().info(e.getPlayer() + " is bypassing cooldown.");
            }
        }
    The only issue here is that players never get removed from the map, only added. If you end up with 10,000 different people connecting to the server before the plugin reloads or the server resets you might run into memory trouble (honestly you should be fine though even with 10,000+).
    You will have to also handle the PlayerLeaveEvent (going from memory might have a slightly different name) and simply remove them from the map when that event is invoked.
    Their cool down will be reset though, which may not be a problem for you.
    If it is problem, for instance cooldowns are like 10 minutes or something, you might have to get a little more fancy.
    You will then have to schedule a task to remove them from the map when the cooldown finishes. Check the current time when they logged off, subtract the last time they placed a block (or what ever), and then schedule a task (or runnable, or what ever floats your boat) to run that many milliseconds from now.
    You have to be able to cancel that if they log back on in the mean time. OnPlayerJoin will have to see if there is a task scheduled that is going to remove them from the map, if there is, you need to cancel the task and leave them in the map instead.

    Hope this helps...
     
    #6 ForbiddenSoul, Jan 23, 2020
    Last edited: Jan 23, 2020
  7. I have to disagree with you on this one. Let's start with code clarity first. Declaring an additional variable for a method call make your code in my opinion more easily readable. It makes it so you don't have to make the same getter call every single time, but can just use the variable. It condenses your code, which makes it easier to read.
    Menory is not a concern here. A variable only points to a block of memory and these pointers take barely any space, a few bytes at the very most; you're working for a server, which usually have, at the very least, a couple gigabytes of space allocated to them.
     
  8. Let me just elaborate on this, because this is not a matter of opinions; the approach of using a getter every time is simply wrong. Even thou I admire @ForbiddenSoul helping others out in a way that some of us could only dream of, this is a practice that should be avoided at all times. Here's why:
    1. There are no memory issues. Using the methodology Player player = e.getPlayer() does not allocate a single bit more of the heap
    2. This methodology is more efficient; in some cases, this is even noticeable
    Here's the reasoning behind 1 and 2:
    player is a variable. As @Stef pointed out, it's a pointer. A pointer (in java) is a 32-bit integer. This variable gets either stored to some (virtual) register; or (and this is the worst that could happen) gets pushed onto the stack. Since player only lives inside this method, after returning, the stack-pointer is where we left it before entering the method (since the method returns void). Also, any register would be overridden by whatever function needs it. After and return-statement, or after the control flow reaches the bottom of the method, the player-variable will have vanished in an instant

    getters and setters do not only get and set the variable in numerous cases. There is a reason, why we use them instead of making variables globally, and one is so that we can perform checks or execute any additional code before we return the variable. Take this example:
    Code (Text):
    public class Rectangle {
        private Vector start;
        private Vector end;

        public Vector getStart() { return start; }
        public Vector getEnd() { return end; }

        public double getDistance() {
            return Math.sqrt(start.x * end.x + start.y * end.y + start.z * end.z);
        }
    }
     
    If we only see the method signature (which is especially the case if we look at interfaces, and Bukkit has a lot of them), we could not see if getDistance() returns a variable or is a "virtual variable". If we assume, that this class stores distance and call it a thousand times per second (for example to update some trajectories), we would compute the same value over and over again and use the expensive sqrt-function.
    If we store the result (because we know that the start and end-point do not change), we have a much more efficient code.

    Again, I do not want to cause any turmoil; but something that is just wrong should never be taught to beginners.
     
  9. You two or both correct with your logic here, but not 100%, and maybe not seeing the whole picture.
    It really does depend on what the getter function actually calculates if it is more efficient to store the variable in memory, and it definitely makes your code easier to read.
    This is not true for managed language like JAVA. Managed languages like JAVA, JS, and C# use references instead of pointers. If this were an unmanned language like C++ you could use a pointer and it would only point to a block of memory, like you say.
    In the case here of it being a reference we the programmers have very little default control over what the virtual machine decides to store as a reference, and it will most likely not just be an int value pointing to a memory address.

    Then we have an example of the getDistance function. Although it does use the sqrt() function, which is more expensive than + or / operators, it is still not that expensive. It is also the same as using pow(x, 1/2) (x to the power of 1/2), so it is not any more or less expensive than the pow() function. You can also get cubed roots etc. this way for example pow(x, 1/3).

    The efficiency of using a variable to store the result or to simply call the function is circumstantial.
    If you were to write some code like so:
    Code (Java):
    double distance = rectangle.getDistance();
    if (distance >= 1) {
        // Do stuff
    } else {
        // Do other stuff
    }
    and the if statement is the only time we make reference to the rectangle's distance, it is less performant than no memory declaration and simply calling the function.
    The above code: Allocates space for a double called distance on RAM -> Allocates space for the return value of getDistance(), but this is usually done on the CPU cache, not RAM and is much much faster -> The return value is written to the cache -> The return value is read from the cache and copied to the distance variable on RAM -> The distance variable is read from RAM, and compared to 1 -> A boolean is determined on the cache and we choose which statement to go to, the if, or the else.

    We can remove over half these steps, and stay within the CPU cache and out of RAM by removing the memory declarations:
    Allocate space for the return value of getDistance() on the CPU cache -> Read value from the cache and compare to 1 -> Get a boolean result and choose a statement to go to.
    Both of these methods require the same CPU cycles to perform the code inside the getDistance() method, and there is no way around not calling the function.

    If we were to make dozens of calls to getDistance() it very well would be more performant to only call the function one time. This is also circumstantial though, how much faster is your CPU clock vs RAM clock, and how much faster are CPU cache Read/Write/Allocate/Assign implementations over the RAM?
    This is on a machine by machine basis depending on that machines specific hard ware, but who knows maybe making 12 getDistance() calls is in fact faster than allocating it to RAM, and reading it 12 times.
    Honestly the rule I go by is if we are going to use the value more than once, use a variable. If not, use the CPU and cache.
    But there are clear instances for example e.getPlayer() where that function is a one liner which is simply return "some other memory value". Here I will never declare new memory space. I don't see benefit of making additional RAM Read/Write calls in exchange for readability.

    At the end of the day it is completely up to you guys what style of code you choose to develop, and honestly there is not a wrong answer there. Painters paint masterpieces in vastly different ways, so do coders.

    Anyways for this specific example, I did not see multiple calls to the same calculations, and the variables were pretty much all one liner return "some other memory space" get functions. This is why I suggested it in this way, but what ever works for you.
     
  10. Gonna fan the flames a bit here but I couldn’t help it. I don’t disagree with the conclusion, but the logic being applied here has all kinds of flaws that I feel would benefit from having some additional... critique, shall we say?

    Let me preface this by saying that the local variable being present or not literally doesn’t matter. There’s no difference.

    You’re overthinking this. Regardless of whether or not you are declaring a variable in javaspace, the compiled bytecode still needs to push the result of getDistance() onto the stack. The local variable is implied if you don’t declare it yourself in your particular example.

    I think you have a fundamental misunderstanding of how most modern CPUs work. Intel and AMD x86 processors rely on MESI derivatives (MESIF and MOESI) which are usually write-back caches, meaning that cache entries are only written to main memory when they are invalidated. I highly doubt this kind of transaction (where something is written from the CPU cache to main memory and read from main memory into the cache again) would ever occur in practice, that’s why cache coherency protocols exist in the first place.

    Even this was something that occurred on a regular basis in real life, that still doesn’t guarantee that local variables require a trip to main memory and back. In fact, arrays of empty memory are used to pad data for the express purpose of keeping each stripe of data on separate cache lines even for data structures which you allocate on the heap (see Striped64 for reference). For localized stack allocations, I would expect an even greater incidence of the CPU caches being used.

    CPU clock and RAM clock are completely irrelevant here. You can have an infinite clock on both your CPU and RAM, but that doesn’t change the fact that accessing main memory is a massive performance hit. A trip to main memory is enormously expensive in comparison looking up something from the cache, not to mention contests for bandwidth with other execution pipelines. CPUs are expressly designed to avoid having to go to main memory.

    The difference in 12 calls to the same method in your argument versus one call stored to a single local variable is not a matter of memory allocation. It might not even matter that you have a local variable or not because the JIT (or a smart compiler) may even perform constant folding on repeated calls to a stateless method.

    The point is, you’re making it significantly more black-and-white than it really is. Don’t call a relatively expensive computation 12 times because you’re afraid of going to RAM. Chances are, it will never even get to that point (see points about cache coherency and CPU design I’ve made above). Use a freakin local variable lol. It makes your code a lot cleaner and avoids having to do any guesswork with the compilers. Reason about the way your code and your logic works, not the hardware. Java was created so that people wouldn’t need to worry about these little details :). Write once, run anywhere and all.

    It’s not possible to control this behavior purely in javaspace. I mentioned as much earlier: you’re at the whim of the JVM when it comes to whether local variables require main memory access, and even then, that scenario is highly highly unlikely.
     
    #10 xTrollxDudex, Jan 24, 2020
    Last edited: Jan 24, 2020
    • Agree Agree x 3
  11. Also, if any compiler produces any byte-code from this
    Code (Java):
    Object foo = bar.getFoo()
    if (foo.equals(a)) {

    } else {

    }
    that is less efficient than this:
    Code (Java):
    if(bar.getFoo().equals(a)) {

    } else {

    }
    You should throw that compiler right out of the window.

    Edit: also, what is the difference between pointers and references (other than that the latter ones don‘t allow arithmetics) and how do you make the case, that using pow() is a cheap operation? That might be true for integers, since you only have to multiply n times; but for doubles this operation is expensive as well (at least significantly more expensive than any addition or subtractions)
     
    #11 Schottky, Jan 24, 2020
    Last edited: Jan 24, 2020
  12. I indeed meant "reference" not "pointer", however I don't think this defeats the point I was trying to make. You're completely right that the JVM can pick any size it pleases for this reference, so this uncertainty could, understandably, be a cause of concern. However, we're talking about something that's very practical here - actual code that will be ran on servers - and not purely theoretical, so I personally think we should also take the practical side into account here. Even though JVMs can decide to dedicate the entirety of your allocated RAM to a single reference, they don't. We're talking in the scope of bytes here and there may be a bit more bytes or less bytes depending on the JVM, but at no point would we even be remotely close to talking about kilobytes for a single reference. This amount is so little, that, at no point in time, I'd even consider sacrificing code readability to gain such a little amount of extra memory.