[1.16] Replace EntityHeight event with EntitySize event

[maxanier]

1.16 version of #6369
For the reasoning see: #6059

This PR replaces the existing EntityEvent#Height with a EntityEvent#Size which allows modifying the entity size in addition to the eye height. This is binary breaking.

I personally only need to modify the player size, but as the existing Height event is posted for all entities, this system is therefore available for all entities.

If needed, I can commit a small test mod.

Very basic test code

@Mod("player_size_test")
public class PlayerSizeTest
{
    public PlayerSizeTest(){
        MinecraftForge.EVENT_BUS.register(this);
    }


    @SubscribeEvent
    public void onEntitySize(EntityEvent.Size event){
        if(event.getEntity() instanceof PlayerEntity){
            if(((PlayerEntity) event.getEntity()).inventory !=null && ((PlayerEntity) event.getEntity()).getHeldItemMainhand().getItem() == Items.STICK){
                event.setNewSize(EntitySize.fixed(0.2f,0.2f));
                event.setNewEyeHeight(0.15f);
                ((PlayerEntity) event.getEntity()).setForcedPose(Pose.STANDING); //In theory only necessary once
            }
            else{
                ((PlayerEntity) event.getEntity()).setForcedPose(null); //In theory only necessary once
            }
        }
    }

}

This simple changes the player's size and pose while holding a stick (have to sneak to update the size here)
If adding a proper test mod, I would write something more suitable.

[maxanier]

I am not sure about this part.
The old eyeHeight method was placed in the constructor as well, so I kept it here.
However, I do not think this is ideal. If mods decide to change an entity's size/eyeHeight, they need something to base their decision upon. But at this point during entity construction neither capabilities nor held items nor anything else is available.
This means a) the event handler has to check if the entity is properly constructed and b) the mod has to trigger a size update anyway.
Only scenario this might be useful might be a mod which e.g. simply scales all cows to 200%. But that could also be done without the constructor patch, by just calling recalculateSize in EntityJoinWorld.

Therefore, I would suggest removing this ugly patch, making it much easier to use.

[LexManos]

The issue is that we have to have SOME default value in the constructor or somewhere that is always called before any other methods in the entity is called because eyeHeight needs to be a sane value until the next time resize is called.

[maxanier]

I would simply use the vanilla methods for size and eyeHeight. Thereby they are at sane values, and if desired a mod can still force a size recalculation once the entity is fully constructed.

[ichttt]

A small nitpick, otherwise looks good to me.
Regarding you question about firing in the contructor, I'm not too sure, someone else will have to decide this

[ichttt]

I think you can add a few newlines to make this patch more readable

[TheCurle]

Yeah. You should aim for conciseness in patches, but not to the degree of unreadability.

[LexManos]

The issue is that we have to have SOME default value in the constructor or somewhere that is always called before any other methods in the entity is called because eyeHeight needs to be a sane value until the next time resize is called.

[LexManos]

honestly all of this should move to ForgeHooks/ForgeEvenntFadctory like all the other events instead of being special.

[maxanier]

moved

[LexManos]

Why is this being forced at all? The data manager already syncs the pose, why add some backdoor to bypass it?

[maxanier]

Unfortunately, the player entity (re)determines the players pose every tick and thereby overrides any manually set pose.
To determine the current pose the PlayerEntity uses hardcoded sizes and therefore e.g. falls back to the swimming pose when the player is in a 1x1 space (where they fit because of actual entity size).
It is possible to override the value again right after that in the Post PlayerTickEvent, so the pose determined by vanilla does not affect rendering or game mechanics. However, every time the pose is changed notifyDataManagerChange calls recalculateSize causing an event to be posted.
Therefore, without this "backdoor" the event would be posted twice every tick instead of not at all.

If you still prefer this, I can remove this patch.

[ichttt]

You don't need to use fully qualified names here, just use EntityEvent.Size instead of net.minecraftforge.event.entity.EntityEvent.Size. Same in the line below

[LexManos]

We're still waiting on @iChun to give his feedback because the eye height was done on request of Morph.
I'm still not a fan of the active event. But it doesn't seem to have a way to have it as a property of the pose or whatever you're using to determine the form.

[iChun]

I've had a look see through the changes, and personally I'm in agreement of extending the EyeHeight event into a proper Size event since the addition of Pose to MC.

I've also gone through #6059 briefly and I understand the challenges in extending Pose with IExtensibleEnum considering issues syncing with the clients as well as breaking vanilla, so I reckon this might be the next best option in regards to changing an entity/player's size.

The only thing I'm not too sure about currently is the check for forcedPose in updatePose(). Although I can see why it's done earlier on to bypass vanilla checks, it also bypasses vanilla's fallback of using the smallest pose possible for the player. This opens things up to errors done by a mod dev eg forgetting to check for clearance like vanilla does, or forgetting to clear the forced pose.

Some documentation reminding devs not to neglect that should be added, as well as a getter for forcedPose added for other devs to check that it is not already being forced by other mods. Mod conflicts with eyeHeight and entity size was always an issue in my experience with Morph, and it would be a nice note to add as well that mods should only change those when they should, instead of eg every tick. I understand wanting to keep patch sizes small, but as this is a new method added by Forge and with potential mod conflicts, it might be worthwhile adding at least a bit of documentation to the method.

[maxanier]

Alright, @iChun thank you for your input.
I will add the requested documentation as well as the getter.
I agree that having some clearance check for the forcedPose would be good, but it would require posting another event.

Was there any specific reason for the "getEyeHeight" in the constructor? Otherwise I will remove it, as it mainly causes issues.
#6858 (comment)

[iChun]

Check for forcedPose can be done when listening to the player tick. Another event is not necessary. It's just worth adding docs so that mod devs don't forget about it.

As to having getEyeHeight in the constructor, it's been a long while and eyeHeight has changed a lot since it was first added for Morph, but I think it's done in the constructor because not all entities will call recalculateSize in their lifetime, and it allows changing the eyeHeight before the entity is ticked. Having the default eye height, and then changing it a tick later, potentially causes issues to other mods (eg projectile firing mods) or rendering (especially if you're changing the player eye height on respawn).

One can simply check isAddedToWorld() or firstUpdate to see if it's been added yet/ticked yet. We pass in the default eyeHeight/size for reference to the dev. It also allows for the dev to see if the height has already been changed by another mod in the same event.

[maxanier]

Ok, then I will keep it and add an appropriate comment to the event

[maxanier]

Rebased and applied suggestions

[maxanier]

@ichttt Anything Lex wants changed?

[ichttt]

Please update to 1.16.2, otherwise this looks good to me and lex

[maxanier]

Done

[LexManos]

I am wholey not a fan of this hack, but the original was a hack as well. And ichun says its fine. So, here we go :/