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

Fix MenuType builder locations clashing with existing blocks. #12055

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Y2Kwastaken
Copy link
Contributor

@Y2Kwastaken Y2Kwastaken commented Feb 2, 2025

I addressed this bug by opening a fake tile if the provided location is null, this should prevent any issues, by just building the fake tile right away instead of delegating through the checks to see if it exists in the real world when the intent is to make a virtual menu. This bug is of no concern with the "CraftAccessLocationInventoryViewBuilder" and a more proper "fix" would be the same diff as #12013, so it would be redundant to include it here. Though as far as I'm aware at this current moment no ContainerAccess menus suffer from the same problem as the chest.

I was experimenting with the MenuType API and ran into an issue where when standing on a non-full block (such as a chest or brewing stand), the InventoryView will be that of the container. So any modifications to the items inside the InventoryView would carry over to the chest, brewing stand, etc. in the world. Would this a bug or an intentional part of the API?

message

@Y2Kwastaken Y2Kwastaken requested a review from a team as a code owner February 2, 2025 03:21
@Y2Kwastaken Y2Kwastaken marked this pull request as draft February 2, 2025 03:22
@KioProject123
Copy link

KioProject123 commented Feb 2, 2025

Using the Zero coordinates may cause unnecessary chunk loading.

By binding a location in an unloaded chunk to this builder it is likely that the given chunk the location is will load. That means that when, building this view it may come with the costs associated with chunk loading.

Perhaps you can set the block position outside the world height boundaries, which ensures that a virtual inventory is always returned, such as player.blockPosition().atY(player.level().getMinY() - 1).

However, this may still have side effects, such as players no longer being able to hear the sounds emitted by the inventory.

@Y2Kwastaken
Copy link
Contributor Author

Y2Kwastaken commented Feb 2, 2025

Using the Zero coordinates may cause unnecessary chunk loading.

This is not the case when it comes to the code that menus run, I could technically provide any arbitrary coordinate with none issue, It only causes problems, when things are at location specified, your second comment seems helpful to this.

However, this may still have side effects, such as players no longer being able to hear the sounds emitted by the inventory.

Hmmm I think sound side effect is no issue whatsoever for fake menus, I will take this idea into considerations. Thank you!

@NonSwag
Copy link
Contributor

NonSwag commented Feb 2, 2025

I would refrain from using positions outside the world boundaries
maybe Mojang decides in the future to disconnect the client for block changes outside the world
Also, last time I did virtual inventories myself the block had to be within the reach of the player plus a few additional blocks to even open

@Y2Kwastaken
Copy link
Contributor Author

I would refrain from using positions outside the world boundaries maybe Mojang decides in the future to disconnect the client for block changes outside the world Also, last time I did virtual inventories myself the block had to be within the reach of the player plus a few additional blocks to even open

I understand your concern with outside the world positions, however it will be tested thoroughly as an option. Also if you were unable to reach the inventory within a a couple blocks you incorrectly set up your menu api. Distance only really matters for physical menus that are stored within the world. Virtual menus are virtual and thus disconnected from the world, even if they create a fake tile the tile is not actually in the world. I have also other things I wish to attempt to mitigate these issues, such as using the players upper body position instead, which is slightly more favorable if I can avoid these glitches with that.

@masmc05
Copy link
Contributor

masmc05 commented Feb 2, 2025

Why don't you just use ContainerLevelAccess.NULL there instead of creating a real one with fake coordinates?

@Y2Kwastaken Y2Kwastaken marked this pull request as ready for review February 2, 2025 15:51
Y2Kwastaken added a commit to Y2Kwastaken/Paper that referenced this pull request Feb 2, 2025
@NonSwag
Copy link
Contributor

NonSwag commented Feb 2, 2025

you pushed your test plugin

@Y2Kwastaken
Copy link
Contributor Author

Y2Kwastaken commented Feb 2, 2025

you pushed your test plugin

yes, yes I did, if you looked at full diff it was removed, they will squash on merge.

@NonSwag
Copy link
Contributor

NonSwag commented Feb 2, 2025

Sorry, wasn't quite sure if you noticed
I didn't see the full diff

lynxplay pushed a commit to Y2Kwastaken/Paper that referenced this pull request Feb 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Awaiting review
Development

Successfully merging this pull request may close these issues.

4 participants