Skip to content

Conversation

Machine-Maker
Copy link
Member

@Machine-Maker Machine-Maker commented Dec 30, 2021

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 #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 Machine-Maker requested a review from a team as a code owner December 30, 2021 05:12
@kashike
Copy link
Member

kashike commented Dec 30, 2021

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?).

@montlikadani
Copy link
Contributor

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.

@kashike
Copy link
Member

kashike commented Dec 30, 2021

That's nice, but not a reasonable reason.

@yannicklamprecht
Copy link
Contributor

yannicklamprecht commented Dec 30, 2021

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.

@Machine-Maker
Copy link
Member Author

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 Machine-Maker force-pushed the feature/block-data-properties branch 2 times, most recently from a215c41 to 648e895 Compare January 2, 2022 05:57
@kashike kashike mentioned this pull request Jan 2, 2022
@Machine-Maker Machine-Maker force-pushed the feature/block-data-properties branch from 648e895 to d43953b Compare January 22, 2022 06:42
@Machine-Maker Machine-Maker changed the title DataProperty API Property API Jan 22, 2022
@Machine-Maker
Copy link
Member Author

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 Machine-Maker force-pushed the feature/block-data-properties branch 3 times, most recently from 5b4edb1 to 7be3d7c Compare January 22, 2022 07:46
@Machine-Maker Machine-Maker force-pushed the feature/block-data-properties branch from 7be3d7c to 1e2fc83 Compare February 2, 2022 19:57
@Machine-Maker Machine-Maker force-pushed the feature/block-data-properties branch from 1e2fc83 to 17c5174 Compare March 29, 2022 19:13
@Machine-Maker
Copy link
Member Author

Rebased for 1.18.2

@stale
Copy link

stale bot commented May 30, 2022

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
Copy link

stale bot commented Jul 30, 2022

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.

@Machine-Maker Machine-Maker force-pushed the feature/block-data-properties branch from 17c5174 to af3a1de Compare August 4, 2022 06:50
@Machine-Maker
Copy link
Member Author

Rebased for 1.19.1

Copy link
Member

@electronicboy electronicboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking over this, this looks fine

@Machine-Maker Machine-Maker force-pushed the feature/block-data-properties branch from af3a1de to b4294ee Compare December 7, 2022 00:59
@Machine-Maker Machine-Maker force-pushed the feature/block-data-properties branch from b4294ee to 69ee884 Compare December 22, 2022 05:40
Copy link
Member

@Owen1212055 Owen1212055 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

The propertyholder/etc classes however I'm not sure about, because that may be useful API to have shared with items/blocks/etc. Unsure tho. The item property PR however creates a very similar property holder class and such.

I'd also maybe mark the block properties for stuff like chiseled bookshelf as experimental.
Looks good tho, really nice powerful api! 😄

@kashike
Copy link
Member

kashike commented Jan 15, 2023

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.

@Owen1212055
Copy link
Member

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.

@Machine-Maker Machine-Maker changed the title Property API BlockProperty API Mar 6, 2023
@Machine-Maker Machine-Maker force-pushed the feature/block-data-properties branch from 69ee884 to 5c836b3 Compare March 6, 2023 05:41
@Machine-Maker Machine-Maker force-pushed the feature/block-data-properties branch from 5c836b3 to 567ca84 Compare August 20, 2023 22:29
@Machine-Maker Machine-Maker force-pushed the feature/block-data-properties branch from 567ca84 to 4e88f48 Compare March 10, 2024 00:29
@Machine-Maker Machine-Maker force-pushed the feature/block-data-properties branch from 4e88f48 to dba19bd Compare May 29, 2024 04:40
@Machine-Maker Machine-Maker force-pushed the feature/block-data-properties branch 2 times, most recently from 75bf092 to c2a3487 Compare November 24, 2024 06:51
@Machine-Maker Machine-Maker force-pushed the feature/block-data-properties branch from c2a3487 to 22ff9c4 Compare November 24, 2024 07:07
@Machine-Maker Machine-Maker deleted the feature/block-data-properties branch January 12, 2025 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Closed

Development

Successfully merging this pull request may close these issues.

8 participants