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

feat: add a getNextHighesBlock method to the plotspawn teleportation #4352

Closed
wants to merge 5 commits into from

Conversation

Tamikaschu
Copy link
Contributor

Fixes #4351

@Tamikaschu Tamikaschu requested a review from a team as a code owner February 20, 2024 17:59
@github-actions github-actions bot added Bugfix This PR fixes a bug Feature This PR proposes a new feature labels Feb 20, 2024
@OneLiteFeather OneLiteFeather removed the Bugfix This PR fixes a bug label Feb 21, 2024
@Tamikaschu Tamikaschu marked this pull request as draft March 3, 2024 20:15
@github-actions github-actions bot added the Bugfix This PR fixes a bug label Mar 3, 2024
@Tamikaschu
Copy link
Contributor Author

@OneLiteFeather Sorry, the Bugfix label applied again :(

@Tamikaschu Tamikaschu marked this pull request as ready for review March 4, 2024 16:10
@OneLiteFeather OneLiteFeather removed the Bugfix This PR fixes a bug label Mar 4, 2024
@OneLiteFeather
Copy link
Member

All good, we still need someone to review

@PierreSchwang
Copy link
Member

What I'm thinking about while reviewing - shouldn't a check be made, that there are at least 2 blocks of space for the player being able to stand up? In the fallback case we can't make sure that's possible, but that's why it's the fallback.

@Tamikaschu
Copy link
Contributor Author

What I'm thinking about while reviewing - shouldn't a check be made, that there are at least 2 blocks of space for the player being able to stand up? In the fallback case we can't make sure that's possible, but that's why it's the fallback.

Technically the top 2 Blocks should be checked yes. The player would Spawn in the Swimming Animation right now if there is a full block above. Currently it would also allow the player spawn in such spawns (see screenshots), which should also be possible, since a player can sneak under a half slab/stand on carpet.
image
image

@PierreSchwang
Copy link
Member

I guess carpets are totally alright - so about 1.85 blocks space or so?

@Tamikaschu
Copy link
Contributor Author

I guess carpets are totally alright - so about 1.85 blocks space or so?

Carpets on the Plotspawn are counted as a blocked spawn, since you cannot really differ between half and full blocks. If there would be a block somewhere above (e.g. 100 blocks above) you would get teleported to that one. This is why i created the issue in first place and suggested replacing the getHighestBlock with a getNextHighestBlock method to make it more user friendly. I get that checking for the next 2 air blocks would be a more safer solution, still it would block the spawns like in the pictures above, even though there would be enough space for a player to fit in.

@OneLiteFeather
Copy link
Member

What about adding this "safety gap" as an configurable option too? In 1.21 we might get in trouble with entities which can change sizes, also for players. 2 blocks would be enough for the most servers I guess but those playing around with these entity sizes would still get stuck in the blocks. You might want to do a different PR regarding that for 1.21 later or add it here.

@Tamikaschu
Copy link
Contributor Author

What about adding this "safety gap" as an configurable option too? In 1.21 we might get in trouble with entities which can change sizes, also for players. 2 blocks would be enough for the most servers I guess but those playing around with these entity sizes would still get stuck in the blocks. You might want to do a different PR regarding that for 1.21 later or add it here.

You would always have to spawn in the Player at a BlockLocation. If the plotspawn should support the players size you would always have to round up the blocks which need to be air in order to spawn in the player safe. Adding a safety gab wouldnt be possible that easy since the plot location only supports integer values.

@Tamikaschu
Copy link
Contributor Author

Tamikaschu commented Mar 4, 2024

Adding support to different sizes would probably also require a different way of checking if the plotspawn is blocked due to not only a possible change in height but also in width. Therefore some more adjustments have to be taken into account.

@Tamikaschu
Copy link
Contributor Author

Closing due to yet impossible expectations in adjustments

@Tamikaschu Tamikaschu closed this Mar 6, 2024
@Tamikaschu Tamikaschu deleted the fix/plot-spawn branch March 6, 2024 19:22
@PierreSchwang
Copy link
Member

I've would've been totally fine without a two-block-height check tbf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature This PR proposes a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refinement to the Plotspawn Teleportation
3 participants