12
0

Update2.0 #22

Manuell gemergt
YoyoNow hat 32 Commits von Update2.0 nach master 2020-12-20 13:52:31 +01:00 zusammengeführt
Besitzer

Closes: 23

Closes: 23
Autor
Besitzer

Dieser PullRequest macht es nun möglich Support Items als JSON zu ermöglichen. Alle Support Items (Items, die bis jetzt drin sind) sind auch als JSON schon implementiert (auf dem server eingestellt). Somit sind nun Missiles wie Items gesoftcoded. Ich werde mich noch dran setzten und JavaDoc dazu schreiben, dass jeder weiß wie man es verwendet.

Dieser PullRequest macht es nun möglich Support Items als JSON zu ermöglichen. Alle Support Items (Items, die bis jetzt drin sind) sind auch als JSON schon implementiert (auf dem server eingestellt). Somit sind nun Missiles wie Items gesoftcoded. Ich werde mich noch dran setzten und JavaDoc dazu schreiben, dass jeder weiß wie man es verwendet.
Besitzer

Wurde das ganze getestet (erfolgreich)?

Warum ist das als JSON implementiert und nicht, wie alles andere auch, als YAML?
Mit YAML hättest du zusätzlich die Möglichkeit, die Items und Skripte direkt zu (de-)serialisieren und damit wesentlich weniger Code-Aufwand.

Wurde das ganze getestet (erfolgreich)? Warum ist das als JSON implementiert und nicht, wie alles andere auch, als YAML? Mit YAML hättest du zusätzlich die Möglichkeit, die Items und Skripte direkt zu (de-)serialisieren und damit wesentlich weniger Code-Aufwand.
Autor
Besitzer

Ja dies habe ich erfolgreich in auf meinem Bau Test Server getestet. Ich habe am Anfang kurz das ganze mal in YAML überlegt und auch ein wenig getestet, fande jedoch, dass das echt nicht schön zu schreiben war und auch für neue sachen nicht gerade es einfach gemacht hat was zu "Scripten", weswegen ich dann nach einiger Zeit mich für JSON entschieden habe. Ich finde auch, dass man ganz gut damit klar kommt und es gefühlt einfacher zu lesen ist, aber das kann auch nur subjektiv von mir kommen.

