-
-
Notifications
You must be signed in to change notification settings - Fork 674
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
base: api/2.4.2
Are you sure you want to change the base?
Move various settings from NonVanillaCustomItemData to CustomItemData and allow use in JSON mappings #4655
Conversation
…om items - updating implementation soon
…or type in vanilla custom registry
api/src/main/java/org/geysermc/geyser/api/item/custom/CustomItemData.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/geysermc/geyser/api/item/custom/CustomItemData.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/geysermc/geyser/api/item/custom/NonVanillaCustomItemData.java
Outdated
Show resolved
Hide resolved
Code looks fine to me, but will delegate to someone else for API approval. |
api/src/main/java/org/geysermc/geyser/api/item/custom/CustomItemData.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/geysermc/geyser/api/item/custom/CustomItemData.java
Show resolved
Hide resolved
api/src/main/java/org/geysermc/geyser/api/item/custom/CustomItemData.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/geysermc/geyser/api/item/custom/CustomItemData.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/geysermc/geyser/api/item/custom/CustomItemData.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/geysermc/geyser/api/item/custom/CustomItemData.java
Show resolved
Hide resolved
api/src/main/java/org/geysermc/geyser/api/item/custom/CustomItemData.java
Outdated
Show resolved
Hide resolved
Co-authored-by: chris <github@onechris.mozmail.com>
24a36cc
to
e55eaa7
Compare
…omitemapi # Conflicts: # core/src/main/java/org/geysermc/geyser/registry/populator/CustomItemRegistryPopulator.java
api/src/main/java/org/geysermc/geyser/api/item/custom/CustomItemData.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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!
There was a problem hiding this 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.
There was a problem hiding this 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
core/src/main/java/org/geysermc/geyser/item/GeyserCustomItemData.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/geysermc/geyser/item/GeyserCustomItemData.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/geysermc/geyser/api/item/custom/CustomItemData.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/geysermc/geyser/api/item/custom/CustomItemData.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/geysermc/geyser/item/GeyserCustomItemData.java
Outdated
Show resolved
Hide resolved
…s, and add proper method argument annotations for these methods
core/src/main/java/org/geysermc/geyser/item/GeyserCustomItemData.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/geysermc/geyser/item/GeyserCustomItemData.java
Outdated
Show resolved
Hide resolved
…ction value values
…ment proper fallbacks
We may want to note in the docs (maybe in the builder?) how setting these new values is client-side. |
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\""); | ||
} |
There was a problem hiding this comment.
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?
This PR moves various settings from the
NonVanillaCustomItemData
to theCustomItemData
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:
NonVanillaCustomItemData.Builder
class has been given overrides for the moved properties to allow for full backwards compatibility.isTool
boolean field in the implementation ofNonVanillaCustomItemData
(GeyserNonVanillaCustomItemData
) has been removed. It was already replaced withdisplayHandheld
and wasn't used anywhere in the implementation anymore. It was not publicly available.GeyserCustomItemData
has been simplified to simply take its builder as argument.stackSize
andmaxDamage
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:createComponentNbt
method forCustomItemData
has been updated to continue supportingGeyserMappingItem
as well.MappingsReader_v1
has been updated accordingly.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!