Skip to content

make bookshelf power configurable and refresh enchantment hint of menu after editing bookshelf power #12345

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

I-am-ddang
Copy link

No description provided.

@I-am-ddang I-am-ddang requested a review from a team as a code owner March 27, 2025 08:04
@github-project-automation github-project-automation bot moved this to Awaiting review in Paper PR Queue Mar 27, 2025
@lynxplay
Copy link
Contributor

Welcome to paper 🥳

It appears you commit / generate a whole slew of completely unrelated and invalid changes.
Please make sure to remove them so we can review your PR change.

@electronicboy
Copy link
Member

I would suggest rebasing this PR or creating a new one

@I-am-ddang
Copy link
Author

I would suggest rebasing this PR or creating a new one

I don't want to create a new one because this is my first PR and its number is 12345. I will look into rebasing the codes

@I-am-ddang I-am-ddang force-pushed the feat-enchantment-hint-with-configurable-bookshelf branch 2 times, most recently from 8248809 to 338303f Compare March 30, 2025 09:14
@I-am-ddang
Copy link
Author

wait. it is not done. I will inform it is ready for review when I end all of rebase procedure

@I-am-ddang I-am-ddang force-pushed the feat-enchantment-hint-with-configurable-bookshelf branch 2 times, most recently from 64bc304 to 597f186 Compare March 30, 2025 09:53
@I-am-ddang
Copy link
Author

I did it. I am now the master of rebase. I am ready for review.

@I-am-ddang
Copy link
Author

@lynxplay @electronicboy
I'd glad for you to take a look whenever you are not busy guys. thanks for your patient.

@I-am-ddang I-am-ddang closed this Apr 6, 2025
@I-am-ddang I-am-ddang force-pushed the feat-enchantment-hint-with-configurable-bookshelf branch from 597f186 to 894631f Compare April 6, 2025 09:56
@github-project-automation github-project-automation bot moved this from Awaiting review to Closed in Paper PR Queue Apr 6, 2025
@I-am-ddang
Copy link
Author

wait I will reopen PR right now. it was accident

@I-am-ddang I-am-ddang reopened this Apr 6, 2025
@I-am-ddang I-am-ddang force-pushed the feat-enchantment-hint-with-configurable-bookshelf branch 2 times, most recently from 23aa125 to 7adadc3 Compare April 17, 2025 03:43
@I-am-ddang
Copy link
Author

I am not sure if I do it right. waiting for review.

@Lulu13022002 Lulu13022002 moved this from Closed to Awaiting review in Paper PR Queue Apr 17, 2025
@cwrayne
Copy link

cwrayne commented Apr 22, 2025

THIS IS PULL REQEUST #12345 AND I LOVE IT (soory this might be off topic but i had to say it)

Copy link
Contributor

@Strokkur424 Strokkur424 left a comment

Choose a reason for hiding this comment

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

What I find a bit unfortunate is that changing the bonus power in the event basically makes any changes in the offers redundant, as you reroll these anyways.

I don't know if there is a particularly clean way to fix this. I personally can also only think of one way to fix this, but I understand it is not very advantageous:

  • The introduction of a new RetrieveItemEnchantPowerEvent, which allows for setting the enchantment bonus before the offers are even put into consideration.

Therefore, the actual PrepareItemEnchantEvent could stay unmodified and enchantment offer modifications would still work as intended.

Of course, before you make this change, I'd like a Paper developer to confirm that this is an acceptable step to take, because I am not sure myself either.

@github-project-automation github-project-automation bot moved this from Awaiting review to Changes required in Paper PR Queue Apr 22, 2025
@I-am-ddang
Copy link
Author

I will fix the build error as soon as possible.

@I-am-ddang
Copy link
Author

I-am-ddang commented Apr 22, 2025

@Strokkur424 your idea is previous my idea.

when I discussed this feature at Paper discord's paper-dev channel, I suggest that structure but lynxplay adviced me that, in his opinion, making the event just set value is not good idea

(the discord dialog link that starts to discuss this feature with lynxplay: https://discord.com/channels/289587909051416579/555462289851940864/1347917294382219417)

So I should change original PrepareItemEnchantEvent. Yeah. I also think it would be good if offers method have more role. and I think "why doesn't it process the values of offers member field at constructor point with considering the bonus value is changed? it would be easy way to handle the logic when making an event." but that is not really feasible. because paper-api package can not import the code from paper-src. (I don't know the principle why it should be like it.)

Finally my decision is making bonus member field non final instead of making new event to set the bonus value and to include the other fields of PrepareItemEnchantEvent, checking the value and refreshing the enchantment hint after PrepareItemEnchantEvent calls. (it is now structure.)

@Strokkur424
Copy link
Contributor

Well, we still need some way to modify the new enchantment suggestions, because this is a valid thing a plugin would do:

  • Add 5 to the bookshelf bonus.
  • Replace smite and band of arthropods with sharpness.

Not entirely sure how this can be achieved cleanly though.

@I-am-ddang
Copy link
Author

@Strokkur424
You are right. refreshing enchantment is needed hint when replacing one of offers to other enchantment. Because paper-src codes can not be imported to paper-api, we should process the entire refreshing logic at paper-src/.../PrepareItemEnchantEvent.java (like logics of my PR)

@lynxplay
Copy link
Contributor

I am sorry I haven't managed to read through this for so long.
The suggestion I made on discord was

Yea, I think there might be a point in adding API. Tho I don't think I'd like an API that says "just set that value" of bookshelves around.
The more powerful API on e.g. the EnchantmentView or whereever would be
Construct me an EnchantmentOFfer for this slot with this amount of bookshelves around

which I still believe to hold true.
Allowing a plugin to reroll specific offers and then setting them via Event#getOffers would enable doing all of this without the need for a second event.

@I-am-ddang I-am-ddang force-pushed the feat-enchantment-hint-with-configurable-bookshelf branch from 7adadc3 to 2add8da Compare April 23, 2025 09:27
@I-am-ddang
Copy link
Author

I-am-ddang commented Apr 23, 2025

@lynxplay Yeah. and I don't make another event.

it is a simple quality-of-life feature update for users who want an enchantment offers of specific level of bonus.

I fix compile error of last commit.

@I-am-ddang I-am-ddang force-pushed the feat-enchantment-hint-with-configurable-bookshelf branch from 2add8da to 4a66e57 Compare May 1, 2025 07:19
@I-am-ddang
Copy link
Author

I retry same fix. before this, it is not properly updated. now this PR should pass the test.

@lynxplay lynxplay moved this from Changes required to Awaiting review in Paper PR Queue May 6, 2025
@lynxplay lynxplay moved this from Awaiting review to Changes required in Paper PR Queue May 6, 2025
I-am-ddang and others added 3 commits May 7, 2025 12:40
…t of menu after editing bookshelf power"

This reverts commit b56273b.
Introduces a new API method that allows plugins to simulate the rolling
process of an enchantment table for enchantment offers. This is
specifically useful to "reroll" an enchantment tables offers during the
PrepareItemEnchantEvent to e.g. change the bookshelf bonus or seed.

Co-authored-by: I-am-DDang_ <[email protected]>
@lynxplay lynxplay force-pushed the feat-enchantment-hint-with-configurable-bookshelf branch from 4a66e57 to bc11611 Compare May 7, 2025 10:43
@github-project-automation github-project-automation bot moved this from Changes required to Awaiting final testing in Paper PR Queue May 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Awaiting final testing
Development

Successfully merging this pull request may close these issues.

6 participants