PrismarineJS / prismarine-block

Represent a minecraft block with its associated data
29 stars 31 forks source link

Support for multi-sided signs #87

Closed PondWader closed 1 year ago

PondWader commented 1 year ago

For 1.20. This is a breaking change for when moving from 1.19 to 1.20

extremeheat commented 1 year ago

If it were a breaking change here, it would also propagate in mineflayer 5.0.

Usually the way changes like these are handled is just adding a new parameter or a new function to handle the new functionality, but since we have getters/setters yeah it's not future proof. Instead to preserve existing API, you can deprecate block.signText = '' and replace it with a block.setSignText(front, back) and block.getSignText(): [string, string?] on all versions.

PondWader commented 1 year ago

If it were a breaking change here, it would also propagate in mineflayer 5.0.

Usually the way changes like these are handled is just adding a new parameter or a new function to handle the new functionality, but since we have getters/setters yeah it's not future proof. Instead to preserve existing API, you can deprecate block.signText = '' and replace it with a block.setSignText(front, back) and block.getSignText(): [string, string?] on all versions.

As well as moving to functions, I could make the previous, to-be deprecated, APIs return data for the front of the sign which would keep it backwards compatible even when changing version to 1.20.

PondWader commented 1 year ago

Just doing this would avoid breaking anything when bots move to 1.20

PondWader commented 1 year ago

There, I've done it for bedrock too. I am a bit unsure if it's a good idea to remove blockEntity as it's even mentioned in the mineflayer docs, but if it does break a few bots I guess it doesn't matter much as it should be pretty easy for those bots to fix it

extremeheat commented 1 year ago

To be clear, the .blockEntity field will still exist. It will just not be overridden in the sign handling.

rom1504 commented 1 year ago

Overall lgtm, left a few comments to resolve

extremeheat commented 1 year ago

Also, you're not testing 1.20.0 in the tests, you can add that to make sure this is working on 1.20

PondWader commented 1 year ago

For some reason when sign text is set for legacy signs, the original code pretty much ends up ignoring new lines. So setSignText('a\nb') would end up effectively being the same as setSignText('ab'), to me this feels like wrong behaviour but is this intentional?

image

rom1504 commented 1 year ago

Ok looks good, merging thanks