Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Enable or disable stacking based on TPS hysteresis #99

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Enable or disable stacking based on TPS hysteresis #99

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f9daa6
disable or enabled stacking based on TPS
quiquelhappy Jan 8, 2024
fdc2123
performance settings, tps toggle
quiquelhappy Jan 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Plugin/src/main/java/dev/rosewood/rosestacker/manager/ConfigurationManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,10 @@ public enum Setting implements RoseSetting {
MISC_MYTHICMOBS_ALLOW_STACKING("misc-settings.mythicmobs-allow-stacking", false, "Should mobs owned by MythicMobs be allowed to stack?", "This is recommended to keep set to false unless you specifically only change mob attributes"),
MISC_SPAWNER_PERSISTENT_COMPATIBILITY("misc-settings.spawner-persistent-compatibility", true, "Some plugins like Jobs, mcMMO, and RoseLoot store special data for spawner mobs.", "Disabling this will cause the functionality within those plugins to break."),
MISC_STACK_STATISTICS("misc-settings.stack-statistics", true, "Should statistics be accurately tracked for stacked entities?", "This can cause issues if you expect players to kill multiple billion mobs"),
PERFORMANCE_SETTINGS("performance-settings", null, "Plugin behavior based on server performance, and other performance tweaks"),
PERFORMANCE_TPS_TOGGLE("performance-settings.tps-toggle.enabled", false, "Should stacking be automatically disabled or enabled based on server TPS?", "Stacks created during periods of low TPS will remain stacked"),
PERFORMANCE_TPS_ENABLE_BELOW("performance-settings.tps-toggle.enable-below", 16D, "When should we enable the stacking?", "Should be lower than re-enable-above. Stacking will remain enabled until disable-above is reached"),
PERFORMANCE_TPS_DISABLE_ABOVE("performance-settings.tps-toggle.disable-above", 18D, "When should we disable the stacking?"),
;

private final String key;
Expand Down
39 changes: 32 additions & 7 deletions Plugin/src/main/java/dev/rosewood/rosestacker/manager/StackManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,10 @@
import dev.rosewood.rosestacker.stack.settings.EntityStackSettings;
import dev.rosewood.rosestacker.stack.settings.SpawnerStackSettings;
import dev.rosewood.rosestacker.utils.DataUtils;
import java.util.Arrays;
import java.util.Collection;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.UUID;

import java.util.*;
import java.util.concurrent.ConcurrentHashMap;

import org.bukkit.Bukkit;
import org.bukkit.Chunk;
import org.bukkit.Location;
Expand All @@ -46,6 +43,8 @@ public class StackManager extends Manager implements StackingLogic {

private boolean isEntityStackingTemporarilyDisabled;
private boolean isEntityUnstackingTemporarilyDisabled;
private boolean isEntityStackingEnabledForPerformance;
private boolean stackingBasedOnPerformance;

private StackedEntityDataStorageType entityDataStorageType;

Expand All @@ -55,6 +54,8 @@ public StackManager(RosePlugin rosePlugin) {
this.stackingThreads = new ConcurrentHashMap<>();

this.isEntityStackingTemporarilyDisabled = false;
this.isEntityStackingEnabledForPerformance = false;
this.stackingBasedOnPerformance = false;
}

@Override
Expand All @@ -70,6 +71,30 @@ public void reload() {
long interval = autosaveFrequency * 20 * 60;
this.autosaveTask = Bukkit.getScheduler().runTaskTimer(this.rosePlugin, () -> this.saveAllData(false), interval, interval);
}

this.stackingBasedOnPerformance = ConfigurationManager.Setting.PERFORMANCE_TPS_TOGGLE.getBoolean();
if(this.stackingBasedOnPerformance){
Bukkit.getServer().getScheduler().runTaskAsynchronously(rosePlugin, new Runnable() {
Copy link
Member

@Esophose Esophose Mar 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why is creating this done in an async task?

@Override
public void run() {
if(ConfigurationManager.Setting.PERFORMANCE_TPS_TOGGLE.getBoolean()){
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why this is being checked again? You checked it above and had a variable assigned to it, but aren't using that variable here.

Bukkit.getScheduler().runTaskTimerAsynchronously(rosePlugin, () -> {
Copy link
Member

Choose a reason for hiding this comment

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

You need to assign async repeating tasks to a variable and ensure they are cancelled when the plugin shuts down. See how the autosave task is handled in this class.

Additionally, as it is now, since this never gets cancelled and gets run every time /rs reload gets run, you can end up with multiple of these tasks running at once, that's bad.

double[] tps = Bukkit.getServer().getTPS();
Copy link
Member

Choose a reason for hiding this comment

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

Server#getTPS() is a Paper-API method and this will throw an error on Spigot servers. I believe it's only possible to check the server TPS using NMS on Spigot servers, so that functionality will need to be added to all the NMSHandler interface and implemented for each handler.

if(tps.length == 0) return;
double lastTPS = tps[0];
// hysteresis
if(isEntityStackingEnabledForPerformance && lastTPS >= Setting.PERFORMANCE_TPS_DISABLE_ABOVE.getDouble()){
// stacking was enabled due to performance, the TPS increased, we can turn it off again
isEntityStackingEnabledForPerformance=false;
} else if(!isEntityStackingEnabledForPerformance && lastTPS <= Setting.PERFORMANCE_TPS_ENABLE_BELOW.getDouble()) {
// stacking was disabled because the performance was above the low bound, but they have decreased past that
isEntityStackingEnabledForPerformance=true;
}
}, 0L, 20L*30L);
}
}
});
}
}

@Override
Expand Down Expand Up @@ -533,7 +558,7 @@ public void setEntityUnstackingTemporarilyDisabled(boolean disabled) {
* @return true if instant entity stacking is temporarily disabled, otherwise false
*/
public boolean isEntityStackingTemporarilyDisabled() {
return this.isEntityStackingTemporarilyDisabled;
return this.isEntityStackingTemporarilyDisabled || (this.stackingBasedOnPerformance && !this.isEntityStackingEnabledForPerformance);
}

/**
Expand Down