Paper icon indicating copy to clipboard operation
Paper copied to clipboard

BlockProperty API

Open Machine-Maker opened this issue 3 years ago • 12 comments

My take on how to add a "get block data properties individually" on top of bukkit's existing method-based system.

Structure

3 types of data properties: Boolean, Integer, and Enum. This pr creates a base BlockProperty class with 3 (+ special cases) implementations of it for each type.

Special Cases

So far, I've noticed 2 API types that are used to wrap integers, so this also adds handling for those, not exposing them as integers.

  • rotation as BlockFace
  • note as Note

The BlockPropertyHolder interface contains all the methods to interact with a set of properties. I left it as a standalone interface in case other uses in the future popup where a separate impl might be warranted. I added a Mutable subinterface to that to separate out the "set" methods, some implementations of it might not want to support mutating properties (such as future FluidState API https://github.com/PaperMC/Paper/pull/8609).

Tests

I wrote some significant tests to ensure all nms Propertys from BlockStateProperties have a counterpart, and they are equal to each other.

Machine-Maker avatar Dec 30 '21 05:12 Machine-Maker

I do not like the name DataProperty. What is wrong with just Property? These are not limited to just blocks, as the game also uses them for fluids (and potentially more in the future?).

kashike avatar Dec 30 '21 08:12 kashike

I do not like the name DataProperty. What is wrong with just Property?

I guess because there is already org.bukkit.inventory.InventoryView.Property enum in InventoryView class, to do not confuse other's about these 2 Property class or smth.

montlikadani avatar Dec 30 '21 08:12 montlikadani

That's nice, but not a reasonable reason.

kashike avatar Dec 30 '21 08:12 kashike

I do not like the name DataProperty. What is wrong with just Property?

I guess because there is already org.bukkit.inventory.InventoryView.Property enum in InventoryView class, to do not confuse other's about these 2 Property class or smth.

No reason as the Damagable exists two times as well. For ItemMeta and the Entities.

I'm happy that such API get introduced into Paper-API. First steps to be able to handle custom block data in future.

yannicklamprecht avatar Dec 30 '21 11:12 yannicklamprecht

I do not like the name DataProperty. What is wrong with just Property? These are not limited to just blocks, as the game also uses them for fluids (and potentially more in the future?).

What does the name “DataProperty” have to do with limiting it to blocks? Or limiting its expansion to other things. Nms calls them Property, and creates them all in a class called BlockStateProperties. They are properties for block/fluid states. And the closest Bukkit representation of that, is BlockData, which is used for both blocks and fluids. I think DataProperty makes sense in that context.

On a much smaller improvement scale, it’s also different from the nms name for it, which makes imports just nicer.

Machine-Maker avatar Dec 30 '21 16:12 Machine-Maker

Ok, renamed to Property and added better handling for int properties that have non-int representations in the api So far those special cases are

  • BlockFace ("rotation")
  • Note ("note")

Machine-Maker avatar Jan 22 '22 06:01 Machine-Maker

Rebased for 1.18.2

Machine-Maker avatar Mar 29 '22 19:03 Machine-Maker

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar May 30 '22 21:05 stale[bot]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Jul 30 '22 14:07 stale[bot]

Rebased for 1.19.1

Machine-Maker avatar Aug 04 '22 06:08 Machine-Maker

As specified before, we probably want to rename this to BlockProperty or something.

I disagree, that would limit its use for other things in the future.

kashike avatar Jan 15 '23 21:01 kashike

As specified before, we probably want to rename this to BlockProperty or something.

I disagree, that would limit its use for other things in the future.

Then we may need to see about aligning this property API in a way that makes it work with the item property api too then? Because at least, it was discussed for them to be separate concepts, and if we are wanting to inline them there's gonna need more talk with that.

Owen1212055 avatar Jan 15 '23 21:01 Owen1212055