Ja dies habe ich erfolgreich in auf meinem Bau Test Server getestet. Ich habe am Anfang kurz das ganze mal in YAML überlegt und auch ein wenig getestet, fande jedoch, dass das echt nicht schön zu schreiben war und auch für neue sachen nicht gerade es einfach gemacht hat was zu "Scripten", weswegen ich dann nach einiger Zeit mich für JSON entschieden habe. Ich finde auch, dass man ganz gut damit klar kommt und es gefühlt einfacher zu lesen ist, aber das kann auch nur subjektiv von mir kommen.
Chaoscaot hat 2020-11-01 10:22:38 +01:00 Änderungen angefragt
@ -0,0 +34,4 @@
import java.io.IOException;
import java.util.Objects;
public class Item extends SpecialItem {
Besitzer

Etwas generischer Name.

Etwas generischer Name.
@ -0,0 +39,4 @@
private ScriptedItem scriptedItem;
public Item(File item) {
try {
Besitzer

Warum kann der kein JSONObject bekommen? Dann müsste der Try-Catch Block nur an einer Stelle sein.

Warum kann der kein JSONObject bekommen? Dann müsste der Try-Catch Block nur an einer Stelle sein.
@ -0,0 +41,4 @@
public Item(File item) {
try {
JsonObject jsonObject = new JsonParser().parse(new FileReader(item)).getAsJsonObject();
System.out.println(jsonObject);
Besitzer

Debug Nachricht?

Debug Nachricht?
@ -0,0 +33,4 @@
}
class RunnableScriptEvent {
Besitzer

Kann man dem nicht eine Eigene klasse geben?

Kann man dem nicht eine Eigene klasse geben?
@ -0,0 +49,4 @@
public Location getLocation() {
// Custom location
if (locationType == LocationType.CUSTOM && customLocation != null) {
Besitzer

Warum kann hier kein Switch Statement sein? Wäre glaube ich schöner an der Stelle.

Warum kann hier kein Switch Statement sein? Wäre glaube ich schöner an der Stelle.
Autor
Besitzer

Ich glaube dies nicht, weil es immer den Fallback auf Default geben soll. Somit ist es linearer

Ich glaube dies nicht, weil es immer den Fallback auf Default geben soll. Somit ist es linearer
@ -0,0 +73,4 @@
}
public void setLocationType(LocationType locationType) {
if (locationType == null) return;
Besitzer

Dieser Check ist Doppelt, LocationScript z. 68.

Dieser Check ist Doppelt, LocationScript z. 68.
@ -0,0 +40,4 @@
return null;
}
String type = jsonObject.getAsJsonPrimitive("type").getAsString();
switch (type.toLowerCase()) {
Besitzer

Könnte man nicht einen Enum, welcher ein RunnableScript speichert, und da dann mit dem Enum Namen drüber Iteriert?

Könnte man nicht einen Enum, welcher ein RunnableScript speichert, und da dann mit dem Enum Namen drüber Iteriert?
Autor
Besitzer

geht nicht, weil die dann statisch währen und man enum values nicht überschreiben sollte

geht nicht, weil die dann statisch währen und man enum values nicht überschreiben sollte
@ -0,0 +43,4 @@
public enum EventType {
onHit("onHit"),
onThrow("onThrow"),
onClick("onClick");
Besitzer

Warum werden dieser String Parameter benötigt, und kann man das nicht name() machen?

Warum werden dieser String Parameter benötigt, und kann man das nicht ```name()``` machen?
Autor
Besitzer

kann man, aber der Enum name muss nicht mit dem Event Namen übereinstimmen

kann man, aber der Enum name muss nicht mit dem Event Namen übereinstimmen
Besitzer

Tut es doch jetzt schon.

Tut es doch jetzt schon.
@ -0,0 +85,4 @@
if (jsonObject.has("lore")) {
List<String> lore = new ArrayList<>();
jsonObject.getAsJsonArray("lore").forEach(jsonElement -> {
if (!jsonElement.isJsonPrimitive()) {
Besitzer

Warum ist dieser Check nötig?

Warum ist dieser Check nötig?
Autor
Besitzer

Weil man auch die Lore weglassen kann, diese ist somit optional

Weil man auch die Lore weglassen kann, diese ist somit optional
@ -0,0 +63,4 @@
try {
clipboard = Objects.requireNonNull(ClipboardFormats.findByFile(schemFile)).getReader(new FileInputStream(schemFile)).read();
} catch (IOException e) {
throw new SecurityException("Could not load shield", e);
Besitzer

Warum konnte er da das Schild nicht laden? Das wird doch von mehr Sachen als nur das Schild was etwas Pastet.

Warum konnte er da das Schild nicht laden? Das wird doch von mehr Sachen als nur das Schild was etwas Pastet.
@ -0,0 +36,4 @@
private Summon summon = null;
public SummonScript(JsonObject summon) {
switch (summon.getAsJsonPrimitive("entity").getAsString().toLowerCase()) {
Besitzer

Warum ist hier dann ein Switch und kein If?

Warum ist hier dann ein Switch und kein If?
Autor
Besitzer

Um es später noch zu erweitern

Um es später noch zu erweitern
Chaoscaot hat 2020-11-01 10:58:24 +01:00 Änderungen angefragt
@ -0,0 +1,10 @@
package de.steamwar.misslewars.scripts;
Besitzer

License Header Fehlt

License Header Fehlt
@ -0,0 +1,58 @@
package de.steamwar.misslewars.scripts;
Besitzer

License Header Fehlt

License Header Fehlt
@ -0,0 +19,4 @@
package de.steamwar.misslewars.scripts;
import com.google.gson.JsonArray;
Besitzer

Unnötiger Import

Unnötiger Import
@ -0,0 +1,77 @@
package de.steamwar.misslewars.scripts.implemented;
Besitzer

License Header Fehlt

License Header Fehlt
Chaoscaot hat 2020-11-01 11:10:24 +01:00 Änderungen angefragt
@ -0,0 +45,4 @@
}
private void resume() {
while (index < runnableScriptList.size()) {
Besitzer

Wäre hier nicht eher eine For oder Foreach Schleife angebracht.
?

Wäre hier nicht eher eine For oder Foreach Schleife angebracht. ?
Autor
Besitzer

Nein weil sie auch abgebrochen werden kann mit dem Delay und danach wieder angefangen wird. Somit ist beides keine Option.

Nein weil sie auch abgebrochen werden kann mit dem Delay und danach wieder angefangen wird. Somit ist beides keine Option.
@ -0,0 +49,4 @@
int blockZ = location.getBlockZ();
if (offset > 0) {
if (blockZ > bz - offset) return true;
if (blockZ < rz + offset) return true;
Besitzer

If Else oder ||?

If Else oder ||?
@ -0,0 +52,4 @@
if (blockZ < rz + offset) return true;
} else {
if (blockZ < bz - offset) return true;
if (blockZ > rz + offset) return true;
Besitzer

Selbiges.

Selbiges.
Besitzer

Ich denke, dass die Menge an Code + die Komplexität des Codes für die gewünschte Aufgabe weitaus zu viel ist. Du hast 1270 neue Zeilen, während du nur 270 alte dadurch weg wirst. Welche großen dynamischen Änderungen sind denn in nächster Zeit an MW zu erwarten?

Ich halte das ganze a) viel zu viel Overhead für etwas, was kaum benötigt werden wird und b) die Implementierung der Idee viel zu lang, bitte die benötigten Codezeilen und Komplexität dafür deutlich einstampfen, dass ist einfach schlecht wartbar, wenn ein kleines Feature so komplex ist.

Ich denke, dass die Menge an Code + die Komplexität des Codes für die gewünschte Aufgabe weitaus zu viel ist. Du hast 1270 neue Zeilen, während du nur 270 alte dadurch weg wirst. Welche großen dynamischen Änderungen sind denn in nächster Zeit an MW zu erwarten? Ich halte das ganze a) viel zu viel Overhead für etwas, was kaum benötigt werden wird und b) die Implementierung der Idee viel zu lang, bitte die benötigten Codezeilen und Komplexität dafür deutlich einstampfen, dass ist einfach schlecht wartbar, wenn ein kleines Feature so komplex ist.
Autor
Besitzer

Du möchtest doch sonst auch immer vieles softcoded haben, sodass man das einfach anpassen kann. Und deswegen habe ich mir gedacht, dass ich das ganze auch auf MissileWars anwende und mich auch hier darum kümmer, dass dies so wird.

Diese Änderung würde definitiv das erstellen von neuen Items einfacher machen und auch ohne viel Programmierkenntnise ermöglicht werden. Auch finde ich, dass man einfacher so Items und Balance Änderungen ermöglichen kann, ohne, dass man immer eine Code analyse machen muss.

Des weiteren kann man auch auf dem Dev Server dann mal eben ein neues Item testen, ohne den Code zu ändern oder Items anpassen/balancen ohne immer neu zu kompilieren.

Du möchtest doch sonst auch immer vieles softcoded haben, sodass man das einfach anpassen kann. Und deswegen habe ich mir gedacht, dass ich das ganze auch auf MissileWars anwende und mich auch hier darum kümmer, dass dies so wird. Diese Änderung würde definitiv das erstellen von neuen Items einfacher machen und auch ohne viel Programmierkenntnise ermöglicht werden. Auch finde ich, dass man einfacher so Items und Balance Änderungen ermöglichen kann, ohne, dass man immer eine Code analyse machen muss. Des weiteren kann man auch auf dem Dev Server dann mal eben ein neues Item testen, ohne den Code zu ändern oder Items anpassen/balancen ohne immer neu zu kompilieren.
Besitzer

Ich softcode da, wo es Sinn macht. Die simple Frage ist halt: Wie häufig müssen neue Items erstellt/alte Items gebalanct werden? Oder, um es noch weiter herunterzubrechen: Macht dieses Feature mehr oder weniger Aufwand?

Ich softcode da, wo es Sinn macht. Die simple Frage ist halt: Wie häufig müssen neue Items erstellt/alte Items gebalanct werden? Oder, um es noch weiter herunterzubrechen: Macht dieses Feature mehr oder weniger Aufwand?
Autor
Besitzer

Ich glaube, dass es weniger aufwand macht, da es den Prozess des Item Erstellen vereinfacht und vom Code loslöst. Des weiteren so die Idee (primär von MaybeCreative) ungefähr alle 2 Wochen (darf muss aber nicht Zitat: MaybeCreative) ein Update zu machen. Dies kann beinhalten, dass Items geändert werden, gelöscht werden, gebalancet werden, Missiles hinzukommen, gelöscht werden, angepasst werden. Dementsprechend glaube ich, dass dieses Feature uns das Leben einfacher macht und der Aufwand minimiert wird.

Ich glaube, dass es weniger aufwand macht, da es den Prozess des Item Erstellen vereinfacht und vom Code loslöst. Des weiteren so die Idee (primär von MaybeCreative) ungefähr alle 2 Wochen (darf muss aber nicht Zitat: MaybeCreative) ein Update zu machen. Dies kann beinhalten, dass Items geändert werden, gelöscht werden, gebalancet werden, Missiles hinzukommen, gelöscht werden, angepasst werden. Dementsprechend glaube ich, dass dieses Feature uns das Leben einfacher macht und der Aufwand minimiert wird.
Besitzer

YoyoNow, ich glaube, wir haben gerade einfach mit unter anderem das Problem, dass sich unsere Führungsstile in die Wolle mit mir als "Anführer" und du als "designiertem Nachfolger" bekommen. Ich würde vorschlagen, dass wir einfach mal die Projekte/Plugins und wer darüber das aus technischer Sicht letzte Wort darüber hat mal aufteilen.

Da würde ich vorschlagen, dass du MissileWars, LobbySystem und BauSystem übernimmst.

Dann muss ich mich langsam daran gewöhnen, in den Projekten das letzte Wort dir zu überlassen (was nicht heißt, dass ich nicht mehr meine Meinung kundtun werde!) :).

Was hälst du davon?

YoyoNow, ich glaube, wir haben gerade einfach mit unter anderem das Problem, dass sich unsere Führungsstile in die Wolle mit mir als "Anführer" und du als "designiertem Nachfolger" bekommen. Ich würde vorschlagen, dass wir einfach mal die Projekte/Plugins und wer darüber das aus technischer Sicht letzte Wort darüber hat mal aufteilen. Da würde ich vorschlagen, dass du MissileWars, LobbySystem und BauSystem übernimmst. Dann muss ich mich langsam daran gewöhnen, in den Projekten das letzte Wort dir zu überlassen (was nicht heißt, dass ich nicht mehr meine Meinung kundtun werde!) :). Was hälst du davon?
Autor
Besitzer

Ich glaube, dass die Aufteilung, besonders was die Änderungen (wer wo primär arbeitet) angeht eine gute Idee ist. Ich möchte definitiv weiterhin deine Meinung dabei haben und auch diese bekommen. Ich finde es nämlich immer wieder interessant deine Meinung und auch Ideen zu hören und drüber nachzudenken. Ich hoffe, dass meine Meinung auch in anderen Bereichen Willkommen ist und zum nachdenken anregt.

Des weiteren würde ich gerne noch sagen, dass die Idee hier dran einfach war, Code (technische Umsetzung) von Implementierung zu trennen. Soll heißen, dass ich einfach den Code haben möchte, der es managed, und dann die Idee (Style sozusagen) von woanders kommt.

Ich glaube, dass die Aufteilung, besonders was die Änderungen (wer wo primär arbeitet) angeht eine gute Idee ist. Ich möchte definitiv weiterhin deine Meinung dabei haben und auch diese bekommen. Ich finde es nämlich immer wieder interessant deine Meinung und auch Ideen zu hören und drüber nachzudenken. Ich hoffe, dass meine Meinung auch in anderen Bereichen Willkommen ist und zum nachdenken anregt. Des weiteren würde ich gerne noch sagen, dass die Idee hier dran einfach war, Code (technische Umsetzung) von Implementierung zu trennen. Soll heißen, dass ich einfach den Code haben möchte, der es managed, und dann die Idee (Style sozusagen) von woanders kommt.
Besitzer

Wie schaut es hier mit aus @YoyoNow ?

Wie schaut es hier mit aus @YoyoNow ?
Chaoscaot hat den Titel von Update2.0 zu WIP: Update2.0 2020-12-18 23:06:41 +01:00 geändert
YoyoNow hat den Titel von WIP: Update2.0 zu Update2.0 2020-12-19 14:11:32 +01:00 geändert
Chaoscaot hat 2020-12-19 20:52:42 +01:00 Änderungen angefragt
@ -85,1 +89,4 @@
BlueSpawn = new Location(Bukkit.getWorlds().get(0), blue.getDouble("SpawnX"), blue.getDouble("SpawnY"), blue.getDouble("SpawnZ"), (float)blue.getDouble("SpawnYaw"), (float)blue.getDouble("SpawnPitch"));
String blueLeader = System.getProperty("blueLeader", null);
String redLeader = System.getProperty("redLeader", null);
Besitzer

In den derzeit geplanten änderungen zum Server System sollten wir eher weg von System Arguementen

In den derzeit geplanten änderungen zum Server System sollten wir eher weg von System Arguementen
Autor
Besitzer

Gut wie soll ich es sonst lösen, so ist es im FightSystem!

Gut wie soll ich es sonst lösen, so ist es im FightSystem!
Besitzer

Da ist dann leider auch eine Änderung im BungeeCore und FightSystem nötig, da brauchen wir noch überhaupt ein Konzept, wie wir dann solche Argumente überhaupt auf den Server bekommen. Evtl. indem wir Configs anpassbar machen. Ich denke aber nicht, dass wir da hier jetzt schon mit dem Umbau beginnen sollten.

Da ist dann leider auch eine Änderung im BungeeCore und FightSystem nötig, da brauchen wir noch überhaupt ein Konzept, wie wir dann solche Argumente überhaupt auf den Server bekommen. Evtl. indem wir Configs anpassbar machen. Ich denke aber nicht, dass wir da hier jetzt schon mit dem Umbau beginnen sollten.
@ -19,6 +19,7 @@
package de.steamwar.misslewars;
import de.steamwar.misslewars.items.Missile;
Besitzer

Unused Import

Unused Import
@ -0,0 +24,4 @@
import de.steamwar.misslewars.Config;
import de.steamwar.misslewars.MWTeam;
import de.steamwar.misslewars.MissileWars;
import de.steamwar.misslewars.items.Missile;
Besitzer

Unused Import

Unused Import
@ -0,0 +29,4 @@
import org.bukkit.command.CommandSender;
import org.bukkit.entity.Player;
public class CommandInviteHandler implements CommandExecutor {
Besitzer

Unpassender Name für /accept und /decline

Unpassender Name für /accept und /decline
@ -0,0 +63,4 @@
}
public static void init() {
File itemsFolder = new File(MissileWars.getPlugin().getDataFolder(), "items");
Besitzer

Sollte in /configs liegen

Sollte in /configs liegen
Autor
Besitzer

Liegen die Missiles auch nicht. Also nein!

Liegen die Missiles auch nicht. Also nein!
@ -155,9 +147,7 @@ public class Missile extends SpecialItem {
public static void init() {
File missileFolder = new File(MissileWars.getPlugin().getDataFolder(), "missiles");
Besitzer

Sollte auch /configs liegen

Sollte auch /configs liegen
Autor
Besitzer

Siehe Oben nein. War schon immer so. Und Außerdem ist auch dort liegen auch die Shield Schem und so. Deswegen sollte das wohl einfach so bleiben!

Siehe Oben nein. War schon immer so. Und Außerdem ist auch dort liegen auch die Shield Schem und so. Deswegen sollte das wohl einfach so bleiben!
Besitzer

Das wird wenn versymlinkt (sobald mehrere Configs gleich sind). Da kann man auch mal umbauen, aber das Plugin sollte nix mit /configs zu tun haben.

Das wird wenn versymlinkt (sobald mehrere Configs gleich sind). Da kann man auch mal umbauen, aber das Plugin sollte nix mit /configs zu tun haben.
Autor
Besitzer

Chaos wie steht es um dein Review?

Chaos wie steht es um dein Review?
Besitzer

Ich würde dich beten, dass du diesen PR in kleinere Teile aufteilst, weil 1200 Zeilen ist doch schon ein bissle viel.

Ich würde dich beten, dass du diesen PR in kleinere Teile aufteilst, weil 1200 Zeilen ist doch schon ein bissle viel.
Chaoscaot hat die Änderungen 2020-12-20 13:48:00 +01:00 genehmigt
Chaoscaot hat einen Kommentar hinterlassen
Besitzer

Immer noch nicht nötig. Alle angaben ohne gewähr!

Immer noch nicht nötig. Alle angaben ohne gewähr!
@ -0,0 +66,4 @@
File itemsFolder = new File(MissileWars.getPlugin().getDataFolder(), "items");
if (!itemsFolder.exists() || !itemsFolder.canRead() || !itemsFolder.isDirectory()) throw new SecurityException("Items could not be loaded");
for (File itemFile : Objects.requireNonNull(itemsFolder.listFiles())) {
if (!itemFile.canRead() || !itemFile.isFile()) continue;
Besitzer

Vielleicht sollte man hier einen Fehler werfen.

Vielleicht sollte man hier einen Fehler werfen.
Autor
Besitzer

Ist bei den Missiles auch nicht, dies habe ich einfach kopiert

Ist bei den Missiles auch nicht, dies habe ich einfach kopiert
@ -33,3 +34,4 @@
public class DeathListener extends BasicListener {
private static Vector ZERO = new Vector(0, 0, 0);
Besitzer

Könnte Final sein

Könnte Final sein
YoyoNow hat diesen Pull-Request 2020-12-20 13:52:31 +01:00 geschlossen
YoyoNow löschte die Branch Update2.0 2020-12-20 13:52:46 +01:00
Anmelden, um an der Diskussion teilzunehmen.
Keine Beschreibung angegeben.