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

1.20.1 Use pose system for calculating the height of crouching instead of hardcoded values #4485

Open
wants to merge 7 commits into
base: 1.20.1
Choose a base branch
from

Conversation

Happyrobot33
Copy link

@Happyrobot33 Happyrobot33 commented Aug 28, 2024

instead of using hardcoded crouching values, just use the built in method for geting a entitys eye height with a specific pose. Should close #4484 from personal testing

closes #4484

@ZacSharp
Copy link
Collaborator

Removing a public method from the API package. I'd say an @Deprecated is more appropriate.

Otherwise a simple and sane looking change.

@wagyourtail
Copy link
Collaborator

IPlayerContext does have access to the player entity, so you could just put the pose thing in the old function

@Happyrobot33
Copy link
Author

IPlayerContext does have access to the player entity, so you could just put the pose thing in the old function

the problem is its static so I'm not sure how to even reference the IPlayerContext directly here since it doesn't have a instance reference. and if it was changed to non static it would still be a breaking change

@ZacSharp
Copy link
Collaborator

ZacSharp commented Sep 6, 2024

There is the possibility of BaritoneAPI.getProvider().getAllBaritones().stream().map(Baritone::getPlayerContext).filter(c -> c.player() == entity).findFirst().get().getEyeHeight(true), but that's clearly worse and (re?)introducing BaritoneProvider::getBaritoneForPlayer wouldn't help much.
Also why assume the entity is a player? I'd say either the method has to take a LocalPlayer argument or assuming the entity is a player is plain wrong. Not sure whether we should assume there is a Baritone instance for the passed player.

EDIT: I've been tricked by an interface default implementation. IBaritoneProvider::getBaritoneForPlayer does exist, but not in the file I looked at. So it would be BaritoneAPI.getProvider().getBaritoneForPlayer((LocalPlayer) entity).getPlayerContext().getEyeHeight(true), with potential for CCE and NPE for no reason.

@Happyrobot33
Copy link
Author

There is the possibility of BaritoneAPI.getProvider().getAllBaritones().stream().map(Baritone::getPlayerContext).filter(c -> c.player() == player).findFirst().get().getEyeHeight(true), but that's clearly worse and (re?)introducing BaritoneProvider::getBaritoneForPlayer wouldn't help much. Also why assume the entity is a player? I'd say either the method has to take a LocalPlayer argument or assuming the entity is a player is plain wrong. Not sure whether we should assume there is a Baritone instance for the passed player.

yeah that long method call is just painful to read as is and feels very breakable lol. I think deprecating it is a better option here, and anyone using it should just refer to the entity itself in the same way the codebase does

@leijurv
Copy link
Member

leijurv commented Sep 16, 2024

can this be done to the 1.19 branch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants