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.

Sign up for GitHub

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

Jump to bottom

Data Components #10613

Closed
wants to merge 13 commits into from
Closed

Data Components #10613

wants to merge 13 commits into from

Conversation

Owen1212055
Copy link
Member

This is like the item property api but split up into a lot of smaller chunks.

In this case, we replace item meta internally by a property map held on item stacks.

Opinions wanted:

  • names
  • serialization logic

@Owen1212055 Owen1212055 requested a review from a team as a code owner April 28, 2024 23:55
@Owen1212055 Owen1212055 mentioned this pull request Apr 28, 2024
Closed
6 tasks
@Owen1212055 Owen1212055 added the build-pr-jar Enables a workflow to build Paperclip jars on the pull request. label Apr 28, 2024
powercasgamer
powercasgamer suggested changes Apr 29, 2024
View reviewed changes
patches/server/1047-WIP-DataKey-API.patch Outdated
return result;
}

+ // Paper
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
+ // Paper
+ // Paper start

patches/server/1047-WIP-DataKey-API.patch Outdated
Comment on lines 47 to 48
+
+
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
+
+

patches/api/0474-WIP-DataKey-API.patch Outdated
public enum Material implements Keyed, Translatable, net.kyori.adventure.translation.Translatable { // Paper
//<editor-fold desc="Materials" defaultstate="collapsed">
- AIR(9648, 0),
+ AIR(9648, 64), // Paper - air techncially stacks to 64
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
+ AIR(9648, 64), // Paper - air techncially stacks to 64
+ AIR(9648, 64), // Paper - air technically stacks to 64

@Malfrador
Copy link
Member

Malfrador commented Apr 29, 2024 •
edited
Loading

Not using vanilla names but "Datakey" instead is really confusing. Mojang officially (in changelogs) calls them "Item Stack Components" or "Data Components" and the Paper API should use a similar name - everything else is just confusing to users.

itemStack.getDataKeyMap().remove(DataKeys.HIDE_ADDITIONAL_TOOLTIP);
is a lot more confusing than just
itemStack.getComponents().remove(ItemComponent.HIDE_ADDITIONAL_TOOLTIP);

I don't think adventure components existing justifies deviating from vanilla naming so much. The distinction between a Text Component and a Item Component is clear enough.

Imagine someone wanting to look up a component they found in the API on the Minecraft Wiki, or the other way around. They wouldn't even know what search term they are looking for.
This is also gonna get worse if Mojang decides to adapt the same component system for other parts of the game, such as entities or block entities.

kennytv
kennytv reviewed Apr 29, 2024
View reviewed changes
Copy link
Member

@kennytv kennytv left a comment

Choose a reason for hiding this comment

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

General ideas

patches/api/0475-WIP-DataKey-API.patch Outdated
+
+public interface DataKeyMap {
+
+ static DataKeyMap empty() {
Copy link
Member

Choose a reason for hiding this comment

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

Is there any use for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

So you can wrap around a patched map

patches/api/0475-WIP-DataKey-API.patch Outdated
Comment on lines 239 to 245
+ DataKeyMapPatch asPatch();
+
+ void applyPatch(@NotNull DataKeyMapPatch patch);
Copy link
Member

Choose a reason for hiding this comment

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

This should just accept/return PatchedDataKeyMap directly and possibly be moved to ItemStack or a separate interface

Copy link
Member Author

@Owen1212055 Owen1212055 Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do we not want some kind of middle man object? So for example people could apply certain changes to multiple item stacks? (I would allow people to make their own patch sets)

kashike
kashike requested changes Apr 29, 2024
View reviewed changes
@kashike kashike added the type: feature Request for a new Feature. label Apr 29, 2024
@Owen1212055
Copy link
Member Author

Owen1212055 commented Apr 29, 2024 •
edited
Loading

General changes:

  • DataKey -> DataComponentType (legit just went with vanilla names, we can trim names if need be)
  • PatchedDataComponentMap no longer has remove, but has unset, and reset. Which I think make the most sense in this case.
    • Reset will remove the patch present on the map
    • Unset will unset it (so, like !minecraft:tool)
  • ItemStack now implements PatchedDataKeyMapHolder which contains methods like setData which can be accessed on ItemStack directly, rather than needing to call components() evey time

Builder Types:

  • Unbreakable: Unbreakable.unbreakable().showInTooltip(false).build()
  • ItemLore: ItemLore.lore().lore(List.of(Component.text("h"))).build()

It should be noted that there are builder for most components for future-proofing. Advanced objects that might hold a bit more context to them...

Additionally it helps gain a little more power in the data component API (such has being able to get the styled tooltip from the lore)

kennytv
kennytv reviewed Apr 30, 2024
View reviewed changes
patches/api/0475-WIP-DataKey-API.patch Outdated
+
+import org.jetbrains.annotations.Contract;
+
+public interface AbstractShownInTooltip<T> {
Copy link
Member

@kennytv kennytv Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No likey the "Abstract" prefix. Though in any case, I would move this to a different package

@Owen1212055
Copy link
Member Author

Added new pot decoration / charged projectiles type.

@kashike kashike changed the title DataKey API WIP Data Components May 5, 2024
@kashike kashike added the status: in progress Issue is currently being worked on label May 5, 2024
@Potothingi

This comment was marked as off-topic.

Sign in to view
@Owen1212055
Copy link
Member Author

Superceded by #10845 to allow for easier contribution by other paper team members.

@Owen1212055 Owen1212055 closed this Jun 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yannicklamprecht yannicklamprecht yannicklamprecht left review comments

@kennytv kennytv kennytv left review comments

@masmc05 masmc05 masmc05 left review comments

@kashike kashike kashike requested changes

@powercasgamer powercasgamer powercasgamer requested changes

Assignees
No one assigned
Labels
build-pr-jar Enables a workflow to build Paperclip jars on the pull request. status: in progress Issue is currently being worked on type: feature Request for a new Feature.
Projects
Paper PR Queue
Status: Closed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@Owen1212055 @Malfrador @Potothingi @yannicklamprecht @kashike @kennytv @powercasgamer @masmc05 @Machine-Maker