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

Remove offset on certain potion strength values #3592

Closed
wants to merge 1 commit into from
Closed

Remove offset on certain potion strength values #3592

wants to merge 1 commit into from

Conversation

uf0h
Copy link
Contributor

@uf0h uf0h commented Aug 13, 2020

fixes #3589

@uf0h uf0h changed the title remove upper limit remove upper limit on potion creation Aug 13, 2020
@pop4959
Copy link
Member

pop4959 commented Aug 13, 2020

Are you sure that check wasn't important for something else in the code?

@pop4959 pop4959 added the bug: confirmed Confirmed bugs in EssentialsX. label Aug 13, 2020
@uf0h
Copy link
Contributor Author

uf0h commented Aug 13, 2020

Have checked usage with power at crazier levels (+50) and it works as expected.

I'm of the opinion if any user opts to set potion power a to crazier amount it's their responsibility but let me know what you think about setting an upper limit; just this time higher than 4.

@pop4959
Copy link
Member

pop4959 commented Aug 13, 2020

Have you checked all the way back to Minecraft 1.8? Sometimes weird things like this result from incompatibilities in older versions. Although it could very well be that this check isn't needed anymore, just needs to be absolutely 100% confirmed so that we aren't reintroducing a bug or something.

@uf0h
Copy link
Contributor Author

uf0h commented Aug 13, 2020

Shows working effects: link
Shows lore of potions: link

@pop4959
Copy link
Member

pop4959 commented Aug 13, 2020

Ah I see, maybe it would be smart to still add a potions.unsafe permission node + config similar to the enchantments stuff, for the levels that do not display properly in game.

Copy link

@VoltroXP VoltroXP 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 👍

@mdcfe mdcfe changed the title remove upper limit on potion creation Remove offset on certain potion strength values Aug 22, 2020
@mdcfe
Copy link
Member

mdcfe commented Aug 22, 2020

ce51a07 added this offset behaviour so that power:<value> meta tags would match the names that vanilla uses for each amplifier value - potion amplifier values are 1 less than the value displayed in the name, so Strength I actually has an amplifier value of 0, Strength III has a value of 2 etc. For some reason, ce51a07 only applies this shift to inputs between 1 and 3 (my guess is IV/V/VI didn't have any vanilla usage at this point), but values over and including 4 are not adjusted.

To clarify: EssentialsX has never enforced an upper limit on potion strengths, and this PR doesn't remove such a limit. Instead, this PR removes the limit on the values that get offset by -1, so that all positive values are adjusted.

Changing this check will change the power applied for all potions in kits, /give, /item and /potion. It's not infeasible for servers to intentionally use levels above 4 with custom language files (or even just vanilla for IV/V/VI), and for that reason I'm not sure that breaking this is the best way to give people access to level IV potions.

I'd prefer we added an alternative syntax that doesn't attempt to offset potion effect strength, as this would avoid breaking existing kits/potions. I've opened #3614 with an implementation of this, which allows you to replace power:<value> with amplifier:<value> to use untranslated amplifier values instead of partially-translated ones.

@mdcfe mdcfe closed this in #3614 Aug 22, 2020
mdcfe added a commit that referenced this pull request Aug 22, 2020
Adds an `amplifier:<value>` potion meta attribute to MetaItemStack that applies a raw amplifier value, instead of translating inputs between 1 and 3 to match their vanilla names like `power:<value>` does. This matches the Mojang `/effect` command, which doesn't translate any potion effect amplifiers, and allows for creation of level IV potions (using `amplifier:3` instead of a `power` value) through EssentialsX without breaking existing usages of `power:<value>`.

More context for this commit can be found at #3592 (comment).

Closes #3592 and fixes #3589.
JRoy pushed a commit to JRoy/Essentials-PR that referenced this pull request Aug 30, 2020
Adds an `amplifier:<value>` potion meta attribute to MetaItemStack that applies a raw amplifier value, instead of translating inputs between 1 and 3 to match their vanilla names like `power:<value>` does. This matches the Mojang `/effect` command, which doesn't translate any potion effect amplifiers, and allows for creation of level IV potions (using `amplifier:3` instead of a `power` value) through EssentialsX without breaking existing usages of `power:<value>`.

More context for this commit can be found at EssentialsX#3592 (comment).

Closes EssentialsX#3592 and fixes EssentialsX#3589.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug: confirmed Confirmed bugs in EssentialsX.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot get potions with health boost power 4
4 participants