Cleanup of enchantments #1687

parlough · 2017-11-12T10:43:42Z

SpongeAPI | SpongeCommon | SpongeVanilla | SpongeForge

I will finish what I need to tomorrow, but for now looking for any feedback and also where I undoubtedly made mistakes as I haven't really tested much and some stuff was done automatically through IntelliJ refactoring and I may have missed mistakes.

Closes #1684

parlough · 2017-11-12T10:49:56Z

src/main/java/org/spongepowered/api/data/Queries.java

-    // Item Enchantment
-    public static final DataQuery ENCHANTMENT_ID = of("Enchantment");
+    // Item EnchantmentType
+    public static final DataQuery ENCHANTMENT_ID = of("EnchantmentType");


This was done automatically by IntelliJ. Should I keep I revert this change to not cause compatibility issues?

Anyone have input here?

Will do. 👍

liach · 2017-11-12T17:05:01Z

src/main/java/org/spongepowered/api/block/tileentity/EnchantmentTable.java

@@ -25,7 +25,7 @@
 package org.spongepowered.api.block.tileentity;

 /**
- * Represents an Enchantment Table.
+ * Represents an EnchantmentType Table.


This one is absolutely wrong.

liach · 2017-11-12T17:05:29Z

src/main/java/org/spongepowered/api/data/manipulator/catalog/CatalogItemData.java

@@ -114,7 +114,7 @@
     */
    public static final Class<DyeableData> DYEABLE_DATA = DyeableData.class;
    /**
-     * Signifies that an item has {@link Enchantment}s applied to it.
+     * Signifies that an item has {@link EnchantmentType}s applied to it.


You probably should revert this one.

liach · 2017-11-12T17:05:46Z

src/main/java/org/spongepowered/api/data/manipulator/catalog/CatalogItemData.java

@@ -169,7 +169,7 @@
     */
    public static final Class<SpawnableData> SPAWNABLE_DATA = SpawnableData.class;
    /**
-     * Signifies that there are stored {@link Enchantment}s available on the
+     * Signifies that there are stored {@link EnchantmentType}s available on the


liach · 2017-11-12T17:09:00Z

src/main/java/org/spongepowered/api/data/meta/Enchantment.java

+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+package org.spongepowered.api.data.meta;


I think we should move the package of this class. It is not really a meta any more, since it has been converted into an interface.

How do you feel about moving Enchantment, EnchantmentType, and EnchantmentTypes to a new enchantment package inside of the effect package?

liach · 2017-11-12T17:13:24Z

src/main/java/org/spongepowered/api/data/meta/Enchantment.java

+ * with a level.
+ *
+ * <p>The contract of enchantments is that their level will
+ * never be below 1.</p>


<p>The contract of enchantments is that their level will be in the range of {@code getType().getMinimumLevel()} and {@code getType().getMinimumLevel()}.</p>

Uhh yes, it can be any natural number as long as the number can be stored in data.
Does this hurt items with empty enchantment tags? Those tags may give item stacks an effect while not having a valid enchantment.

So we only check when building if it is above 0 or below Short.MAX_VALUE. There are issues if the level is below 0 if I recall correctly but in reality it can be 0(I think?). I just changed it to 1 since I didn't see a reason why an enchantment would ever need to be 0 and Minecraft doesn't really expect it with all of theirs having a minimum level of 0. Do you think I should switch back to 0 or greater?

Also to your suggestion. I don't think that is a great idea as we return the vanilla max and min which are not necessarily as high as they could be set.

I'm heavily open to feedback relating to our lower limit and how to specify it.

liach · 2017-11-12T17:16:11Z

src/main/java/org/spongepowered/api/item/EnchantmentTypes.java

+     * Increases damages to arthropod mobs. In vanilla this includes spiders,
+     * cave spiders, silverfish, and endermites.
+     *
+     * <p>In vanilla the maximum level is 5.</p>


At high level, the enchantment will slow down the arthropod hit for few seconds.

Interesting, never knew this.

liach · 2017-11-12T17:17:13Z

src/main/java/org/spongepowered/api/item/EnchantmentTypes.java

+    // SORTFIELDS:ON
+
+    /**
+     * Increases underwater mining rate.


You should say it just increase the speed to the regular mining speed.

liach · 2017-11-12T17:21:27Z

src/main/java/org/spongepowered/api/item/EnchantmentTypes.java

+
+    /**
+     * Reduces all damage, outside of a few sources, such as the void,
+     * the kill command, and hunger damage in vanilla.


Probably mention that it doesn't reduce damage that bypasses armor.

liach · 2017-11-12T17:22:17Z

src/main/java/org/spongepowered/api/item/EnchantmentTypes.java

+    public static final EnchantmentType THORNS = DummyObjectProvider.createFor(EnchantmentType.class, "THORNS");
+
+    /**
+     * RIncreases effective durability.


Remove that R I guess

liach · 2017-11-12T17:23:25Z

src/main/java/org/spongepowered/api/util/TypeTokens.java


-    public static final TypeToken<ListValue<ItemEnchantment>> LIST_ITEM_ENCHANTMENT_VALUE_TOKEN = new TypeToken<ListValue<ItemEnchantment>>() {private static final long serialVersionUID = -1;};
+    public static final TypeToken<ListValue<Enchantment>> LIST_ITEM_ENCHANTMENT_VALUE_TOKEN = new TypeToken<ListValue<Enchantment>>() {private static final long serialVersionUID = -1;};


You should rename these 2 fields.

Cybermaxke · 2017-11-13T12:12:09Z

Could we also use a custom ListValue and extra methods in the enchantment data manipulator to turn in some kind of mapped data? So you don't have to loop through all the enchantments to get the level, this could also be optimized internally.

import org.spongepowered.api.data.value.mutable.ListValue;
import org.spongepowered.api.item.EnchantmentType;
import org.spongepowered.api.item.Enchantment;

import java.util.OptionalInt;

public interface EnchantmentListValue extends ListValue<Enchantment> {

    OptionalInt getLevel(EnchantmentType enchantment);

    void add(EnchantmentType enchantment, int level);
}

ryantheleach · 2017-11-14T00:12:30Z

No we can not @Cybermaxke :(

Vanilla very occasionally will generate with multiple enchants of the same type on an item.

https://www.youtube.com/watch?v=66zHMfi-bsI

parlough · 2017-11-14T04:31:49Z

What are people's opinions on the minimum enchantment value? Should we allow it to be 0 or above, or 1 and above to match vanilla's enchantments and to avoid possible issues.

liach · 2017-11-14T04:38:27Z

@Meronat We should probably allow only positive levels, no zero. The glowing effects some items have is from an empty enchantment tag list (so the list can exist and be empty, a different concept) so level 0 enchantments are useless

parlough · 2017-11-16T08:19:28Z

These 4 PRs should be ready for more testing and review. 🥂

ryantheleach · 2017-11-16T08:29:11Z

@Meronat are we exposing the 'natural maximum' enchantment level of each item anywhere? Is it even plausible?

parlough · 2017-11-16T08:31:08Z

@ryantheleach I believe this is what you mean? https://github.com/Meronat/SpongeAPI/blob/ae6f6d92a545bdd9ff8fd40b08f77c5b724b87f8/src/main/java/org/spongepowered/api/item/enchantment/EnchantmentType.java#L62

ryantheleach · 2017-11-16T08:34:02Z

https://gaming.stackexchange.com/questions/262999/what-is-the-highest-effective-enchantment-level/263013#263013 States that some out of band negative values may have some effect. Not sure if we should handle them, or reject them outright. How do other parts of the API behave with out of band values?

Also don't know if any mods do anything outrageous.

gabizou · 2017-11-17T01:35:48Z

src/main/java/org/spongepowered/api/item/enchantment/Enchantment.java

+         *
+         * @param level The desired level
+         * @return The modified builder for chaining
+         * @throws IllegalArgumentException If the level is below 1


This contradicts the argument in the java doc for the class of Enchantment here

And I'm somewhat concerned about mod compatibility here.

There I said their level will always be larger than 0, which equates to the same thing as never below 1, but I can clean up the wording and add the max as well.

How do you propose I handle the concern? Should we simply allow them to use the full range of Short.MIN_VALUE to Short.MAX_VALUE, specifying that unintentional issues may arise if you go outside of EnchantmentType#getMinumumLevel and EnchantmentType#getMaxmimumLevel?

Should we simply allow them to use the full range of Short.MIN_VALUE to Short.MAX_VALUE, specifying that unintentional issues may arise if you go outside of EnchantmentType#getMinumumLevel and EnchantmentType#getMaxmimumLevel?

I am going to have to say "yes" because we cannot 100% control the possibilities of enchantments. If developers choose to make unworkable enchantments, that's more on them.

I will make this change and the necessary changes to the docs later tomorrow.

Edit: Sorry haven't had a chance to get around to this yet, will do soon!

parlough · 2017-11-22T05:34:55Z

Any final reviews on API or implementation? Changes work for me.

…pongePowered#1684

parlough requested review from Deamon5550, Faithcaio and gabizou as code owners November 12, 2017 10:43

This was referenced Nov 12, 2017

Cleanup of enchantments SpongePowered/Sponge#1593

Merged

Cleanup of enchantments SpongePowered/SpongeVanilla#361

Closed

Cleanup of enchantments SpongePowered/SpongeForge#1847

Closed

parlough commented Nov 12, 2017

View reviewed changes

liach reviewed Nov 12, 2017

View reviewed changes

parlough changed the title ~~[WIP] Cleanup of enchantments~~ Cleanup of enchantments Nov 16, 2017

gabizou self-assigned this Nov 17, 2017

gabizou added system: data type: enhancement api: 7 (u) version: 1.12 labels Nov 17, 2017

gabizou requested changes Nov 17, 2017

View reviewed changes

gabizou approved these changes Nov 22, 2017

View reviewed changes

JBYoshi added this to the Revision 7.0 milestone Nov 23, 2017

gabizou added the status: ready to merge label Nov 27, 2017

parlough force-pushed the feature/enchantment-cleanup branch from 662934f to 9719f9f Compare December 1, 2017 06:08

Cleanups enchantments and moves them to the implementation - Closes S…

a26b454

…pongePowered#1684

parlough force-pushed the feature/enchantment-cleanup branch from 9719f9f to a26b454 Compare December 1, 2017 06:35

parlough merged commit 8055496 into SpongePowered:bleeding Dec 1, 2017

merjilin mentioned this pull request Dec 6, 2017

Changes to Enchantment API causing breakages EpicBanItemTeam/VirtualChest#38

Closed

parlough mentioned this pull request Dec 12, 2017

Negative enchantment value causes crashes #1337

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup of enchantments #1687

Cleanup of enchantments #1687

parlough commented Nov 12, 2017 •

edited

parlough Nov 12, 2017

parlough Nov 12, 2017

kashike Nov 12, 2017

parlough Nov 12, 2017

liach Nov 12, 2017

liach Nov 12, 2017

liach Nov 12, 2017

liach Nov 12, 2017

parlough Nov 12, 2017

liach Nov 12, 2017

liach Nov 12, 2017

parlough Nov 12, 2017 •

edited

liach Nov 12, 2017

parlough Nov 12, 2017

liach Nov 12, 2017

liach Nov 12, 2017

liach Nov 12, 2017

liach Nov 12, 2017

Cybermaxke commented Nov 13, 2017

ryantheleach commented Nov 14, 2017 •

edited

parlough commented Nov 14, 2017

liach commented Nov 14, 2017

parlough commented Nov 16, 2017

ryantheleach commented Nov 16, 2017

parlough commented Nov 16, 2017

ryantheleach commented Nov 16, 2017 •

edited

gabizou Nov 17, 2017

gabizou Nov 17, 2017

parlough Nov 17, 2017

gabizou Nov 17, 2017

parlough Nov 17, 2017 •

edited

parlough commented Nov 22, 2017


		public static final TypeToken<ListValue<ItemEnchantment>> LIST_ITEM_ENCHANTMENT_VALUE_TOKEN = new TypeToken<ListValue<ItemEnchantment>>() {private static final long serialVersionUID = -1;};
		public static final TypeToken<ListValue<Enchantment>> LIST_ITEM_ENCHANTMENT_VALUE_TOKEN = new TypeToken<ListValue<Enchantment>>() {private static final long serialVersionUID = -1;};

Cleanup of enchantments #1687

Cleanup of enchantments #1687

Conversation

parlough commented Nov 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parlough Nov 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cybermaxke commented Nov 13, 2017

ryantheleach commented Nov 14, 2017 • edited

parlough commented Nov 14, 2017

liach commented Nov 14, 2017

parlough commented Nov 16, 2017

ryantheleach commented Nov 16, 2017

parlough commented Nov 16, 2017

ryantheleach commented Nov 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parlough Nov 17, 2017 • edited

Choose a reason for hiding this comment

parlough commented Nov 22, 2017

parlough commented Nov 12, 2017 •

edited

parlough Nov 12, 2017 •

edited

ryantheleach commented Nov 14, 2017 •

edited

ryantheleach commented Nov 16, 2017 •

edited

parlough Nov 17, 2017 •

edited