Incompatibility with PocketMine's default mobs

[Muqsit]

When you try spawning a PocketMine entity that shares the same savename as a pet, the following error gets thrown:

[21:53:58] [Server thread/CRITICAL]: RuntimeException: "Tag with name "petOwner" not found and no valid default value given" (EXCEPTION) in "vendor/pocketmine/nbt/src/tag/CompoundTag" at line 176

You can reproduce this error by giving yourself a zombie spawn egg (/give yourself spawn_egg:32) and try spawning the zombie.

[CuongVnZ]

When this will be fixed?

[ericsimard52]

I have done some digging around, it is a conflict with PureEntitiesX. The issue does not occur if there is no spawning happening. I think it is a change in NBT code that triggers. I have traced the issue to the construct of BasePet.php: $this->petOwner = $level->getServer()->getPlayerExact($nbt->getString("petOwner")); Judging by the following code, I assume that $nbt->getString use to return null if the tag did not exists. Now it throws an error. The original construct looked like this: ` public function construct(Level $level, CompoundTag $nbt) { $this->petOwner = $level->getServer()->getPlayerExact($nbt->getString("petOwner")); if($this->petOwner === null) { $this->close(); return; } parent::construct($level, $nbt);

} ` The crashdump is original.log original.log

NBT now takes a default value that seems to be returning when the tag does not exists. Sometime I notice it is returning "" I change the BasePet construct to this: ` public function construct(Level $level, CompoundTag $nbt) { $this->petOwner = $level->getServer()->getPlayerExact($nbt->getString("petOwner", "NA")); if ( ($this->petOwner == "NA") || ($this->petOwner == "") ){ $this->close(); return; } parent::construct($level, $nbt);

} Which brought me to this error: [Server thread/CRITICAL]: Error: "Call to a member function getPluginManager() on null" (EXCEPTION) in "BlockPets_dev-47.phar/src/BlockHorizons/BlockPets/pets/BasePet" at line 134 [15:56:10] [Server thread/DEBUG]: #0 BlockPets_dev-47.phar/src/BlockHorizons/BlockPets/pets/BasePet(789): BlockHorizons\BlockPets\pets\BasePet->getLoader() ` Crashdump: Tue_Jan_1-15.56.10-CST_2019.log

I have attempted to hack around but it just keeps bouncing the error around. I do not quite understand the relation that is happening between BlockPet and PureEntitiesX and I am unsure as to where I should look from here. Any pointers?

[Muqsit]

@ericsimard52 While a fallback value to petOwner tag would solve this issue, a pet with an incomplete pet NBT shouldn't be created. A fallback value to petOwner would still keep creating and instantly close()ing the pet entity but no one would be able to see this happening in-game because the pet supposedly gets closed right as it spawns. The log you attached exposes another issue arising from this issue. When the petOwner tag isn't present, the pet skips calling the parent constructor, so BasePet::$server isn't defined and vomits out an error in BasePet::close() which requires BasePet::$server to be a Server instance.

There should be a method to register entities without registering their network id as the savename.

The error in PureEntities replicates the same error when spawning entities through spawn eggs: `

4 PureEntitiesX.phar/src/revivalpmmp/pureentities/PureEntities(265): pocketmine\entity\Entity::createEntity(integer 10, pocketmine\level\Level object, pocketmine\nbt\tag\CompoundTag object)

`

PureEntities gets enabled PureEntities registers savenames "10" and "Chicken" to point to PureEntities' Chicken class BlockPets gets enabled BlockPets registers savenames "10" and "ChickenPet" to point to BlockPets' ChickenPet class

Result? BlockPets overrides PureEntities' savename "10"

