Skip to content

Conversation

@IvanCraft623
Copy link
Member

Introduction

The CustomNameAlwaysVisible tag is not being read/saved in the entities, causing data loss.

Relevant issues

Tests

$human = new Human($location, $skin);
$human->setNameTag("mctestDylan");
$human->setNameTagAlwaysVisible();

jasonw4331
jasonw4331 previously approved these changes Sep 20, 2022
@jasonw4331 jasonw4331 added Category: API Related to the plugin API Type: Contribution Type: Fix Bug fix, typo fix, or any other fix labels Sep 20, 2022
@ColinHDev
Copy link
Contributor

Do we follow Mojang's entity save format within PM? Just asking because a save key with a comparable functionality does not exist according to the wiki.

@dktapps
Copy link
Member

dktapps commented Sep 28, 2022

Do we follow Mojang's entity save format within PM? Just asking because a save key with a comparable functionality does not exist according to the wiki.

That's precisely why I hesitated to merge it.

@ShockedPlot7560
Copy link
Member

I don't think this should be an obstacle to merge the PR. Could it have any impact on PM at some point?

Joshy3282 added a commit to Joshy3282/PocketMine-MP that referenced this pull request Oct 2, 2024
@dktapps
Copy link
Member

dktapps commented Nov 14, 2024

I don't think this should be an obstacle to merge the PR. Could it have any impact on PM at some point?

As long as we're sure Mojang won't introduce a tag with the same name and a different value type...

@dktapps dktapps added the Opinions Wanted Request for comments & opinions from the community label Nov 14, 2024
@dktapps
Copy link
Member

dktapps commented Nov 23, 2024

I think we need to think about standard ways to store PM-specific data before merging this, so this doesn't become a problem in the future. (Really should've done this years ago before plugin devs were able to start injecting all kinds of crap into the NBT...)

@dktapps dktapps added the Status: Blocked Depends on other changes which are yet to be completed label Nov 23, 2024
@dktapps dktapps requested a review from a team as a code owner December 12, 2024 13:22
@ipad54
Copy link
Member

ipad54 commented Sep 27, 2025

I think we need to think about standard ways to store PM-specific data before merging this, so this doesn't become a problem in the future.

The first thing that comes to mind is to have a PMMPExtraData Compound tag that will be written into an entity NBT and also loadExtraData(CompoundTag $nbt) : void, saveExtraData() : CompoundTag methods. Any custom entity properties will be stored inside this tag. What do you think?

@dktapps
Copy link
Member

dktapps commented Sep 27, 2025

@ipad54 I agree with putting PM custom data inside a subtag. Not sure if it's necessary to add extra methods for it though since it won't be needed in most cases.

@ipad54
Copy link
Member

ipad54 commented Sep 27, 2025

Extra methods can be pretty useful in the future if we want to store more than one custom property, especially in different entity classes.

@dktapps
Copy link
Member

dktapps commented Sep 27, 2025

I feel like it's redundant for one property. We can add more stuff later on if necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Category: API Related to the plugin API Opinions Wanted Request for comments & opinions from the community Status: Blocked Depends on other changes which are yet to be completed Type: Fix Bug fix, typo fix, or any other fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Nametag visibility is not persisted

6 participants