Tile NBT usage enhancements #1259

Muqsit · 2017-07-30T06:08:19Z

Introduction

Relevant issues

It looks like for every time PocketMine needs to change the value of a tile's NBT tag, it creates a totally new NBT tag and assigns it a value.

/** @var \pocketmine\tile\Furnace $furnace */

//onUpdate()...
if($furnace->namedtag["BurnTime"] > 0){
    $furnace->namedtag["BurnTime"] = new ShortTag("BurnTime", ((int) $furnace->namedtag["BurnTime"]) - 1);
}

Its pointless. You can change the tag's value using Tag::setValue($value).
It also seems pointless to (mixed) cast every nbt tag. This can be avoided by using Tag::getValue().

//Before:
if($furnace->namedtag["BurnTime"] > 0){
    $furnace->namedtag["BurnTime"] = new ShortTag("BurnTime", ((int) $furnace->namedtag["BurnTime"]) - 1);
}

//After:
if($furnace->namedtag->BurnTime->getValue() > 0){
    $furnace->namedtag->BurnTime->setValue($furnace->namedtag->BurnTime->getValue() - 1);
}

Changes

API changes

This pull request goes hand in hand with #1116. You can still assign an index to a CompoundTag array, but it's going to be useless.
If you are modifying a tile, please use the new way which again, is what's explained in #1116.

//Do NOT:
$tileNBT = new CompoundTag("", [
    "x" => new IntTag("x", 100)
]);
var_dump((int) $tileNBT["x"]);

//Do:
$tileNBT = new CompoundTag("", [
    new IntTag("x", 100)
]);
var_dump($tileNBT->x->getValue());

"------------------------------"

//Do NOT:
$tileNBT["x"] = new IntTag("x", ((int) $tileNBT["x"]) + 10);

//Do:
$tileNBT->x->setValue($tileNBT->x->getValue() + 10);

Spawnable::getSpawnCompound() is no longer an abstract function and returns generic tile NBT.

Added abstract function addAdditionalSpawnData(CompoundTag $nbt) which is where all the tile-specific nbt tags are applied.

Behavioural changes

This can be considered a 'micro-optimization' since we aren't creating multiple new NBT tags every tick as seen in case of Furnace.

Backwards compatibility

This PR is backwards compatible.

Tests

Tests done:
Chest
Place a chest. Put some items in it. Close the chest. Restart the server. Open the chest. Works just like it should.

Enchanting Table:
Place an enchanting table. Open it. Close it. Restart the server. Open it. Close it. Works.

Flower Pot:
Place a flower pot. Add a flower (tried with cactus). Restart the server. Works.

Item Frame:
Place an item frame. Rotate item 45° (1 tap). Restart the server. Works.

Sign:
Place a sign. Fill up all four lines. Restart the server. Works.
Place a sign. Fill up the 2nd and 4th line. Restart the server. Works.

Skull:
Place a skull with rotation % 90 !== 0. Restart the server. Works.
Place a skull with rotation % 90 === 0. Restart the server. Works.

API Changes made after initial changes

Spawnable::getSpawnCompound() is now a final function. It will return the common NBT tag for tiles...

final public function getSpawnCompound() : CompoundTag{
    return new CompoundTag("", [
        $this->namedtag->id,
        $this->namedtag->x,
        $this->namedtag->y,
        $this->namedtag->z
    ]);
}

Muqsit · 2017-07-30T07:45:38Z

src/pocketmine/tile/FlowerPot.php

