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

Close block windows when player is too far away #6382

Closed

Conversation

CoderJoeW
Copy link

@CoderJoeW CoderJoeW commented Jul 3, 2024

Introduction

If a player is inside of a container window and is moved over 6 blocks from the container the window will automatically be closed

In vanilla minecraft when I was testing this it closed the window after be moving 12 blocks(I could have miscounted) so that is what I implemented here

By running the following this change can be disabled

$inventory->setMaxDistanceFromContainer(0)

Relevant issues

This is a resolution for the issue

#6349

Test

https://file.io/mphsWsJrFfcy

@jasonw4331 jasonw4331 added Category: Gameplay Related to Minecraft gameplay experience Category: Core Related to internal functionality Status: Insufficiently Tested Type: Enhancement Contributes features or other improvements to PocketMine-MP labels Jul 3, 2024
Copy link
Contributor

@jasonw4331 jasonw4331 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The composer lock file needs removed from the PR and some other API might be nice, but it looks clean.

src/player/Player.php Outdated Show resolved Hide resolved
src/player/Player.php Outdated Show resolved Hide resolved
@CoderJoeW
Copy link
Author

I am not super familiar with the API implementation so that might take me a min to figure out

Do you think we should have some method like setWindowCloseDistance and setting it to 0 would disable the functionality?

@CoderJoeW
Copy link
Author

@ShockedPlot7560 @nicholass003 @jasonw4331

I believe I addressed all concerns and included a video of testing the server against a vanilla world

I found that in further testing vanilla seems to kick you out of an inventory window > 6 blocks distance so thats what I also updated this PR to do

src/player/Player.php Outdated Show resolved Hide resolved
src/player/Player.php Outdated Show resolved Hide resolved
src/block/ProximityRestricted.php Outdated Show resolved Hide resolved
src/block/ProximityRestricted.php Outdated Show resolved Hide resolved
src/block/ProximityRestricted.php Outdated Show resolved Hide resolved
@ShockedPlot7560 ShockedPlot7560 requested a review from a team as a code owner November 18, 2024 10:33
@ShockedPlot7560 ShockedPlot7560 changed the title 6349 container window not closed Close block windows when player is too far away Nov 18, 2024
@ShockedPlot7560
Copy link
Member

I've taking back this PR to try this to be merged.

I've made some changes to fix stale reviews but still has question about we have to include this utility.
For now, we introduce a new ProximityRestricted interface which need to be combined with the BlockInventory to be functionnal. We can't include this interface into the BaseInventory bc its linked to a block inventory and we can't include it into the BlockInventory without any BC breaks. Do we keep a new interface which need to be included everywhere we have a BlockInventory or anything else come in your minds ?

@dktapps
Copy link
Member

dktapps commented Nov 28, 2024

Do we keep a new interface which need to be included everywhere we have a BlockInventory or anything else come in your minds ?

I think the whole thing is overengineered. Most likely the only thing needed is to check if the block is still within the player's reach distance. I don't think this should be the Block's responsibility at all.

@dktapps
Copy link
Member

dktapps commented Nov 28, 2024

Also worth asking if we really want to make an invasive change like this considering the upcoming #6533.

@dktapps
Copy link
Member

dktapps commented Nov 29, 2024

Closing this as I think the solution is way too complicated for the requirements.

@dktapps dktapps closed this Nov 29, 2024
@dktapps dktapps added the Resolution: Declined PR has been declined by maintainers label Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Core Related to internal functionality Category: Gameplay Related to Minecraft gameplay experience Resolution: Declined PR has been declined by maintainers Type: Enhancement Contributes features or other improvements to PocketMine-MP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants