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

Move various settings from NonVanillaCustomItemData to CustomItemData and allow use in JSON mappings #4655

Open
wants to merge 26 commits into
base: api/2.4.2
Choose a base branch
from

Conversation

eclipseisoffline
Copy link
Contributor

@eclipseisoffline eclipseisoffline commented May 11, 2024

This PR moves various settings from the NonVanillaCustomItemData to the CustomItemData class, which is useful for datapack/server side mod developers when porting custom items to Bedrock using Geyser's mapping system.

Properties that have been moved to the super-interface:

  • stackSize
  • maxDamage
  • attackDamage
  • armorType
  • protectionValue
  • isHat
  • isFoil
  • isEdible
  • canAlwaysEat

The builders for these classes have been correctly updated, alongside the implementations of these classes and their use in CustomItemRegistryPopulator.

A few notes:

  • The NonVanillaCustomItemData.Builder class has been given overrides for the moved properties to allow for full backwards compatibility.
  • The private isTool boolean field in the implementation of NonVanillaCustomItemData (GeyserNonVanillaCustomItemData) has been removed. It was already replaced with displayHandheld and wasn't used anywhere in the implementation anymore. It was not publicly available.
  • The constructor of GeyserCustomItemData has been simplified to simply take its builder as argument.
  • To not change the behaviour when the stack size is unset when extending vanilla items (in that case the Java base item's stack size is used), the stack size property has been changed slightly in implementation, which shouldn't be noticeable for API users. The Javadoc has been updated correctly:
    • Returns 0 if not set. When not set, Geyser defaults to the stack count of the Java item when based on a vanilla item, or 64 when registering a non-vanilla item.

  • To not change the behaviour when the max damage is unset when extending vanilla items (in that case the Java base item's max damage is used), the max damage property has been changed slightly in implementation, which shouldn't be noticeable for API users. The Javadoc has been updated correctly:
    • Returns -1 if not set. When not set, Geyser defaults to the maximum damage of the Java item when based on a vanilla item, or uses 0 when registering a non-vanilla item.

  • The stackSize and maxDamage methods now properly check that their values are compatible to each other, following the logic of Java Edition (so, when stack size is above 1, max damage has to be 0, and when max damage is above 0, stack size has to be 1). The Javadoc has been updated correctly:
    • Note that, to copy Java behaviour, setting the stack size of an item to a value above 1 will set the max damage to 0. If a max damage value above 0 was explicitly set, an exception will be thrown.

    • Note that, to copy Java behaviour, setting the max damage value of an item to a value above 0 will set the stack size to 1. If a stack size above 1 was explicitly set, an exception will be thrown.

  • The createComponentNbt method for CustomItemData has been updated to continue supporting GeyserMappingItem as well.
  • All properties moved are now supported in JSON item mappings as well, the MappingsReader_v1 has been updated accordingly.
  • The wiki has to be updated to reflect the changes (I could make the PR).

All of the new JSON mapping properties have been tested, and backwards compatibility of using builder methods in NonVanillaCustomItemData.Builder in extensions has been tested as well, which all seem to work okay.

Feel free to suggest any changes and I'll be sure to implement them!

@Camotoy
Copy link
Member

Camotoy commented May 12, 2024

Code looks fine to me, but will delegate to someone else for API approval.

@onebeastchris onebeastchris added the PR: Feature When a PR implements a new feature label May 28, 2024
…omitemapi

# Conflicts:
#	core/src/main/java/org/geysermc/geyser/registry/populator/CustomItemRegistryPopulator.java
Copy link
Member

@onebeastchris onebeastchris left a comment

Choose a reason for hiding this comment

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

Overall, looks good to me - please also target the api/2.4.1 branch, and we'll be able to include this in the upcoming 2.4.1 release. Thanks!

@eclipseisoffline eclipseisoffline changed the base branch from master to api/2.4.1 July 24, 2024 07:54
Copy link
Member

@onebeastchris onebeastchris left a comment

Choose a reason for hiding this comment

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

Looks good to me now, thanks!
Please target the newly created api/2.4.2 branch :) Might need a rebase due to the way api/2.4.1 was merged into master though.

Copy link
Member

@Konicai Konicai left a comment

Choose a reason for hiding this comment

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

thank you! sorry for the late review

@Konicai Konicai changed the base branch from api/2.4.1 to api/2.4.2 August 2, 2024 01:21
@Konicai Konicai added this to the api/2.4.2 milestone Aug 2, 2024
@Konicai Konicai added the API The issue/feature request relates to the Geyser API label Aug 2, 2024
@Konicai Konicai changed the title Move various settings from non vanilla custom item data to custom item data and allow use in JSON mappings Move various settings from NonVanillaCustomItemData to CustomItemData and allow use in JSON mappings Aug 2, 2024
…s, and add proper method argument annotations for these methods
@Camotoy
Copy link
Member

Camotoy commented Aug 23, 2024

We may want to note in the docs (maybe in the builder?) how setting these new values is client-side.

Comment on lines 323 to 326
public Builder armorType(@Nullable String armorType) {
if (!VALID_ARMOR_TYPES.contains(armorType)) {
throw new IllegalArgumentException("Invalid armor type " + armorType + "! Can be \"boots\", \"leggings\", \"chestplate\", or \"helmet\"");
}
Copy link
Member

Choose a reason for hiding this comment

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

do we want to force it to lowercase?

@onebeastchris onebeastchris removed this from the api/2.4.2 milestone Aug 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API The issue/feature request relates to the Geyser API PR: Feature When a PR implements a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants