Skip to content
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

Add player size and pose event #6166

Open
wants to merge 3 commits into
base: 1.14.x
from
Open

Conversation

@maxanier
Copy link
Contributor

maxanier commented Sep 18, 2019

For original intention see #6059

Because the vanilla pose system is rather difficult to modify, this PR simply introduces a way to modify the player's size regardless of their pose.

This is done by adding override size field to PlayerEntity. If this is set (and the current pose isn't dying), it will be used as the player size in Entity#getSize.

This PR adds two things:

  • A player size event which is fired when the player pose changes and allows mods to specify their own size.
  • A player pose event which is fired once per tick and allows mods to bypass vanilla logic and enforce a certain vanilla pose.

An example why the latter event is required:
A mod want's to shrink a player to just one block height. For this it can use the first event and shrink the player's bounding box. Thereby a player can move into a 1x1 hole.

However, PlayerEntity updates the players pose every tick. Therefore it checks if the theoretical size of the STANDING pose (1.8x0.6) is clear, which is not the case in said hole. Therefore, it falls back to the SWIMMING pose. This does not only affect the renderer but the game logic as well.

Therefore, the second event has to be used to enforce the standing pose.
This has to be done every tick as vanilla will update the pose every tick and always switch back to swimming.

Cons

  • Pose event fired once per tick per player
  • Does not allow non vanilla poses, but this would require a much larger patch since the pose is synced with a DataParameter,
@diesieben07

This comment has been minimized.

Copy link
Contributor

diesieben07 commented Sep 18, 2019

How about a system like this (would also allow pose changes I guess):

  • Only fire an event (for pose and size) once and let mods provide a new pose and size for that player. Mods can be compatible with each other here by e.g. multiplying the size (or adding to it) instead of setting it to a fixed value.
  • If a mod detects "my pose or size value needs to change" they call (a new to be added method) EntityPlayer#refreshSizeAndPose, which will fire the event again and thereby recompute the size and pose (from all mods).

This way you do not have to fire an event each tick and/or frame, mods can still change the values in a compatible way and the values can be changed as frequently (or infrequently) as possible/needed.

@Alpvax

This comment has been minimized.

Copy link

Alpvax commented Sep 18, 2019

If a mod detects "my pose or size value needs to change" they call (a new to be added method) EntityPlayer#refreshSizeAndPose, which will fire the event again

Can we not just fire the event from PlayerEntity#setPose if the pose is different to the current pose?

@maxanier

This comment has been minimized.

Copy link
Contributor Author

maxanier commented Sep 25, 2019

For the size itself there already is a system like diesieben07 described in Vanilla. Entity#recalculateSize which updates the players bounding box is only called if the pose of the player changes. I can add an event there (see 1.14.x...maxanier:size3). It would only be fired infrequently and mods could call recalculateSize if they want to trigger it again.

The issue is that the PlayerEntity checks the theoretical bounding boxes of the current pose every tick and falls back to SWIMMING if it does not fit. Therefore, if we change the size e.g. to 0.9f x 0.9f and the players enters a 1 block hole, the STANDING pose won't fit anymore and it will fall back to SWIMMING.
We could add an event into PlayerEntity#setPose which is only fired if the current pose is different from the old one. Thereby mods could override the fallback. However, it will be fired every tick as long as the enforced pose does not match the one vanilla has calculated in PlayerEntity#updatePose

I think it would be better to somehow prevent the pose from being changed in the first placed. Without being able to define custom poses the entire pose system is rather useless for mods in my opinion anyway. But @Alpvax you are using it, right? Can you elaborate for what

@Alpvax

This comment has been minimized.

Copy link

Alpvax commented Sep 25, 2019

I add a keybind to allow players to crawl/go prone. I do it by overriding the pose in the player tick event in the post phase, which does work (as long as updatePose continues to be called after anything which processes the pose).
It is not ideal however, as I am setting the pose every tick, sometimes just after it has been set to something else. I also have the advantage of not caring about the current size or pose.

@maxanier

This comment has been minimized.

Copy link
Contributor Author

maxanier commented Sep 27, 2019

Alright, so my current suggestion would be to combine the recalculateSize hook, described in my last comment, with another event in updatePose. It would be fired once per player per tick, which shouldn't be to big of a deal and would allow to force a pose. I would place it right at the beginning, I think it is not necessary to let vanilla calculate the pose first, if it is discarded anyway. I don't think any mod would need to know the vanilla pose!?

This still does not "fix" the theoretical value returned by getSize(Pose) (which is used by e.g. isPoseClear(Pose)). But I can't find a solution that does not include ton's of event's or a rework to the entire pose system. And in vanilla it does not cause any issue.

@Alpvax

This comment has been minimized.

Copy link

Alpvax commented Sep 27, 2019

I still think an ideal or at least more ideal solution would be to only fire the event (a PoseChanged event) when the pose actually changes. The event could have an overridePose field and maybe a custom size field as well. Then modders could just call setPose (would require an at) and/or react to the event to react to vanilla/other mod pose changes.

@maxanier

This comment has been minimized.

Copy link
Contributor Author

maxanier commented Sep 27, 2019

MC determines and sets the pose every tick. So if we were to only throw an event if (currentPose!=newPose) it would still be fired every tick, when a mod enforces a different pose.
If you set the pose the SWIMMING in the event, it would fire again next tick because Minecraft tries to set it to STANDING again.
Therefore this would only prevent the event from firing when no mod changes the pose.
So we would introduce some additional accesses to the EntityDataManager in exchange for sometimes skipping the event.

@maxanier maxanier force-pushed the maxanier:size2 branch from b612bf6 to efd08e8 Oct 16, 2019
@maxanier maxanier changed the title Add player entity size override Add player size and pose event Oct 16, 2019
@maxanier maxanier force-pushed the maxanier:size2 branch from efd08e8 to d3be9e8 Oct 16, 2019
@maxanier

This comment has been minimized.

Copy link
Contributor Author

maxanier commented Oct 16, 2019

Alright, since there hasn't been any new feedback, I went ahead and implemented my last proposal:

This PR adds two things:

  • A player size event which is fired when the player pose changes and allows mods to specify their own size.
  • A player pose event which is fired once per tick and allows mods to bypass vanilla logic and enforce a certain vanilla pose.

An example why the latter event is required:
A mod want's to shrink a player to just one block height. For this it can use the first event and shrink the player's bounding box. Thereby a player can move into a 1x1 hole.

However, PlayerEntity updates the players pose every tick. Therefore it checks if the theoretical size of the STANDING pose (1.8x0.6) is clear, which is not the case in said hole. Therefore, it falls back to the SWIMMING pose. This does not only affect the renderer but the game logic as well.

Therefore, the second event has to be used to enforce the standing pose.
This has to be done every tick as vanilla will update the pose every tick and always switch back to swimming.

I can commit a small test mod if you want

@MennoMax

This comment has been minimized.

Copy link

MennoMax commented Oct 17, 2019

Would it be smart to combine the pose event with the other event that determines the eye height (forgot its exact name) which is fired just a few lines later?

@maxanier

This comment has been minimized.

Copy link
Contributor Author

maxanier commented Oct 17, 2019

Yes, that would probably make sense. The eye height event is probably only used by mods that modify the size as well.

However, we don't want to break compatibility with existing mods. We could deprecate the old event and stop firing it (which means any mod already subscribing to the event does not have any effect) or create a new subclass of the event and fire that one instead (should work I think, but is a little bit weird).

I guess breaking functional (not binary) compatibility with existing mods shouldn't be too bad, as I don't expect any mod to really use this feature yet (as player size cannot be modified). But that's not my call to make.

So 3 options,

  1. Keep the two events separated and add TODO for 1.15
  2. Create subclass (if it works)
  3. Create new event and deprecate old one. (Break functional compatibility)
@tterrag1098

This comment has been minimized.

Copy link
Member

tterrag1098 commented Oct 17, 2019

Players are not the only entities that can have poses, why scope this to players only?

@Alpvax

This comment has been minimized.

Copy link

Alpvax commented Oct 18, 2019

While other entities have poses, they aren't set each tick like players' are. That said, for consistency it would be best to have one method of setting pose for all entities.

@maxanier

This comment has been minimized.

Copy link
Contributor Author

maxanier commented Oct 18, 2019

Pose event:
As Alpvax said, normal entities almost never change their pose and don't have an updatePose method which overrides the pose every tick. Therefore it is sufficient to set the pose once using setPose.

Size event:
Yes we can do this here. I just restricted it to players to reduce the number of events and for custom entities you can just override the relevant methods.
But especially, if combined with the EyeHeight event, this would make sense

@maxanier

This comment has been minimized.

Copy link
Contributor Author

maxanier commented Nov 4, 2019

@tterrag1098 What do you think, should the new size event be combined with the eye height event (see four comments above). Then I would make the size event applicable to all entities.
If not, the PR is ready from my end.

@maxanier maxanier force-pushed the maxanier:size2 branch from d7f0f26 to 242d6ac Dec 10, 2019
@maxanier

This comment has been minimized.

Copy link
Contributor Author

maxanier commented Dec 11, 2019

@ichttt
Sorry to bother you, but you seem like the main PR manager right now.
This PR is good to go from my side and there aren't any change requests open. tterrag hat some question/input, but appears to have lost interest. It have been almost two months since the last comment on this PR.
Could you maybe assign this issue to Lex :)
I am not asking for it getting merged right away. If there is anything that should be changed, I am happy to do so. But it would be cool if there was some kind of progress here :/

@Sunconure11

This comment has been minimized.

Copy link

Sunconure11 commented Dec 13, 2019

Maybe go on IRC or Discord regarding this? I know of at least one person waiting on this.

@Unnoen

This comment has been minimized.

Copy link
Contributor

Unnoen commented Dec 13, 2019

Most of the dev team are busy with 1.15.x, however I feel this is in a good spot, I know it means even more time, but if you wait a few days and port this to 1.15, I would feel comfortable assigning it in it's current form and pushing it along.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.