Arrows Stopping in Techhider Blocks #208
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/FightSystem#208
Laden…
In neuem Issue referenzieren
Einen Benutzer sperren
Keine Beschreibung angegeben.
Branch "arrow-in-techhider" 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?
Closes: #99
@ -0,0 +26,4 @@
@SuppressWarnings("deprecation")
static int blockToId(Block block){
return block.getTypeId() << 4 + block.getData();
In der 1.12 ist in Config.HiddenBlocks nur block.getTypeId() ohne << 4 + getData()
@ -0,0 +71,4 @@
task.cancel();
}
private static int blockToId(Block block){
Eine solche Funktionalität sollte bereits mit dem Techhider bzw. RecordSystem enthalten sein, wenn würde ich diese generalisieren.
@ -0,0 +21,4 @@
import org.bukkit.block.Block;
public class ArrowStopper_12 {
Das müsste ArrowStopper_8 sein (kleinste unterstützte Version)
@ -0,0 +22,4 @@
import org.bukkit.block.Block;
import org.bukkit.craftbukkit.v1_15_R1.block.CraftBlock;
public class ArrowStopper_15 {
Das müsste ArrowStopper_14 sein (kleinste unterstützte Version)
@ -0,0 +52,4 @@
return;
for (Entity entity : arrows) {
if(entity.getTicksLived() > Config.ArrowTechhiderCollision ||
((AbstractArrow) entity).isInBlock()){
Landen arrows wirklich in Blöcken?
@ -0,0 +55,4 @@
((AbstractArrow) entity).isInBlock()){
LAST_LOCATION.remove(entity);
continue;
}
Hätte es gerne, dass du zur kognitiven Komplexitätsreduktion einfach am Ende prüfst (beim derzeitigen else), wie lange die Entity schon gelebt hat, und sie dann ggf. einfach nicht mehr einfügst.
@ -0,0 +58,4 @@
}
if(!LAST_LOCATION.containsKey(entity))
LAST_LOCATION.put(entity, ((Player) ((AbstractArrow) entity).getShooter()).getEyeLocation());
Location last = LAST_LOCATION.remove(entity);
Bitte reduziere mal deine Map-Operationen. Optimierungsidee: Du machst LAST_LOCATION.remove(), und wenn du null bekommst, nimmst du deine EyeLocation, 1,5 Mapoperationen weniger.
Ist das schon als Production-Ready angesehen oder noch als WIP angedacht?
@ -0,0 +26,4 @@
private ArrowStopper_14(){}
static int blockToId(Block block){
return net.minecraft.server.v1_14_R1.Block.REGISTRY_ID.getId(((CraftBlock)block).getNMS());
Funktioniert nur für die 1.14, für die 1.15 ist eine separate Klasse erforderlich.
@ -0,0 +40,4 @@
import java.util.*;
public class ArrowStopper implements StateDependent, Listener {
Wenn das ganze StateDependent und Listener ist, wäre es evtl. eleganter, das ganze als listener zu haben, und für den Task dann einfach die enable und disable-Methode überschreiben und die super()-Methode mit aufzurufen.
@ -0,0 +53,4 @@
private void run() {
Collection<Entity> arrows = Bukkit.getWorlds().get(0).getEntitiesByClasses(AbstractArrow.class);
if(arrows.isEmpty())
Auch wenn die arrows Empty sind, sollten die Pfeile aus der LAST_LOCATION entfernt werden (also mehr oder weniger der normale Programmfluss genommen werden), oder?
@ -0,0 +55,4 @@
Collection<Entity> arrows = Bukkit.getWorlds().get(0).getEntitiesByClasses(AbstractArrow.class);
if(arrows.isEmpty())
return;
for (Map.Entry<Entity, Location> e : LAST_LOCATION.entrySet()) {
Bitte auch noch "tote" Arrows/ stillstehende Arrows rauswerfen.
@ -0,0 +56,4 @@
if(arrows.isEmpty())
return;
for (Map.Entry<Entity, Location> e : LAST_LOCATION.entrySet()) {
if(checkBlocks(e.getKey().getLocation().getBlock(), e.getValue().getBlock()))
Wenn man so häufig e.getKey() verwendet (weiter unten ja auch noch), ist es einfach wesentlich besser lesbar, wenn man das dann einfach mal als temporäre extravariable deklariert.
@ -0,0 +57,4 @@
return;
for (Map.Entry<Entity, Location> e : LAST_LOCATION.entrySet()) {
if(checkBlocks(e.getKey().getLocation().getBlock(), e.getValue().getBlock()))
e.getKey().remove();
Das dürfte glaube ich nicht den Wert aus dem Set entfernen, sondern nur die Entity aus der Welt. LAST_LOCATION wird damit immer weiter zugemüllt.
@ -0,0 +61,4 @@
else {
if(e.getKey().getTicksLived() <= Config.ArrowTechhiderCollision ||
!((AbstractArrow) e.getKey()).isInBlock()) {
LAST_LOCATION.put(e.getKey(), e.getKey().getLocation());
Ich glaube, replace() wäre hier die angebrachtere Operation
@ -0,0 +67,4 @@
}
}
@EventHandler()
Wozu die Klammern?
@ -0,0 +69,4 @@
@EventHandler()
public void onEntityShootBow(EntityShootBowEvent event) {
if(!(event.getEntity() instanceof Player))
Da eh kaum etwas anderes als Spieler den Pfeil schießen werden, ist die Filterung hier eigentlich unnötig
@ -0,0 +75,4 @@
}
@EventHandler
public void onPlayerPickupArrow(PlayerPickupArrowEvent event) {
Spieler heben keine Pfeile auf (ist unterbunden)
@ -0,0 +142,4 @@
}
private boolean checkBlock(Block block) {
return Config.HiddenBlocks.contains(blockToId(block));
Ich glaube, dass du nicht unbedingt die Blöcke zu ihren IDs umwandeln musst, da es ja auch die HiddenBlockTags gibt, in welchen die MATERIAL-Namen der einzelnen Blöcke drin sind, dann sparst du dir das ganze Version-Dependent-Zeugs (und ist wsl. schneller, da ein Map-Lookup weniger).
Würde es dann auch mit
block.getType().ordinal()
gehen?Obwohl, Ne mit dem HiddenBlockTags ist es glaube ich auch besser.
Immoment könnte man es in die Produktion tuhen, sollte aber vorher wirklich noch mal die beeinflussung des Kampfgeschehens genauer begutachten um evt. das Tracking noch mal mehr an einen Fight anzupassen
@ -0,0 +55,4 @@
Entity entity = e.getKey();
if(checkBlocks(entity.getLocation().getBlock(), e.getValue().getBlock())){
entity.remove();
LAST_LOCATION.remove(entity);
Das müsste aus meiner Java-Erfahrung heraus auf jeden fall eine ConcurrentModificationException werfen
@ -0,0 +60,4 @@
else {
if(entity.getTicksLived() > Config.ArrowTechhiderCollision ||
((AbstractArrow) entity).isInBlock() || entity.getLocation().equals(e.getValue())) {
LAST_LOCATION.remove(entity);
Ebenfalls. Nutze einen Iterator.
Arrows Stopping in Techhider Blockszu WIP: Arrows Stopping in Techhider Blocks@ -0,0 +56,4 @@
Entity entity = e.getKey();
if(checkBlocks(entity.getLocation().getBlock(), e.getValue().getBlock())){
entity.remove();
LAST_LOCATION.remove(entity);
Da musst du iterator.remove() verwenden.
@ -0,0 +59,4 @@
LAST_LOCATION.remove(entity);
}
else {
if(entity.getTicksLived() > Config.ArrowTechhiderCollision ||
Die Verschachtelung kann man evtl. durch eine else if-Konstruktion ersetzen.
@ -0,0 +61,4 @@
else {
if(entity.getTicksLived() > Config.ArrowTechhiderCollision ||
((AbstractArrow) entity).isInBlock() || entity.getLocation().equals(e.getValue())) {
LAST_LOCATION.remove(entity);
Auch hier wieder: Iterator benutzen.
@ -0,0 +63,4 @@
((AbstractArrow) entity).isInBlock() || entity.getLocation().equals(e.getValue())) {
LAST_LOCATION.remove(entity);
}else {
LAST_LOCATION.replace(e.getKey(), e.getKey().getLocation());
Ich denke, dass du auch hier den Iterator benutzen musst, weiß aber gerade nicht, ob eine Änderung da überhaupt vorgesehen ist.
@ -0,0 +77,4 @@
@Override
public void enable() {
super.enable();
task = Bukkit.getScheduler().runTaskTimerAsynchronously(FightSystem.getPlugin(), this::run, 1, 1);
Das kannst du nicht Asynchron machen (es dauert 50ms+, bis du einen Chunk für Blockabfragen bekommst), das produziert hier nur Threads.
Ist das noch WIP?
Nope
WIP: Arrows Stopping in Techhider Blockszu Arrows Stopping in Techhider BlocksFände es auch noch schön, wenn du auch mal statt über die Zeit mal mit der Richtung versuchst, zu arbeiten (d.h. Die Flugrichtung zum Faktor machst, ob das jetzt geblockt wird oder passieren darf, finde aber, dass es wsl. auch so geht.
@ -0,0 +37,4 @@
public class ArrowStopper extends BasicListener {
private BukkitTask task;
private static final HashMap<Entity, Location> LAST_LOCATION = new HashMap<>();
Statt der (aufwändigen) Verwaltung von LAST_LOCATIONS wäre es wsl. eleganter, mit entity.getVelocity() / 20 zu arbeiten. Das dürfte einiges an Verwaltungsaufwand sparen. Man könnte dann wsl. in run auch mit world.getEntitiesWithClass() arbeiten.
?, entity.getVelocity() gibt einen Vektor zurück, welchen man nicht einfach durch 20 teilen kann
.divide?
braucht einen Vektor
Du brauchst ja sowieso nur die einzelnen Komponenten, also teile doch einfach die durch 20....
@ -0,0 +43,4 @@
super(EnumSet.of(FightState.RUNNING));
}
public static void init() {
Die Init-Methode kann gestrichen werden und alles in den Konstruktor (der dann Public wird) übernommen werden. Konstruktor wäre dann super(Config.ArrowTechhiderCollision != 0 ? EnumSet.of(RUNNING), EnumSet.empty());
@ -0,0 +46,4 @@
public static void init() {
if(Config.ArrowTechhiderCollision == 0)
return;
FightSystem.registerStateDependent(new ArrowStopper());
BasicListener übernimmt im Konstruktor schon das Registrieren als StateDependent
@ -0,0 +54,4 @@
prevLocation = ((Player) arrow.getShooter()).getEyeLocation();
if(checkBlocks(arrow.getLocation().getBlock(), prevLocation.getBlock())) {
arrow.remove();
iterator.remove();
Das dürfte nichts bringen, da der Iterator eigens auf einem für diese Aufgabe erstellter ArrayList läuft. Die Zeile drüber reicht schon vollkommen aus, um den Pfeil zu entfernen.
@ -0,0 +80,4 @@
for (BlockFace face : BLOCK_FACES) {
Block relative = cursor.getRelative(face);
double distance = relative.getLocation().distance(end.getLocation());
if(distance < nearestDistance) {
Ok, wenn ich jetzt richtig verstanden habe, was diese Funktion macht, dann tut sie linear zwischen den Blöcken interpolieren? Ich meine, das ist zwar nicht die korrekte Minecraft-Berechnungsreihenfolge, soll mir aber auch recht sein.
@ -0,0 +97,4 @@
return Config.HiddenBlockTags.contains(block.getType().name());
}
private boolean isValidEntity(Arrow entity) {
Der Name der Funktion verwirrt etwas, da die Funktion ja einen Invalid boolean zurückgibt. evtl. isInvalid?