Skip to content

EnchantItemEvent#1954

Merged
Faithcaio merged 1 commit into
stable-7from
feature/enchantevents
Sep 21, 2019
Merged

EnchantItemEvent#1954
Faithcaio merged 1 commit into
stable-7from
feature/enchantevents

Conversation

@Faithcaio

@Faithcaio Faithcaio commented Jan 26, 2019

Copy link
Copy Markdown
Contributor

SpongeAPI | SpongeCommon

3 new Events:

  • EnchantItemEvent.CalculateLevelRequirement first event ; calculates the level requirement for each enchant
  • EnchantItemEvent.CalculateEnchantment second event ; calculates the list of enchantments to apply - fires twice - once for the preview then again when actually enchanting - plugins should return the same list for the same input
  • EnchantItemEvent.Post the final event ; contains the slot transactions of the enchanted item getting changed and the lapis used.

For reference:
#1303

Comment thread src/main/java/org/spongepowered/api/event/item/inventory/EnchantItemEvent.java Outdated
Comment thread src/main/java/org/spongepowered/api/event/item/inventory/EnchantItemEvent.java Outdated
Comment thread src/main/java/org/spongepowered/api/event/item/inventory/EnchantItemEvent.java Outdated
Comment thread src/main/java/org/spongepowered/api/event/item/inventory/EnchantItemEvent.java Outdated

@liach liach left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Beyond all these issues, you need to remove the implnotes from the api.

Comment thread src/main/java/org/spongepowered/api/event/item/inventory/EnchantItemEvent.java Outdated
Comment thread src/main/java/org/spongepowered/api/event/item/inventory/EnchantItemEvent.java Outdated
Comment thread src/main/java/org/spongepowered/api/event/item/inventory/EnchantItemEvent.java Outdated
EnchantmentList with(int seed, int slot, int level, boolean treasure);
// redo random vanilla calculation - with treasure enchantments
// EnchantmentHelper.buildEnchantmentList
EnchantmentList withTreasure(boolean treasure);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can curse be optional as well?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Look like curses are treasure enchantments.
Not allowing curses would require us to write our own version for getting random enchantments.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Treasure and Curse are separate - there are only two curses (binding_curse, vanishing_curse), while there are 4 treasures (binding_curse, frost_walker, mending, vanishing_curse).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

EnchantmentHelper#buildEnchantmentList(Random randomIn, ItemStack itemStackIn, int level, boolean allowTreasure) doesn't allow filtering curses.
We can still add it to the API if its doable.

Comment thread src/main/java/org/spongepowered/api/event/item/inventory/EnchantItemEvent.java Outdated
Comment thread src/main/java/org/spongepowered/api/event/item/inventory/EnchantItemEvent.java Outdated
@Faithcaio Faithcaio force-pushed the feature/enchantevents branch from a4141e6 to 933d922 Compare February 2, 2019 09:38
@Faithcaio Faithcaio changed the base branch from stable-7 to 1.13 February 3, 2019 10:26
@Faithcaio Faithcaio changed the base branch from 1.13 to stable-7 February 3, 2019 10:27
@Faithcaio Faithcaio force-pushed the feature/enchantevents branch 2 times, most recently from 59390ab to 487764c Compare February 3, 2019 10:29
@Faithcaio Faithcaio changed the base branch from stable-7 to 1.13 February 3, 2019 10:29
@Faithcaio Faithcaio force-pushed the feature/enchantevents branch from 487764c to ace77ed Compare February 3, 2019 10:40
@Faithcaio Faithcaio force-pushed the feature/enchantevents branch 2 times, most recently from 2faacf2 to 6c067b3 Compare February 8, 2019 21:49
*
* @return the slot of the enchanting item.
*/
Slot getEnchantingSlot();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

getOutputSlot instead? How do we want to word these? Other Tiles that have logic could have similar events (like the Anvil one we already do)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Its the also the input slot.

/**
* Triggers when the item was enchanted.
*/
interface EnchantItem extends EnchantItemEvent, ChangeInventoryEvent {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe call this Post? The docs imply the action occurred, the name of the event implies it is about to occur.


// Utility functions to generate enchantment lists

/**

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The below two methods feel like they belong elsewhere. What if I want to build an EnchantmentList but in my code I don't have an EnchantmentList already on hand? How would I do this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Atm. you cant do it.
I guess if you wanted to emulate an enchanting table somewhere else you might need this.
We would have to expose it in some kind of builder class then?

@Faithcaio Faithcaio force-pushed the feature/enchantevents branch from b43aeaf to c3e8e16 Compare August 11, 2019 13:25
@Faithcaio Faithcaio changed the base branch from 1.13 to stable-7 August 11, 2019 13:25
@Faithcaio Faithcaio force-pushed the feature/enchantevents branch 3 times, most recently from d7d0ab8 to 4da8348 Compare August 11, 2019 17:25
@Faithcaio Faithcaio force-pushed the feature/enchantevents branch from 4da8348 to 46ac2d4 Compare September 15, 2019 10:52
@Faithcaio Faithcaio force-pushed the feature/enchantevents branch from 46ac2d4 to 81fca20 Compare September 21, 2019 19:23
@Faithcaio Faithcaio merged commit 57b0177 into stable-7 Sep 21, 2019
@Faithcaio Faithcaio deleted the feature/enchantevents branch September 21, 2019 19:33
@ImMorpheus ImMorpheus mentioned this pull request Aug 24, 2020
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants