-
Notifications
You must be signed in to change notification settings - Fork 97
Display entity projectiles #1502
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
base: dev
Are you sure you want to change the base?
Conversation
9d6fa9c
to
e5e0a5d
Compare
I feel like this implementation doesn't quite fit pgm. Don't get me wrong tho: i do like the idea of supporting this type of projectile! but it should be in a slightly different way. Consider that instead of calling them "display entities" you just call them "block" projectiles. All the logic related to simulating the trajectory, can be shared for both, so there's little reason why that shouldn't be supported or should be gatekept/locked behind modern requirement. Ideally this doesn't use "NMS_HACKS" at all, as none of this is actually reliant on NMS, it's just version-dependent. In fact i think you should take a look at the existance of |
e5e0a5d
to
5299c21
Compare
…ve yet or have collision.
…n interpolation setting
5299c21
to
00282c6
Compare
Projectile for this feature is no longer "BlockDisplay" but "Block". In newer versions functionality remains the same and spawns a block display entity, but in lower versions it will now spawn a falling block instead. |
this.maxTravelTime = maxTravelTime; | ||
} | ||
|
||
public static sealed class ProjectileEntity { |
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.
A class with no members and no functions, may as well be a sealed interface instead:
public static sealed class ProjectileEntity { | |
public sealed interface ProjectileEntity permits RealEntity, CustomEntity { |
public static final class RealEntity extends ProjectileEntity { | ||
public final Class<? extends Entity> entityType; | ||
|
||
public RealEntity(Class<? extends Entity> entityType) { | ||
this.entityType = entityType; | ||
} | ||
} | ||
|
||
public static final class AbstractEntity extends ProjectileEntity { | ||
public final AbstractEntityType entityType; | ||
|
||
public AbstractEntity(AbstractEntityType entityType) { | ||
this.entityType = entityType; | ||
} | ||
} |
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.
You can probably use a record for these:
public static final class RealEntity extends ProjectileEntity { | |
public final Class<? extends Entity> entityType; | |
public RealEntity(Class<? extends Entity> entityType) { | |
this.entityType = entityType; | |
} | |
} | |
public static final class AbstractEntity extends ProjectileEntity { | |
public final AbstractEntityType entityType; | |
public AbstractEntity(AbstractEntityType entityType) { | |
this.entityType = entityType; | |
} | |
} | |
record RealEntity(Class<? extends Entity> entityType) implements ProjectileEntity {} | |
record CustomEntity(CustomEntityType type) implements ProjectileEntity {} |
} | ||
} | ||
|
||
public static final class AbstractEntity extends ProjectileEntity { |
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.
Abstract
has a very specific meaning in the java world, and given this class isn't even abstract, i'd vote against it, and instead probably use something like CustomEntity
float scale, | ||
boolean solidBlockCollision, | ||
Duration maxTravelTime) { |
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.
These seem to only affect custom projectile entities, so it probably makes sense that they'd go in as part of the CustomEntity
implementation of ProjectileEntity.
Additionally scale
should probably be size
as that leaves it more open to future projectiles (ie: raytraced beam)
} | ||
} | ||
|
||
enum AbstractEntityType { |
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.
Same as above, avoid Abstract
when something is not abstract
@Override | ||
public BlockEntity spawnBlockEntity(Location loc, BlockMaterialData blockMaterialData) { | ||
final Entity entity = loc.getWorld().spawn(loc, BlockDisplay.class); | ||
return new DisplayEntity(entity); | ||
} | ||
|
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 is for entity packets, ie: fake-entities that are packet-driven
i mentioned this as an example of how you could make your impl, but this is not the place for it
@@ -0,0 +1,14 @@ | |||
package tc.oc.pgm.util.nms.packets; |
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 nms -> packets package is not a good place for this, this is nethier needing to access NMS, nor is it packet based
// Entity must have an actual rotation of 0 before invoking this | ||
// as this merely modifies the transformation matrix, otherwise it may appear offset | ||
void align(float pitch, float yaw, float scale); | ||
void setBlock(Material block); | ||
void setTeleportationDuration(int duration); |
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 is way too fine-grained of a control, the mere spawning of it should be handling this so that they're not at this level
|
||
public interface BlockEntity { | ||
boolean isDisplayEntity(); | ||
Entity entity(); |
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.
ideally you should NOT expose the entity itself, that there even is an entity is an implementation detail that should be hidden from the observer, because if for example armor stands are used, the position of the armor stand itself will not align with the position of the block entity (the armor stand feet will always be 1.8 blocks below), so if you expose the entity, that illusion breaks.
if instead you add here move methods, an armor-stand-backed impl can:
private static final double OFFSET = 1.8f;
void teleport(Location loc) {
loc = loc.clone();
loc.setY(loc.getY() - OFFSET);
this.entity.teleport(loc);
}
which is the whole idea behind hiding this in impl-dependant methods
import org.bukkit.entity.Entity; | ||
|
||
public interface BlockEntity { | ||
boolean isDisplayEntity(); |
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.
Again this breaks encapsulation, the user of this api (the projectile module) should not need to know or care how the underlying magic happens
Signed-off-by: Pablo Herrera <[email protected]>
Signed-off-by: Pablo Herrera <[email protected]>
677965c
to
25afbbc
Compare
This feature introduces display entities (specifically block display entities) as projectiles. This feature is only supported for modern PGM.
This can be done in XML, specifying "projectile" to be "BlockDisplay". Speed can be controlled with the "velocity" attribute, damage with the "damage" attribute, and cooldown with the "cooldown" attribute. In addition, the size of the projectile can be specified with the "scale" attribute, where 1 is the default size. The travel time can be specified with the "maxTravelTime" in seconds, where 1 second is the default travel time. Currently, these projectiles travel in a linear path with no falloff.
This is my first PR to PGM, so I understand if I missed some things. I hope I can fix these and make it usable! Thanks for the help.
Example projectile: