FireworkMeta inconsistencies with vanilla

[Machine-Maker]

Expected behavior

FireworkMeta#get/setPower methods respect Vanilla bounds on the power.

Observed/Actual behavior

FireworkMeta#setPower restricts the to >= 0 and < 128 when a value of

Steps/models to reproduce

To produce the error linked below, just serialize and then deserialize the itemstack you get from /give @p minecraft:fireworks 1 0 {Fireworks:{Flight:128,Explosions:[]}}

Plugin and Datapack List

just my test plugin

Paper version

This server is running Paper version git-Paper-"41263d9" (MC: 1.18.1) (Implementing API version 1.18.1-R0.1-SNAPSHOT) (Git: 41263d9)
You are running the latest version
Previous version: git-Paper-"cccb5d8" (MC: 1.18.1)

Other

Mentioned in #7409, but should have its own issue because there's a discussion to be had about the correct fix.

Issue

Minecraft uses the byte nbt tag to store the firework rocket's power (or flight) which produces values of between -128 and 127 (inclusive). In the ctor for CraftMetaFirework, that value is converted to an unsigned int with

this.power = 0xff & fireworks.getByte(FLIGHT.NBT);

which will produce values between 0 and 255 (inclusive).
That value is then returned by FireworkMeta#getPower.

When the itemmeta is serialized with craftbukkit's api serialization for metas, that 0-255 value is stored, but when it's deserialized
this happens. (from @Phoenix616)

Possible solutions

Allow values between 0 and 255 to be passed into setPower and note that values > 127 will be turned into negative numbers.
OR
Keep the validations and just in fix the problem in deserialization.

Either one I think requires some note in the javadocs that getPower can return between 0 and 255 not 0-127 that might be inferred from the javadocs on setPower

[Machine-Maker]

Here is the Mojira issue confirming negative values for flight/power as WAI

[Phoenix616]

Allow values between 0 and 255 to be passed into setPower and note that values > 127 will be turned into negative numbers.

If this is really supported by the API then I think it should allow values from -128 to 127 to correctly describe the actual behaviour as well as match up with all the other docs on the firework power/flight out there (e.g. the Minecraft wiki). Also don't think end-users of the API should be bothered with implementation details that the overflows/(un)signed bytes logic used internally is.

[Doc94]

Yeah like mention Phoenix its better change the range in the validation.. when make the PR for the docs issue dont notice the behaviour for negative values... then maybe its better only allow the negative bounds..

[Machine-Maker]

But its not that simple. It's serialized as 0-255. So if the validation in setPower is changed to -128 to 127, then the deserialization ctor needs to have its own check that its between 0 and 255.

Also, there has to be some mention of the whole overflow thing, cause a plugin could do setPower(-128) and then getPower returns what... 128? or -128?

getPower returns an int between 0 and 255 right now.

[Phoenix616]

But its not that simple. It's serialized as 0-255. So if the validation in setPower is changed to -128 to 127, then the deserialization ctor needs to have its own check that its between 0 and 255.

I feel like ideally the serialization should be done in the same format that is set by the end user but yeah, it would need two different checks if it is currently serializing as unsigned instead of signed.

Also, there has to be some mention of the whole overflow thing, cause a plugin could do setPower(-128) and then getPower returns what... 128? or -128?

I don't see how there would need to be any mention there? Setting -128 should obviously return -128. No plugin should expect any value above 127 or below 0 anyways as the API currently disallows that.