Block break progress improvement #6500
Conversation
|
Duplicates #5581 |
|
@GameParrot what's your source of information for the data in this PR? |
dktapps
added
Category: Gameplay
Haste speed modifier was from the wiki, mining fatigue speed modifier was found by testing (wiki numbers inaccurate) |
|
I'd test those thoroughly if I were you. The last guy that tried to do this in #5581 couldn't get them to fully match up, not with wiki sources and not by guesswork either. Even better if you can pull up an old BDS in IDA and find the code that handles this in vanilla. |
Haste and mining fatigue seem pretty much perfect from my testing (tested with both low and high levels) |
…/GameParrot/PocketMine-MP into block-break-progress-improvement
| /** | ||
| * Checks if the player is currently on the ground. This is more accurate than {@link Player::isOnGround()} but slower. | ||
| */ | ||
| public function isActuallyOnGround() : bool{ |
There was a problem hiding this comment.
Something like calculate or smth in the name can be better than this function combined with docs ?
There was a problem hiding this comment.
Yeah, I'm not a fan of this naming either. Also, why do we actually need this?
There was a problem hiding this comment.
Yeah, I'm not a fan of this naming either. Also, why do we actually need this?
We need it because when using the normal is on ground method the break progress indicator is still too fast when jumping. What should it be renamed to?
There was a problem hiding this comment.
Sounds like that warrants investigation into regular onGround behaviour.
There was a problem hiding this comment.
Legend has it that PlayerAuthInputPacket should allow making onGround checks more accurate, so this hack wouldn't be needed anymore.
There was a problem hiding this comment.
I think the PR should use the built-in onGround for now. It doesn't make sense to add an extra function specifically for this.
Once the proper onGround is fixed per #6547 we won't need this workaround. I don't like adding stuff to the API that we know we're gonna delete anyway.
| @@ -869,7 +869,7 @@ static function() : void{ | |||
| * Checks if the player is currently on the ground. This is more accurate than {@link Player::isOnGround()} but slower. | |||
| */ | |||
| public function isActuallyOnGround() : bool{ | |||
| $bb = $this->boundingBox->expandedCopy(0, 0.001, 0); | |||
| $bb = $this->boundingBox->expandedCopy(-0.001, 0.001, -0.001); | |||
There was a problem hiding this comment.
This change doesn't look related to the PR. Why would this be needed for ground calculation?
There was a problem hiding this comment.
Fixes inaccurate progress at high x/z coordinates
There was a problem hiding this comment.
Many things break down at large coordinates. I doubt this is the proper fix for the problem.
dktapps
added
Status: Blocked
There was a problem hiding this comment.
Please change this to use the normal onGround() and remove isActuallyOnGround().
Fixing onGround() needs to be done separately from this change as it affects a variety of other functions and the way this PR is doing it isn't the right way.
Including this change in here is blocking the PR.
dktapps
added
Status: Waiting on Author
and removed
Status: Blocked
|
This PR has been marked as "Waiting on Author", but we haven't seen any activity in 7 days. If there is no further activity, it will be closed in 28 days. Note for maintainers: Adding an assignee to the PR will prevent it from being marked as stale. |
Fixes block break progress being inaccurate when not on ground or swimming. Also fixes syncing block break speed with client.
Related issues & PRs
Changes
API changes
Added Player::isActuallyOnGround(), more accurate than Player::isOnGround() but slower.
Behavioural changes
Block breaking progress is now much closer to vanilla.
In-Game Testing
Before:
https://github.com/user-attachments/assets/22f50b6e-f994-4a17-8dfc-53df70a8d6eb
After:
https://github.com/user-attachments/assets/3341eaaf-2bbc-4ac1-ad35-39947e907080