Add basic sleep command to ScriptSystem #142
Keine Reviewer
Label
Kein Label
Bug
Codeverbesserung
Einsteiger Freundlich
Idee
In Arbeit
Neues Feature
Prio A
Security Breach
Überprüfung notwendig
Verbesserung
Zu Beobachten
Kein Meilenstein
Niemand zuständig
2 Beteiligte
Fällig am
Kein Fälligkeitsdatum gesetzt.
Abhängigkeiten
Keine Abhängigkeiten gesetzt.
Referenz: SteamWar/BauSystem#142
Laden…
In neuem Issue referenzieren
Einen Benutzer sperren
Keine Beschreibung angegeben.
Branch "ScriptSleep" löschen
Das Löschen eines Branches ist permanent. Obwohl der Branch für eine kurze Zeit weiter existieren könnte, kann diese Aktion in den meisten Fällen NICHT rückgängig gemacht werden. Fortfahren?
@ -38,3 +42,1 @@
public void onLeftClick(PlayerInteractEvent event) {
if(event.getAction() != Action.LEFT_CLICK_AIR && event.getAction() != Action.LEFT_CLICK_BLOCK)
return;
private Map<BookMeta, Script> scriptMap = new HashMap<>();
Wozu musst du dir die Scripte speichern? Ist glaube ich unnötig.
@ -49,3 +46,1 @@
for(String page : meta.getPages()){
for(String command : page.split("\n")){
if (command.startsWith("#")) continue;
private BookMeta bookMeta;
Wozu braucht das Skript die BookMeta?
@ -50,2 +46,2 @@
for(String command : page.split("\n")){
if (command.startsWith("#")) continue;
private BookMeta bookMeta;
private List<String> commands = new ArrayList<>();
LinkedList wäre bei Arbeit mit entfernen besser geeignet.
Ich entferne ja absolut nichts.
YoyoNow:
@ -51,1 +46,3 @@
if (command.startsWith("#")) continue;
private BookMeta bookMeta;
private List<String> commands = new ArrayList<>();
private int uses = 0;
Wozu müssen die uses gespeichert werden? Fire and forget!
@ -52,0 +58,4 @@
}
}
public void keep() {
KISS: Lass das weg.
@ -52,0 +62,4 @@
uses++;
}
public void free() {
KISS: Lass das weg.
@ -52,0 +71,4 @@
}
private class ScriptExecutor {
ScriptExecutor mit Script mergen, gibt keinen Grund das zu trennen.
@ -52,0 +75,4 @@
private Player player;
private Script script;
private int index = 0;
Nicht mit einem index arbeiten, sondern die abgearbeiteten Commands aus der Liste löschen.
Dies ist extra so, um weitere Features einzubauen, die das Turing Complete machen würden.
@ -52,0 +87,4 @@
script.keep();
}
public void start() {
Verwendest du nie, und solltest du bei Arbeit nur mit Skript nicht brauchen (im Konstruktor einfach starten)
@ -52,0 +91,4 @@
resume();
}
private void resume() {
Fehlend: Überprüfung, ob der Spieler überhaupt noch online ist, ansonsten könnten da schnell Fehler fliegen.
@ -52,0 +95,4 @@
while (index < script.commands.size()) {
String command = script.commands.get(index++);
if (command.startsWith("sleep")) {
CaSeInDePeNdEnCe
@ -52,0 +96,4 @@
String command = script.commands.get(index++);
if (command.startsWith("sleep")) {
String sleepTimeString = command.substring(5).trim();
Evtl eher mit String.split() arbeiten (vgl andere Commands)
@ -52,0 +99,4 @@
String sleepTimeString = command.substring(5).trim();
int sleepTime = 1;
try {
Integer.parseInt(sleepTimeString);
Ergebnis wird ignoriert.
Auch dies sollte behoben sein, das habe ich einfach vergessen.
Wann wird der Tracer gefixt?
@ -38,3 +42,1 @@
public void onLeftClick(PlayerInteractEvent event) {
if(event.getAction() != Action.LEFT_CLICK_AIR && event.getAction() != Action.LEFT_CLICK_BLOCK)
return;
private static class ScriptExecutor {
Subklassen kommen üblicherweise erst nach den Methoden der eigenen Klassen im Quelltext
@ -43,2 +44,2 @@
if(item == null || isNoBook(item) || item.getItemMeta() == null)
return;
private Player player;
private List<String> commands = new ArrayList<>();
player & commands sind final
@ -52,0 +59,4 @@
private void resume() {
if (!player.isOnline()) {
player = null;
Du musst hier nix auf null setzen, nach Ablauf der Funktion wird dieses Objekt sowieso nirgends mehr referenziert.
@ -52,0 +68,4 @@
String command = commands.get(index++);
String[] strings = command.split(" ");
if (strings.length > 0) {
Befehle ohne Leerzeichen werden nicht mehr ausgeführt.
@ -52,0 +73,4 @@
int sleepTime = 1;
if (strings.length > 1) {
try {
sleepTime = Integer.parseInt(strings[1]);
Negative und 0 sleepTimes incoming
@ -52,0 +75,4 @@
try {
sleepTime = Integer.parseInt(strings[1]);
} catch (NumberFormatException e) {
// Ignored
Ne Fehlermeldung wäre schon ganz nice, wenn man da was nichtnumerisches angegeben hat.
@ -52,0 +77,4 @@
} catch (NumberFormatException e) {
// Ignored
}
}
Hast du das überhaupt mal getestet (und trotzdem als fertigen PR markiert)? Der einzige Befehl, den das Skriptsystem so interpretiert, ist dein Sleep-Command!
@ -62,0 +106,4 @@
event.setCancelled(true);
Player player = event.getPlayer();
BookMeta meta = (BookMeta) item.getItemMeta();
Das casten und getPlayer() kann man auch beim Aufruf des Konstruktors machen, kein Bedarf für diese zusätzlichen Throw-Away-Referenzen
Soll es so sein, dass Scripte auch beim Öffnen ausgeführt werden?
Beim Öffnen des Buches? Nein, eigentlich nicht, allerdings scheint das trotzdem irgendwie als LEFT_CLICK_AIR oder LEFT_CLICK_BLOCK gewertet zu werden. Wenn du da ein besseres Event findest: Go for it!
Wenn du die Luft rechts klickst dann wird dies als left click gezählt. Auf einem Block tut es einwandfrei
@ -74,0 +73,4 @@
this.player = player;
for(String page : bookMeta.getPages()) {
for (String command : page.split("\n")) {
Bitte auch leere Zeilen rausfiltern (kein gültiger Befehl).
@ -74,0 +89,4 @@
while (index < commands.size()) {
String command = commands.get(index++);
if (command.contains(" ")) {
Bugs inc! Prüfe bitte ZUERST, ob der Befehl mit SlEEp anfängt, und dann kümmere dich um die Sleep-abarbeitung, und nicht zuerst, ob da ein space drinist!
Und wie soll ich bitte case insensitive das ganze checken mit einer regex?
command.toLowerCase().startsWith()
Auch dies sollte behoben sein!
@ -74,0 +95,4 @@
if (strings[0].equalsIgnoreCase("sleep")) {
int sleepTime = 1;
if (strings.length > 1) {
try {
Woho! Die 9. offene Klammer! Bester Programmierstil!
Hab ich behoben
@ -74,0 +106,4 @@
}
}
Bukkit.getScheduler().runTaskLater(BauSystem.getPlugin(), this::resume, sleepTime);
break;
return;
Dann ist der Programmfluss klarer.
@ -74,0 +113,4 @@
PlayerCommandPreprocessEvent preprocessEvent = new PlayerCommandPreprocessEvent(player, "/" + command);
Bukkit.getServer().getPluginManager().callEvent(preprocessEvent);
if (preprocessEvent.isCancelled()) {
Das macht wenn schon der PreprocessEventHandler, lass das hier raus!
Sollten wir nicht ab dem ersten Befehl der nicht ausführbar ist abbrechen?
Dann machst du einfach ein Return, und erzählst dem User keinen Schmarrn!
Das problem ist: Ob der eigentliche Befehl ausgeführt wurde (das kommt ja erst drunter, das ist nur die Rechteüberprüfung für WE-Befehle (mehr oder weniger)) können wir als Befehlsdispatcher gar nicht feststellen. Daher: Das Skript läuft immer durch, egal wie viele Befehle davon "schiefgehen"
YoyoNow, deshalb: Die Nutzung eines anderen Events wäre ggf. besser (wenn es ein passenderes gibt)
Ok ich schaffe es, dass ich alles, was mit der Luft zu tun hat nun wegcancel? richtig komisch.
Ich jetzt bekomme ich sowohl ein RightClickAir und LeftClickAir wenn ich RightClickAir ausführe. Das ist absolut komisch, was dahinter steckt!
Von der Abstrahierung des ScriptCommands halte ich nichts, da es neben sleep keinen Skriptcommand gibt und auch keiner geplant ist. Verzweigungen & Schleifen sind KEINE Commands, diese benötigen nämlich andere Datenstrukturen. (Ansonsten kommst du schnell in Probleme durch verzweigten Verzweigungskonstrukten.
Bitte wieder ausbauen, die Komplexität tut dem Skriptsystem nicht gut.
Ich hätte damit noch ein 'exit' befehl eingebaut, der einfach die exection stoppt. Deswegen hätte ich das so gebaut.
Overhead ist für 2 Befehle zu groß (mMn - meiner Meinung nach)
Ich glaube dann sollte es jetzt nach deinen ansprüchen sein mit dem Sleep Befehl!
@ -35,3 +41,3 @@
public class ScriptListener implements Listener {
@EventHandler
private static final String scriptPrefix = "§eScriptSystem§8» §7";
Nutze wenn bitte den BauSystem-Prefix
Warum das, das ist ja explizit das ScripSystem
Nein, das ist hier immer noch das BauSystem.
@ -37,1 +43,3 @@
@EventHandler
private static final String scriptPrefix = "§eScriptSystem§8» §7";
private Set<Player> playerSet = new HashSet<>();
Sollte ein Spieler den Server verlassen oder das Item in der Hand wechseln, musst du ihn auch hier rauswerfen. Evtl. ist es einfacher, alle Tick das zu clearen? (Je nachdem, wie die zeitlich getimet sind?)
@ -38,0 +44,4 @@
private Set<Player> playerSet = new HashSet<>();
@EventHandler(priority = EventPriority.HIGH)
Lass das HIGH da raus, das hat da nix verloren.
Das muss so, weil bei Normal in den RightClickAir nicht mehr bekomme. Dies habe ich getestet. Dies ist ein Bukkit Feature
Dafür möchte ich ne Quelle, weil das scheint mir aufgrunddessen, wie Events abgearbeitet werden, absolut unwahrscheinlich.
https://bukkit.org/threads/right-click-air-not-working.103210/
Von Malikk, Sep 30, 2012
etwas weiter unten
Das ist halt auch nur eine Meinung, ne Ursachenbegründung ist das nicht.
@ -38,2 +47,3 @@
@EventHandler(priority = EventPriority.HIGH)
public void onLeftClick(PlayerInteractEvent event) {
if(event.getAction() != Action.LEFT_CLICK_AIR && event.getAction() != Action.LEFT_CLICK_BLOCK)
if (event.getAction() != Action.LEFT_CLICK_AIR && event.getAction() != Action.LEFT_CLICK_BLOCK) {
Umfassend getestet mit allen möglichen Aktionen, unterbindet es alle ungewollten aktivierungen und lässt noch alle gewollten Aktivierungen durch?
Derzeit dürfte das Bugs geben, wenn du z.B. zwischendurch durch ein Tripwire läufst oder ne Druckplatte drückst.
@ -41,3 +57,4 @@
}
ItemStack item = event.getItem();
if(item == null || isNoBook(item) || item.getItemMeta() == null)
Bitte wenn erst diese simple überprüfung machen, und DANN erst die ungewollten aktivierungen unterbinden
@ -52,0 +86,4 @@
for(String page : bookMeta.getPages()) {
for (String command : page.split("\n")) {
if (command.startsWith("#")) continue;
if (command.trim().isEmpty()) continue;
Bitte ifs verodern und zusammenfassen!
@ -71,0 +124,4 @@
return fullCommand.substring(command.length() + 1).split(" ");
}
private static void sleepCommand(ScriptExecutor scriptExecutor, String[] args) {
Da wäre es Sinnvoller, einfach den player zu übergeben, was will sleep mit dem ScriptExecutor?
Ihn resumen. ganz unten
@ -71,0 +130,4 @@
try {
sleepTime = Integer.parseInt(args[0]);
if (sleepTime <= 0) {
scriptExecutor.player.sendMessage(scriptPrefix + "Sleep kleiner gleich 0, default 1 GameTick");
Das kannst du so keinem Spieler vorwerfen, bitte überarbeiten.
Wie sollte ich diese Nachricht denn dann überarbeiten
Hirn an und dem Nutzer eine für ihn informative Nachricht in einer ihm verständlichen Sprache, Art und Weise senden.
@ -71,0 +134,4 @@
sleepTime = 1;
}
} catch (NumberFormatException e) {
scriptExecutor.player.sendMessage(scriptPrefix + "Sleep ohne Zahl, default 1 GameTick");
Gleichfalls.
Dann sollte dies nun auch behoben sein.
@ -71,0 +133,4 @@
try {
sleepTime = Integer.parseInt(args[0]);
if (sleepTime <= 0) {
scriptExecutor.player.sendMessage(BauSystem.PREFIX + "Eine Sleep zeit von kleiner gleich 0 ist nicht erlaubt. Der default 1 Tick wird verwendet.");
Deutsch!
@ -71,0 +137,4 @@
sleepTime = 1;
}
} catch (NumberFormatException e) {
scriptExecutor.player.sendMessage(BauSystem.PREFIX + "Eine Sleep zeit sollte keine Buchstaben oder sonstige Zeiten verwenden. Der default 1 Tick wird verwendet.");
Deutsch!
Ich glaube das ist jetzt besseres Deutsch
Ne, nicht wirklich. lass mal sleep raus, und default interessiert den Nutzer auch nicht.
So?
sollte ist von der Formulierung nicht ganz richtig... => muss
Farbcodes nicht vergessen, ansonsten schaut das jetzt schon langsam gut aus.
Gut dann habe ich das nun auch noch gemacht
@ -71,0 +135,4 @@
sleepTime = 1;
}
} catch (NumberFormatException e) {
scriptExecutor.player.sendMessage(BauSystem.PREFIX + "§cDie Zeit muss nur aus Zahlen bestehen.");
darf
Unbedingt nochmal vor Mergen testen.