Display entity projectiles

[KaevonD]

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:

<projectile
        id="laser"
        name="laser"
        projectile="Block"
        material="RED_STAINED_GLASS"
        velocity="0.5"
        damage="1"
        scale="0.2"
        throwable="false"
        max-travel-time="2s"
        cooldown="1s"/>

[Pablete1234]

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.
In 1.8 they can be represented with a falling-block (only supports scale = 1, potentially 0.8 and 0.4 using normal and small armor stands with the block in their head, values of scale >1 just get ignored in legacy)
In modern they can be represented with a display entity

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 BlockMaterialData#spawnFallingBlock, and just add an extra method for spawnBlockEntity, which returns a PGM type that is a wrapper around a "BlockEntity", in legacy the impl relies in a falling block (with no ai enabled, so it doesn't have phyisics), while in modern it relies on display entities. That way all the code in the projectile module stays clean of calls to nms hacks, it just interacts with this wrapper to rotate, move, or whatever, and the wrapper takes care of the necessary translation layer

[KaevonD]

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.

[Pablete1234]

A class with no members and no functions, may as well be a sealed interface instead:

Suggested change

	public static sealed class ProjectileEntity {
	public sealed interface ProjectileEntity permits RealEntity, CustomEntity {

[Pablete1234]

You can probably use a record for these:

Suggested change

	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 {}

[Pablete1234]

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

[Pablete1234]

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)

[Pablete1234]

Same as above, avoid Abstract when something is not abstract

[Pablete1234]

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

[Pablete1234]

the nms -> packets package is not a good place for this, this is nethier needing to access NMS, nor is it packet based

[Pablete1234]

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

[Pablete1234]

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

[Pablete1234]

Again this breaks encapsulation, the user of this api (the projectile module) should not need to know or care how the underlying magic happens

[Pablete1234]

i believe velocity is irrelevant in all of BlockEntity itself

[Pablete1234]

Doesn't properly respect rules of who can attack who, eg: friendly fire may be enabled.

Look for methods that may allow you to check this, if all else fails, nms.
You'll still want to keep the canInteract tho, that's a pretty easy check to ensure thye're not like, dead etc

[Pablete1234]

This may be the only available damage value that exists in the intersect jar (ie: both 1.8 and modern), but i'm sure if you look at the bukkit apis of each version there have to be ways to provide more info, like a damage source/type style apis so that it can know its thru a projectile thrown by another player, instead of counting as more or less "player A hit player B" as if it was a punch, so that hopefully the pgm tracker can display it properly.

If you can't find anything like that, maybe you need a custom way to signal the death tracker module what kind of kill this is such that it can create appropriate messages (thinking of just using the projectile ones, like arrows/bows, ie: PlayerA was shot by PlayerB (x blocks) or similar)

[Pablete1234]

not needed anymore, you're already calculating substeps such that it shouldn't be an issue

[Pablete1234]

these are also checked inside blockDisplayCollision, so i'd say they're redundant and you can get rid of them

[Pablete1234]

avoid wildcard imports

[Pablete1234]

Suggested change

	net.minecraft.server.v1_8_R3.World world = ((org.bukkit.craftbukkit.v1_8_R3.CraftWorld) pos.getWorld()).getHandle();
	var world = ((CraftWorld) pos.getWorld()).getHandle();

[Pablete1234]

Suggested change

	AxisAlignedBB AABB = new AxisAlignedBB(
	AxisAlignedBB aabb = new AxisAlignedBB(

[Pablete1234]

i'm unsure why you chose to implement this in nms per-version when the bukkit API provides a proper way of getting this, via World#getNearbyPlayers(center, xrad, yrad, zrad, predicate)

You can do the bounding box grow and intercept here if u want but seems like the extracting of the list of players could be done outside, or even be baked into the predicate, like:

p -> isEnemy(shooter, p) && NMS_HACKS.raytrace(p, vecStart, vecEnd, halfSize)

then off the resulting list of entities, you pick the closest to vecStart (if any)

that way the per-version impl would be very limited, just something like:

public boolean raytrace(Entity e, Vector start, Vector end, double grow) {
    Vec3D vecStart = new Vec3D(start.getX(), start.getY(), start.getZ());
    Vec3D vecEnd = new Vec3D(end.getX(), end.getY(), end.getZ());

    var bb ((CraftEntity) e).getHandle().getBoundingBox().grow(grow, grow, grow);
    return bb.a(vecStart, vecEnd) != null;
}

[Pablete1234]

This logic seems to inherently assume that just because you collided with a block, you no longer hit a player. That's not true.

Imagine you shoot a high-speed projectile (5 blocks per second), a player is standing 2 blocks away from the wall, and you're standing 9 blocks away from the wall. You fire, first tick the projectile advances 5 blocks and ends at 4 blocks from the wall (doesn't hit the player), next tick it tries to travel 5 blocks forward, hits the wall, and is cancelled, having completely phased thru the enemy player.

For this logic to be correct, collidesWithBlock needs to return a vector of where you collided with a block, and that should be reused for player collision checks, where effectively they're "cut short", so in the above example, on the 2nd tick, the projectile hit the wall after 4 blocks, so the start and end positions of the player collisions should be that 4-block range, instead of the 5 it would otherwise be.