-
-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature: API for AFK and excluded players #88
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @nkomarn is currently pretty busy so he asked me to review these changes 😄.
In general the changes look good. (only one comment)
I do however want to object against the dependency on StaffFacilities.
Harbor only provides first party support for plugins that get used A LOT by our target audience (survival servers). The StaffFacilities integration would better as a separate plugin, as it is probably not used by the majority of the servers using Harbor.
PS: I really like the ExclusionProvider
pattern. It is very clean! 👍 👍
…ture/exclusion-api # Conflicts: # src/main/java/xyz/nkomarn/harbor/util/PlayerManager.java
Also change examples and remove stafffacilities
374f30d
to
1d627ba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its getting better and better :D
import java.util.Map; | ||
import java.util.UUID; | ||
|
||
public class DefaultAFKProvider implements AFKProvider, Listener { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this class doesnt contain any event listener, but is a listener.
It should be only an AFKProvider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well... Seems like I overlooked this xD Then ignore this I guess xD
I think I mixed some stuff up and got a bit confused since my branch is now in here...
if (enabled = (harbor.getConfig().getBoolean("afk-detection.fallback-enabled", true))) { | ||
harbor.getLogger().info("Registering fallback AFK detection system."); | ||
listeners = new AfkListeners(this); | ||
harbor.getServer().getPluginManager().registerEvents(this, harbor); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
listeners
needs to be registered not this
.
Also:
Better call enableListeners()
and move the print and registration to this method.
Move the object creation to enable listener as well. See below for reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay this makes sense now with the stuff above... I think this whole thing should be changed...
There is now need to seperate the default AFK provider from the AFK listener. This just causes confusion.
import java.util.ArrayDeque; | ||
import java.util.Queue; | ||
|
||
public final class AfkListeners extends BukkitRunnable implements Listener { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fault on my side. Class should be singular named: AfkListener not AfkListeners.
*/ | ||
public void enableListeners() { | ||
if (enabled) { | ||
listeners.runTaskTimer(harbor, 1, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will throw an exception if the task was run before and canceled. A once canceled task can not be run again.
|
||
@Override | ||
public void cancel() { | ||
super.cancel(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since the whole system changes now with this change the listeners needs to be unregistered as well.
This should be ensured by the cancel method to avoid that the listener produces overhead.
|
||
public void disableListeners() { | ||
if (enabled) { | ||
listeners.cancel(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throw away the listener reference to avoid that the listener is started again after canceled.
* An {@link AFKProvider} that uses Essentials; can be used as an example of how external | ||
* plugins can implement an {@link AFKProvider} | ||
*/ | ||
public class EssentialsAFKProvider implements AFKProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love how clean this looks now.
I have done a bit of a rewrite that should address all of the earlier concerns |
Biggest question is if the following ternary (in PlayerManager) is acceptable: /**
* Checks if a player is considered "AFK" for Harbor's player checks.
*
* @param player The player to check.
*
* @return Whether the player is considered AFK.
*/
public boolean isAfk(@NotNull Player player) {
return !(andedProviders.isEmpty() && oredProviders.isEmpty()) ?
(!andedProviders.isEmpty() && andedProviders.stream().allMatch(provider -> provider.isAFK(player)))
&& (oredProviders.stream().anyMatch(provider -> provider.isAFK(player)))
: defaultProvider.isAFK(player);
} |
No it isn't 😅 It has very bad readability. Use a regular if statement, reversing the statement would be even cleaner. ex:
|
Would this be acceptible? /**
* Checks if a player is considered "AFK" for Harbor's player checks.
*
* @param player The player to check.
*
* @return Whether the player is considered AFK.
*/
public boolean isAfk(@NotNull Player player) {
// If there are no providers registered, we go to the default provider
if(oredProviders.isEmpty() && andedProviders.isEmpty()){
return defaultProvider.isAFK(player);
}
return
// Is true if at least one of oredProviders returns true; is false if there are no oredProviders
oredProviders.stream().anyMatch(provider -> provider.isAFK(player)) ||
/*
* If all of anded providers are true are true, then this statement is true. If there are no anded providers
* then allMatch would return true, therefore since we don't want this to be true in that case (where there
* are no anded providers), we want this to be false, therefor we need to make sure it's not empty
*/
(!andedProviders.isEmpty() && andedProviders.stream().allMatch(provider -> provider.isAFK(player)));
} |
Yes, if you remove the comments in the middle of the code 😅. The reason that the previous code was "unreadable" was because of the formatting not the actual logic. Ternary operators are never a good idea when handling long lines of code. Also note that comments do not necessarily add to code readability or clarity. Sometimes comments do make sense, but I think the logic in this method is easy enough to understand without them 😄 |
Understood, will remove before commit!
…On Wed, Mar 31, 2021, 12:25 PM Jeroen Verwimp ***@***.***> wrote:
...
Would this be acceptible?
/**
* Checks if a player is considered "AFK" for Harbor's player checks.
*
* @param player The player to check.
*
* @return Whether the player is considered AFK.
*/
public boolean ***@***.*** Player player) {
// If there are no providers registered, we go to the default provider
if(oredProviders.isEmpty() && andedProviders.isEmpty()){
return defaultProvider.isAFK(player);
}
return
// Is true if at least one of oredProviders returns true; is false if there are no oredProviders
oredProviders.stream().anyMatch(provider -> provider.isAFK(player)) ||
/*
* If all of anded providers are true are true, then this statement is true. If there are no anded providers
* then allMatch would return true, therefore since we don't want this to be true in that case (where there
* are no anded providers), we want this to be false, therefor we need to make sure it's not empty
*/
(!andedProviders.isEmpty() && andedProviders.stream().allMatch(provider -> provider.isAFK(player)));
}
Yes, if you remove the comments in the middle of the code 😅. The reason
that the previous code was "unreadable" was because of the formatting not
the actual logic. Ternary operators are never a good idea when handling
long lines of code.
Also note that comments do not necessarily add to code readability or
clarity. Sometimes comments do make sense, but I think the logic in this
method is easy enough to understand without them 😄
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#88 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABFUCWUZFVE373Q46SMRYGTTGNSKXANCNFSM4ZVXZRBQ>
.
|
All fixed |
afkProvider.updateActivity(player.player); | ||
} | ||
playerQueue.add(player); | ||
} while (System.currentTimeMillis() - start < MAX_PROCESS_TIME_MS && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed this.
This is exactly what I wanted to avoid. Iterating over all players each tick. Also the synchronized doesnt makes any sense at all since this task is executed synced by the mainthread. There is no way that this will ever be executed twice.
This is all just very over complicated without any advance in functionallity. Its even worth then before.
My mechanic checked every player every 20 ticks, which is by far enough. The new mechanic checks every player every thick, which is way to much and is not required at all.
The do while makes it also overly complex.
Also I would remove the max process time mechanic since it is not really required like stated in #87.
I think merging my pr into this pr makes everything a bit messy and confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also revisiting you code this is a real mess.
You peek a player. Then you put the player back to the queue which results in the player being twice in the queue.
After this you poll a player without knowing that the player will be added back into the queue, which will result in a loss of players in the queue.
this still may work somehow, but you cant deny that my implementation was simpler and has less computation in one tick with the same result. Any my code is more readable and maintainable. It seems like you like fancy looking code, but clean and simple code is preferable. You just solved the problem with way more logic than it is required.
Please just change this back to my routine. You can also remove the 20 ms mechanic since this condition will never be hit unless over 1000 players are on a server.
Also remove the useless synchronize keywords, which makes no sense at all in a blocking task in the main thread. You meant it good, but made it worse sadly. Sometime simple is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I miswrote it while far too tired and should not have pushed, I;m sorry.
afkProvider.updateActivity(player.player); | ||
} | ||
playerQueue.add(player); | ||
} while (System.currentTimeMillis() - start < MAX_PROCESS_TIME_MS && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also revisiting you code this is a real mess.
You peek a player. Then you put the player back to the queue which results in the player being twice in the queue.
After this you poll a player without knowing that the player will be added back into the queue, which will result in a loss of players in the queue.
this still may work somehow, but you cant deny that my implementation was simpler and has less computation in one tick with the same result. Any my code is more readable and maintainable. It seems like you like fancy looking code, but clean and simple code is preferable. You just solved the problem with way more logic than it is required.
Please just change this back to my routine. You can also remove the 20 ms mechanic since this condition will never be hit unless over 1000 players are on a server.
Also remove the useless synchronize keywords, which makes no sense at all in a blocking task in the main thread. You meant it good, but made it worse sadly. Sometime simple is better.
* | ||
* @return true if the position changed | ||
*/ | ||
private synchronized boolean changed() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove synchronized here. this is missleading in a main thread task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The synchronized keywords were added accidentially due to an incorrect flag set in my IDE, and are now fixed.
I will also do the 20ms cleanup, and, as that condition is unnecessary, we can simply switch to an arraylist (or a hashset!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rainbowdashlabs I replaced my atrocious code as per your suggestion; this is why I shouldn't try to code on minimal sleep for public projects! |
@aakatz3 |
I misunderstood what you were doing, in the checking 1/20th players per tick, I misread as 20 ticks in a second. My only concern is the double rather than an int being used in the loop. Now that I actually understand your code I will put it back, I misunderstood what you were saying before. I was under the impression that filtered streams would be faster, and I apologize for that misunderstanding. I want to really apologize for being incredibly annoying, and I'd like this to be taken as a formal apology as I try to re-merge your code, in a way that doesnt break things. |
@rainbowdashlabs I really hope I got it in there properly this time; I'm not quite sure what I'm going to do if I somehow did it wrong yet again. |
afkProvider.updateActivity(afkPlayer.player); | ||
} | ||
players.add(afkPlayer); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just use a double :D This wont kill anyone and will make the logic simpler :D
Currently you will check a player in an unstable state. If one player is on the server the player will be checked every tick.
4 Players -> Every 5 Ticks
10 Players -> Every 10 Ticks
Only after 20 players online the splitting will have the effect I want.
That I used a double had the thought that I might want to check only a friction of the players and I will wait until I have a full player 1 > checksToMake
. this will allow empty runs when less then 20 players are online and when more then 20 players are online it will check in some runs more than one,
Doubles arent bad for loops :D
Otherwise we could just merge it and I will apply the optimizations by myself in a new PR. Your choice.
Or still the choice of the maintainers of this project.
if (players.isEmpty()) return;
checksToMake += players.size() / 20.0;
while (checksToMake > 1) {
AfkPlayer afkPlayer = players.poll();
if (afkPlayer.changed()) {
playerManager.updateActivity(afkPlayer.player);
}
players.add(afkPlayer);
checksToMake--;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Converted to double, using for loop syntax:
private final class PlayerMovementChecker extends BukkitRunnable {
private double checksToMake = 0;
@Override
public void run() {
if(players.isEmpty()){
checksToMake = 0;
return;
}
// We want every player to get a check every 20 ticks. Therefore we check 1/20th of the players
for (checksToMake += players.size() / 20.0D; checksToMake > 0 && !players.isEmpty(); checksToMake--) {
AfkPlayer afkPlayer = players.poll();
if (afkPlayer.changed()) {
afkProvider.updateActivity(afkPlayer.player);
}
players.add(afkPlayer);
}
}
}
@thatgoofydev Don't mean to bother you but can you give a final review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks very good!
Just two tiny comments about "code style" related things.
* Provides a way to start the listener | ||
*/ | ||
public void start() { | ||
JavaPlugin plugin = JavaPlugin.getPlugin(Harbor.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please pass the Harbor instance trough the constructor, to maintain consistency with the rest of the code.
// percentage of players over all 20 ticks. Thusly, the runnable must run on every tick | ||
movementChecker.runTaskTimer(plugin, 0, 1); | ||
|
||
JavaPlugin.getPlugin(Harbor.class).getLogger().info("Fallback AFK detection system is enabled"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass Harbor instance through the constructor
(ps you already had the harbor instance assigned to a variable in at the top of the method 🙃 )
movementChecker.cancel(); | ||
HandlerList.unregisterAll(this); | ||
players = null; | ||
JavaPlugin.getPlugin(Harbor.class).getLogger().info("Fallback AFK detection system is disabled"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass Harbor instance through the constructor
private final Logger logger; | ||
|
||
public DefaultAFKProvider() { | ||
Harbor harbor = JavaPlugin.getPlugin(Harbor.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass Harbor instance through the constructor
public void updateActivity(@NotNull Player player) { | ||
playerActivity.put(player.getUniqueId(), System.currentTimeMillis()); | ||
public void addAfkProvider(AFKProvider provider, LogicType logicType) { | ||
(logicType == LogicType.AND ? andedProviders : oredProviders).add(provider); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bad readability a normal if else or switch would be better.
Not that big of a deal, but just so you know ig. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8PM Me thought that was more readable than using a hashmap to deal with it, but an if/else would be more readable (though in this case, I'm going to go with a switch)
Alright, I'll try to put these in! |
@thatgoofydev All done! |
@aakatz3
Environment:
A other small thing: I think excluding vanished players should be enabled by default? The default configuration should be what fits best for most people. And I think most people would want to keep vanished people hidden by not including them in the player count. 😅 |
I agree on the vanished, it was set as false in the original. Harbor reload should be easy to fix, but AFK I need to take a look in a little bit deeper. I'll keep you posted! |
Found the issue with AFK, the config value was renamed and I didn't take it into account, because i'm dumb |
It's slightly more expensive, but I also made each exclusion provider check if it should be enabled via the config, as otherwise I'd need to add callbacks on reloads of the config. |
int hash = player.getLocation().hashCode(); | ||
boolean changed = hash != this.hash; | ||
this.hash = hash; | ||
return changed; | ||
Location prev = location; | ||
location = player.getEyeLocation(); | ||
return !prev.equals(location); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this change to Location instead of it's hash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dont know. Maybe we just just merge this and I will create another Merge request and fix based on my recommendations.
We are already discussing on this PR for a month now and I am getting slightly tired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to test something and forgot to revert it, it was part of trying to troubleshoot the "everyone is afk" issue, using a debugger
import java.util.List; | ||
|
||
public class Config { | ||
private final List<ReloadListener> reloadListeners; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like I included some bits in the commit that were supposed to not be there from an earlier version of the code. The original idea was to have a way for code to detect and respond to a reload rather than always checking with the config file, but I ended up just always checking the File config. I have both implementations so we can go with whichever you all think is more preferable, I just committed one full and one half together by accident
Remove things I thought I removed but didn't
Bumping this one more time, trying not to be annoying. I believe I've addressed everything |
I've been maintaining a fork of Harbor to work with a couple of plugins on a server I develop for. Rather than maintain a fork, I thought it might be better to introduce an API that allows external plugins to provide logic to Harbor on excluding players, or determining if a player is AFK. In addition, I added a way to unregister Harbor's AFK listeners, which would be useful if an external plugin is being used.
I am open to comment and suggestion. One thought I had is moving the essentials code into an example implementation of AFKProvider, and including that by default in Harbor. I was also thinking of adding support for StaffFacilities, as an example of how to use the ExclusionProvider.