@@ -66,12 +65,12 @@ public function canAddItem(Item $item) : bool{
 	}

 	public function getItem() : Item{
-		return Item::get((int) ($this->namedtag["item"] ?? 0), (int) ($this->namedtag["mData"] ?? 0), 1);
+		return Item::get($this->namedtag->item->getValue(), $this->namedtag->mData->getValue(), 1);


In case you were wondering why I did not check whether $this->namedtag->item or $this->namedtag->mData is set before using getValue(), see https://github.com/Muqsit/PocketMine-MP/blob/b7100070e2b2071a1b38579d337362e579dfbef9/src/pocketmine/tile/FlowerPot.php#L35-#L40

It's possible for a plugin to unset them though. Unlikely: yes, but still possible

@jasonwynn10 Even before this commit was done, it was possible to unset this value. But if you meant unsetting and then defining another tag type for the value then I think the plugin should be blamed for their incorrect usages.

Muqsit · 2017-07-30T07:50:39Z

src/pocketmine/tile/Chest.php

@@ -74,7 +74,7 @@ public function close(){
 	}

 	public function saveNBT(){
-		$this->namedtag->Items = new ListTag("Items", []);
+		$this->namedtag->Items->setValue([]);
 		$this->namedtag->Items->setTagType(NBT::TAG_Compound);


I'm guessing this line is unnecessary because https://github.com/Muqsit/PocketMine-MP/blob/b7100070e2b2071a1b38579d337362e579dfbef9/src/pocketmine/tile/Chest.php#L51

dktapps · 2017-07-30T07:57:36Z

Looks good at first glance, I'll review this properly later when I have the brainpower to do so.

dktapps · 2017-07-30T14:12:14Z

src/pocketmine/tile/Furnace.php

@@ -170,7 +169,7 @@ public function setItem(int $index, Item $item){
 			}
 			$this->namedtag->Items[$i] = $d;
 		}else{
-			$this->namedtag->Items[$i] = $d;
+			$this->namedtag->Items[$i]= $d;


dktapps · 2017-07-31T07:37:33Z

Since there is a lot of undefined behaviour around the NBT implementation (type checking and such) it is sometimes preferable to create a new NBT tag to ensure that the tag is of the expected type (plugins can cause problems, as seen in earlier comments).

Your alterations to the spawn compound handling are reasonable. A possible improvement might be to make getSpawnCompound() final and have a separate abstract method something like addAdditionalSpawnData() which would be called by getSpawnCompound(). That would prevent accidents with forgetting the parent call and would also ensure that future additions must follow the new rules - this PR, for example, does not clean up the Bed tile implementation I added since this PR was opened. If I hadn't remembered beds myself it would have gone unnoticed.

Muqsit · 2017-07-31T19:37:05Z

Nice, this also reduces some boilerplate code.

dktapps · 2017-08-02T09:57:35Z

Looks good to me, have you tested this fully?

Muqsit · 2017-08-02T11:09:25Z

@dktapps Yes, I've re-tested it after the bed tile commit.

dktapps · 2017-08-02T19:43:16Z

src/pocketmine/tile/Sign.php

@@ -89,29 +86,24 @@ public function getLine(int $index) : string{
 		if($index < 0 or $index > 3){
 			throw new \InvalidArgumentException("Index must be in the range 0-3!");
 		}
-		return (string) $this->namedtag["Text" . ($index + 1)];
+		return $this->namedtag{"Text" . ($index + 1)}->getValue();


Missed something?

The casting was unneeded because we're using getValue()

I'm not talking about the cast. You're missing a ->.

The cast wouldn't be needed regardless (it's there to shut PhpStorm up because it can't determine the type of the value gotten from the tag).

Muqsit · 2017-08-06T11:38:32Z

:D

Muqsit added 5 commits July 30, 2017 00:52

Do not create new NBT objects on Tile::getSpawnCompound()

3ad0658

PocketMine's string formatting

eabe99e

Remove more useless array indices and create lesser new NBT objects.

57a1b5c

Remove unused imports and type-hint Sign::setText() params

81e7ece

Do not mess with Sign::setText() params due to #1204

b710007

Muqsit commented Jul 30, 2017

View reviewed changes

dktapps added Category: Core Related to internal functionality Category: API Related to the plugin API PR: Contribution labels Jul 30, 2017

dktapps reviewed Jul 30, 2017

View reviewed changes

Fix formatting

96f1b92

Muqsit added 4 commits July 31, 2017 15:10

Make getSpawnCompound() final and add abstract addAdditionalSpawnData()

599cc85

Merge branch 'master' of github.com:Muqsit/PocketMine-MP

a815f9e

Merge remote-tracking branch 'upstream/master'

5f59aff

Make the same changes for Bed tile

26823bb

dktapps reviewed Aug 2, 2017

View reviewed changes

Fix a missing "->" and remove some unneeded int casting.

9986b75

dktapps force-pushed the master branch 2 times, most recently from bc6a8c7 to fdf7184 Compare August 4, 2017 12:20

dktapps merged commit 7d3fca8 into pmmp:master Aug 6, 2017

dktapps added Type: Enhancement Contributes features or other improvements to PocketMine-MP and removed Type: Contribution labels Nov 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tile NBT usage enhancements #1259

Tile NBT usage enhancements #1259

Muqsit commented Jul 30, 2017 •

edited

Muqsit Jul 30, 2017 •

edited

jasonw4331 Jul 30, 2017

Muqsit Aug 2, 2017 •

edited

Muqsit Jul 30, 2017

dktapps commented Jul 30, 2017

dktapps Jul 30, 2017

dktapps commented Jul 31, 2017

Muqsit commented Jul 31, 2017

dktapps commented Aug 2, 2017

Muqsit commented Aug 2, 2017 •

edited

dktapps Aug 2, 2017

Muqsit Aug 2, 2017 •

edited

dktapps Aug 2, 2017

Muqsit commented Aug 6, 2017

Tile NBT usage enhancements #1259

Tile NBT usage enhancements #1259

Conversation

Muqsit commented Jul 30, 2017 • edited

Introduction

Relevant issues

Changes

API changes

Behavioural changes

Backwards compatibility

Tests

API Changes made after initial changes

Muqsit Jul 30, 2017 • edited

Choose a reason for hiding this comment

jasonw4331 Jul 30, 2017

Choose a reason for hiding this comment

Muqsit Aug 2, 2017 • edited

Choose a reason for hiding this comment

Muqsit Jul 30, 2017

Choose a reason for hiding this comment

dktapps commented Jul 30, 2017

dktapps Jul 30, 2017

Choose a reason for hiding this comment

dktapps commented Jul 31, 2017

Muqsit commented Jul 31, 2017

dktapps commented Aug 2, 2017

Muqsit commented Aug 2, 2017 • edited

dktapps Aug 2, 2017

Choose a reason for hiding this comment

Muqsit Aug 2, 2017 • edited

Choose a reason for hiding this comment

dktapps Aug 2, 2017

Choose a reason for hiding this comment

Muqsit commented Aug 6, 2017

Muqsit commented Jul 30, 2017 •

edited

Muqsit Jul 30, 2017 •

edited

Muqsit Aug 2, 2017 •

edited

Muqsit commented Aug 2, 2017 •

edited

Muqsit Aug 2, 2017 •

edited