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

MenuType API addition InventoryView Builders #11816

Merged
merged 10 commits into from
Jan 11, 2025

Conversation

Y2Kwastaken
Copy link
Contributor

This Pull request ports InventoryView Builders from Spigot (Quite the Christmas Surprise!)
Relevant Spigot Pulls that this PR pulls from

Bukkit
Add MenuType ViewBuilder

Improve ViewBuilder Chaining
Deprecate HumanEntity#open[Menu]; Add Server#createMerchant()

CraftBukkit
Add MenuType ViewBuilder
Improve ViewBuilder Chaining
Implement Server#createMerchant; Fix MerchantView opening inconsistency

Rational
The Server#createInventory and current HumanEntity#open[Container] APIs are currently extremely limited and are not comparably close. These methods built around previously very non consistent and broken behavior. They goal of this Pull Request is to take one step closer to deprecating the old Server#createInventory PR and InventoryType to allow for a much more well rounded and modern API. Previously implementing MenuType with a simple #create(Component, HumanEntity) method started a basis for this API.

Why Builders? And not just more create methods?
InventoryView builders for both me, and those I discussed this idea with, were the clear answer to building off the current MenuType API, for a couple of reasons. The reasons behind my rational to builders specifically is below

InventoryView builders are an easy API to use. It is much easier for an API consumer not to have to worry about different, possibly unknown casting types if something were to be delegated to the Server interface. Currently, this ambiguity is present with the Server#createInventory API, which leads some to believe you can cast to the "expected" type of inventory. Or, with the unrelated ItemMeta, it may be unclear if you can cast in a kind.

InventoryView builders are flexible with nonstandard InventoryView types. Builders allow nonstandard types, such as the current MerchantMenu builder, to be added with less stress about developing a lasting API. Even though many InventoryViews can be created with a location parameter, adding a MenuType#create(Location, Component, HumanEntity) makes a non-true assumption about all current MenuTypes and perpetuates this assumption into the future. ViewBuilders aims to be as long-lasting as possible, and most, if not all, other implementations of such an API would be exposed to the drawbacks of these unsafe assumptions.

Expanding the InventoryView builders API will lead to a better "Inventory" API. It is clear and has been for years, that Server#createInventory is simply dysfunctional. Future API could be added to view builders to allow for the integration of an Inventory parameter on select InventoryViews. For example, an API added to a view builder might look like this:

MenuType.GENERIC_9X3.builder()
	.inventory(someFutureMethodToCreateInventories())
	.build(player)

Such an API is the way forward, as we should begin to move away from Server#createInventory methods. InventoryView builders allow for more explicit control over which InventoryViews are permitted to have an Inventory attached to them.

The last reason I see InventoryView builders as a good idea is their ability to "store" values. In the future, as people shift away from the old Server#createInventory API, some benefits will be a pain to lose. InventoryView titles aren't actually declared until they are opened. While a standard title could be saved as a constant, using a "saving" type method with some standard values could help API consumers have some standard opening protocol for specific menus they want to create. An example of such a concept is below:

private static final LocationInventoryViewBuilder<InventoryView> STANDARD_CHEST = MenuType.GENERIC_9X3.builder().title(Component.text("This is some title that won't ever change or is defined somewhere")

public static InventoryView createStandardChest(HumanEntity player) {
	return STANDARD_CHEST.copy().location(player.getBlockExact(10)).build(player);
}

While maybe not a "super" realistic example, this portrays another rational behind InventoryView builders.

Final Notes and Edits

I'm not exactly sure what the "new" standard is on paper comments, especially when it comes to pulling from upstream, are they no longer needed? What's the deal there. Feedback in that facet would be appreciated

@Y2Kwastaken Y2Kwastaken requested a review from a team as a code owner December 25, 2024 04:28
@kennytv kennytv added the type: feature Request for a new Feature. label Dec 25, 2024
Copy link
Contributor

@NonSwag NonSwag left a comment

Choose a reason for hiding this comment

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

Just wanted to leave my thoughts on some things I find a bit iffy
other than that, I'd love this getting merged, great work
side note: I didn't read everything in order, so the comments might seem random, but the explanation for it is somewhere else
edit: after reading the description i feel a bit stupid now

Copy link
Contributor

@lynxplay lynxplay left a comment

Choose a reason for hiding this comment

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

Two points:

  • While maybe not the most correct - preserving event order for open event / lid event means we need to split the openMenu calls out of initMenu() and have them be called right before the 5 places inventory open events are fired / initMenu methods are called.

  • While there is 100% not a default title that randomly appears, the nature of the menu provider specific builder implementations (BE, LocationAccess etc) does allow us to technically construct the MenuProvider for each and fetch the title from there.
    This way, LocationAccess Builder impl should get a reference to the nms.Block the menu will be sourced from so it can call the BlockBehaviour#getMenuProvider and source a default title from there.

@Y2Kwastaken Y2Kwastaken force-pushed the feature/inventory-view-builders branch 2 times, most recently from 3a3b686 to e1c9fc8 Compare January 3, 2025 21:16
@lynxplay lynxplay force-pushed the feature/inventory-view-builders branch from 19db024 to a4417dc Compare January 11, 2025 18:44
@lynxplay lynxplay merged commit c949225 into PaperMC:main Jan 11, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature Request for a new Feature.
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

8 participants