FireworkMeta#setPower Javadoc information incorrect

[TehBrian]

Expected behavior

Setting the FireworkMeta's power to 128 should succeed, as per the Javadoc:

/**
 * Sets the approximate power of the firework. Each level of power is half
 * a second of flight time.
 *
 * @param power the power of the firework, from 0-128
 * @throws IllegalArgumentException if {@literal height<0 or height>128}
 */
void setPower(int power) throws IllegalArgumentException;

Observed/Actual behavior

Calling fireworkMeta.setPower(128); resulted in an IllegalArgumentException:

[19:21:18 WARN]: java.lang.IllegalArgumentException: Power cannot be more than 127: 128
[19:21:18 WARN]: 	at org.apache.commons.lang.Validate.isTrue(Validate.java:93)
[19:21:18 WARN]: 	at org.bukkit.craftbukkit.v1_18_R1.inventory.CraftMetaFirework.setPower(CraftMetaFirework.java:404)
(irrelevant lines omitted)

Steps/models to reproduce

Run the following code in a plugin:

final ItemStack item = new ItemStack(Material.FIREWORK_ROCKET);
if (item.getItemMeta() instanceof FireworkMeta firework) {
    firework.setPower(128);
}

Plugin and Datapack List

Plugins (1): Iteminator
There are 2 data packs enabled: [vanilla (built-in)], [file/bukkit (world)]

Paper version

This server is running Paper version git-Paper-175 (MC: 1.18.1) (Implementing API version 1.18.1-R0.1-SNAPSHOT) (Git: 9490475)
You are running the latest version

Other

No response

[Doc94]

Created a PR in Upstream clarify the limits in the setPower.
https://hub.spigotmc.org/stash/projects/SPIGOT/repos/bukkit/pull-requests/720/overview

[Phoenix616]

A related issue is that the lower boundary is set to 0 while in Vanilla firework up to -128 are allowed which breaks firework's serialisation (https://paste.gg/p/Pencil/d0c487237c824916b46509b082003e9b)

[Doc94]

A related issue is that the lower boundary is set to 0 while in Vanilla firework up to -128 are allowed which breaks firework's serialisation (https://paste.gg/p/Pencil/d0c487237c824916b46509b082003e9b)

for what works a power in negative values?

[Phoenix616]

A related issue is that the lower boundary is set to 0 while in Vanilla firework up to -128 are allowed which breaks firework's serialisation (https://paste.gg/p/Pencil/d0c487237c824916b46509b082003e9b)

for what works a power in negative values?

Are you asking what they do? If so it basically makes the firework explode instantly. (I was under the impression that 0 does that too though)

Don't think they are really used anywhere but at least the internal validation in the CraftMetaFirework#setPower method should not error on those in order for those items to be properly serializable/loadable as Bukkit ItemStacks. Whether or not the api should actually support that is questionable though.

[TehBrian]

Whether or not the api should actually support that is questionable though.

The API should aim to reflect Minecraft's feature-set as best as possible, no? Of course, the counterargument to that would be that there are many Minecraft features that Bukkit doesn't support through API. However, in this specific case regarding fireworks, API/internal parity could be achieved simply by not enforcing an incorrect restriction.

[Doc94]

A related issue is that the lower boundary is set to 0 while in Vanilla firework up to -128 are allowed which breaks firework's serialisation (https://paste.gg/p/Pencil/d0c487237c824916b46509b082003e9b)

for what works a power in negative values?

Are you asking what they do? If so it basically makes the firework explode instantly. (I was under the impression that 0 does that too though)

Don't think they are really used anywhere but at least the internal validation in the CraftMetaFirework#setPower method should not error on those in order for those items to be properly serializable/loadable as Bukkit ItemStacks. Whether or not the api should actually support that is questionable though.

hmm i ask because in past PR i has comments related to not validate info in wiki.. i mean if less of zero works equals than zero, then the issue is most the wiki than the api (?

Whether or not the api should actually support that is questionable though.

The API should aim to reflect Minecraft's feature-set as best as possible, no? Of course, the counterargument to that would be that there are many Minecraft features that Bukkit doesn't support through API. However, in this specific case regarding fireworks, API/internal parity could be achieved simply by not enforcing an incorrect restriction.

well im sure vanilla allow any number in the nbt now if this values works is another thing...

[Phoenix616]

well im sure vanilla allow any number in the nbt now if this values works is another thing...

No it doesn't. NBT has types and e.g. the power is a Byte and not an Integer as seen on the linked wiki page. Of course whether or not the negative power is an intended behaviour is a different question and is basically the same as the question on whether or not the API should support it but that is more secondary. The fact that those items break and are unusable for plugins accessing that serialised meta while they work in Vanilla is on another level.