PureEntities could use the non-networkid savename (i.e Entity::createEntity("Chicken", ... as opposed to Entity::createEntity(10, ...) to solve this issue, but then this also affects pocketmine's spawn egg behaviour as well.

Edit: This issue can be solved by setting all pet entities' NETWORK_ID to -1 and then overriding the BasePet::sendSpawnPacket() method to handle spawning the pet with a custom network id constant instead. https://github.com/pmmp/PocketMine-MP/blob/ea9f9aa2503a2044fa2572f6def77fd642f6de06/src/pocketmine/entity/Entity.php#L335

[ericsimard52]

Thank you. I have enough pointer to fix this. For those waiting on a fix please have a little patience, this is my first attempt at this. I don't even know the general mechanics of PocketMine-MP and plugins, but with the info provided I will be able to fix this.

[ericsimard52]

I have made progress, I can now spawn a pet and an creature from an egg of the same type. I practice with horse, pig and cow. I have no crash happening. I want to explain my mods here first. I may not have done it the best or right way, looking for feedback and comments: For my testing, I commented out in Loader.php all but my 3 test class. This way blockpet only registered 3 pets. I will use the cow as an example here. Each of those pets file in creature got a modification like the one for cow: const NETWORK_ID = -1; const NETWORK_NAME = "COWPET"; const NETWORK_ORIG = self::COW;

Next, off to Loader.php in registerEntities: public function registerEntities(): void { foreach(self::PET_CLASSES as $petClass) { Entity::registerEntity($petClass, true,array($petClass::NETWORK_ID,$petClass::NETWORK_NAME)); } } If I understand properly, this registers the pet with a network id of -1 and also register the network name which is unique to this pet creature. I think that properly register a pet that is separated from the same creature as a normal creature.

Next off to BasePet.php. The constructor now looks like: public function __construct(Level $level, CompoundTag $nbt) { $this->petOwner = $level->getServer()->getPlayerExact($nbt->getString("petOwner","NA")); parent::__construct($level, $nbt); }

sendSpawnPackets now looks like: protected function sendSpawnPacket(Player $player): void { $pk = new AddEntityPacket(); $pk->entityRuntimeId = $this->getId(); if(static::NETWORK_ID == "-1"){ $pk->type = static::NETWORK_ORIG; //Set type to original creature self::.... } else{ $pk->type = static::NETWORK_ID; } $pk->position = $this->asVector3(); $pk->motion = $this->getMotion(); $pk->yaw = $this->yaw; $pk->pitch = $this->pitch; $pk->attributes = $this->attributeMap->getAll(); $pk->metadata = $this->propertyManager->getAll(); $pk->links = array_values($this->links); $player->dataPacket($pk); }

Without the above modification, i get and error right away when I join: ErrorException: "Undefined offset: -1" (EXCEPTION) in "src/pocketmine/network/mcpe/protocol/AddEntityPacket" at line 210 If I understand properly, this kind of spoof the NETWORK_ID of our pet with the original one, and allows it to spawn.

After, we go to initEntity, still in BasePets.php. Here addition between: parent::initEntity(); and $this->selectProperties(); The rest of the code did not change: protected function initEntity(): void { parent::initEntity(); if ( static::NETWORK_ID != -1 ){ return; } $this->selectProperties(); So once again if I got this right, this makes sure that after parent init the creature, if it is not a pet, we do nothing else.

I have done testing with spawning pet and creature of the same type, I got no crash... I can still ride them, horse does not move when being ridden, but that issue was there before. When I ride, I get a lot of Unhandled PlayerInputPacket received from Papa: 0x3900000000000000000100 Unsure about what that is.

What do you think of the fix?

[ericsimard52]

Maybe talked a little too fast. I sometime have this error reported after crashing. Seems to happen when I fly too far away it seems like it is unable to teleport the pets. [Server thread/CRITICAL]: Error: "Call to a member function broadcastPacketToViewers() on null" (EXCEPTION) in "src/pocketmine/entity/Entity" at line 1189 Fri_Jan_4-20.56.03-CST_2019.log Not really asking any question on this yet, just keeping you guys in the loop.

Before this, I had a crash when feeding my pets, fixed it quick, just needed to cast a float to a int before calling the function addPetsLevelPoint.

[ericsimard52]

Confirmed, happen when attempting to tp a pet. in Function: protected function broadcastMovement(bool $teleport = false): void { if($this->isRiding()) { return; } parent::broadcastMovement($teleport); } Only happens when $teleport is true. Would that be related to the current issue or completely separate?

[Muqsit]

@ericsimard52 Looks great. There could be a few improvements made though. For example, since every class extends BasePet, you could have const NETWORK_ID = -1 set in BasePet only, the other classes extending BasePet will already inherit it, so you can remove the redeclared NETWORK_ID constant from extending classes.

class A{
    const KONST = -1;
}
class B extends A{}
var_dump(B::KONST); //-1

While at it, I think the BasePet constructor can be declared final and can be used to handle the constant values.

class BasePet ... {
    const NETWORK_ID = -1;
    const NETWORK_NAME = null;
    const NETWORK_ORIG = null;

    final public function __construct(Level $level, CompoundTag $nbt) {
        if(static::NETWORK_ID === -1) {
            throw new \LogicException("Network IDs of pets cannot be overridden.");
        }
        if(static::NETWORK_NAME === null) {
            throw new \LogicException("NETWORK_NAME constant in " . get_class($this) . " must be defined.");
        }
        if(static::NETWORK_ORIG === null) {
            throw new \LogicException("NETWORK_NAME constant in " . get_class($this) . " must be defined.");
        }

        //...
    }
}

If these checks exist in the constructor, there won't be code executing any further than the constructor, so no extra NETWORK_ID !== -1 (or vice versa) would be required in other class methods like initEntity() or sendSpawnPacket() since that'll make it impossible for a pet with NETWORK_ID !== -1 to exist.

BTW, does it work without the fallback value ("NA") assigned to petOwner tag in the constructor? If the tag isn't present and we provide it a fallback value, it might cause other runtime errors while trying to save/retrieve pet's data from/to the database, so I believe it's better to have an exception be thrown if the tag doesn't exist.

[Muqsit]

The Unhandled PlayerInputPacket... is sent as a debug message and is sent when pocketmine doesn't handle the received packet from the player. Well, pocketmine doesn't have a PlayerInputPacket handler at all, so I believe that message would be printed every time the client sends that packet to the server, unless a plugin cancels it during DataPacketReceiveEvent (I could be wrong, though).

[Muqsit]

The level on null error could possibly be due to the registerEntity. Try:

Entity::registerEntity($petClass, true, [$petClass::NETWORK_NAME]);

[ericsimard52]

Sweet, I'm really having fun doing this and I have 2 boys that enjoy testing!

[ericsimard52]

Working good. The level on null error is a different issue all together. I have ManyWorld plugins. It only happens when I switch world. I have removed the plugin and tested on a single world server and the issue did not happen. I will open another issue for that.

I have update .poggit.yml dependency list and config.yml API version. I think I'm ready to create a pull request.

[ericsimard52]

Pull request done. I am new to contributing to project, if I can do things better let me know. If there are other steps to be done to have this update on poggit list them and I am willing to do the work.

[DenielWorld]

Why isn't the pull request accepted yet?