Implement Villager v2 and trade functionality #6310

alvin0319 · 2024-03-26T05:52:00Z

Introduction

This PR implements Villager V2 and trade functionality.

Relevant issues

Implementation of trade #2166

Changes

API changes

Added VillagerV2:
- Villager V2 has 15 variants: Unemployed, Farmer, Fisherman, Shepherd, Fletcher, Librarian, Cartographer, Cleric, Armorer, Weaponsmith, Toolsmith, Butcher, Leatherworker, Mason (aka Stone Mason), Nitwit
- Unemployed and Nitwit villager can't trade, so they don't have recipes.
- Villagers now have their recipes corresponding their professions (except unimplemented items), following this wiki.
- VillagerV2::setProfession(VillagerProfession): Sets a profession of this villager, also updates recipes.
- VillagerV2::getProfession() : VillagerProfession: Returns the current profession of the villager.
- VillagerV2::getRecipeData() : RecipeData: Returns the recipe data of this villager.
Added VillagerProfession and its related friends:
- Each professions have their corresponding translation key (for their name), variant id and job block.
Added TradeRecipeData:
- TradeRecipeData contains necessary values for trade: recipes, current tier, trader experience, tier experience requirements
Added TradeRecipe:
- buyA: Item that trader wants.
- sell: Item that trader gives to you.
- buyB: Item that trader additionally wants, optional.
- maxUses: Max uses of this trade. 16 is default.
- priceMultiplier: affects price of item, not used currently. 0.05 is default.
- rewardExp: Reward exp that player earns by completing this trade. 0 is default.
- tier: Required tier for this trade. 0 is default.
- traderExp: Reward exp for the villager. 0 is default.
- uses: Amount of current uses of this recipe. 0 is default.

Behavioural changes

Villager Spawn Egg will now spawn Villager V2 instead of legacy Villager.

Backwards compatibility

There is no known BC breaking change.

Follow-up

N/A

Tests

I tested this PR by doing the following (tick all that apply):

Writing PHPUnit tests (commit these in the tests/phpunit folder)
Playtesting using a Minecraft client (provide screenshots or a video)
Writing a test plugin (provide the code and sample output)
Other (provide details)

Minecraft.2024-03-26.14-47-44.mp4

Plugin used for this test:

<?php

/**
 * @name TradeTest
 * @author alvin0319
 * @main alvin0319\TradeTest\TradeTest
 * @version 1.0.0
 * @api 5.0.0
 */

declare(strict_types=1);

namespace alvin0319\TradeTest;

use pocketmine\entity\Location;
use pocketmine\entity\trade\TradeRecipe;
use pocketmine\entity\trade\TradeRecipeData;
use pocketmine\entity\Villager;
use pocketmine\event\EventPriority;
use pocketmine\event\player\PlayerJoinEvent;
use pocketmine\inventory\TradeInventory;
use pocketmine\item\VanillaItems;
use pocketmine\player\Player;
use pocketmine\plugin\PluginBase;

final class TradeTest extends PluginBase{

	protected function onEnable() : void{
		$this->getServer()->getPluginManager()->registerEvent(PlayerJoinEvent::class, fn(PlayerJoinEvent $event) => $this->sendTrade($event->getPlayer()), EventPriority::NORMAL, $this);
	}

	/**
	 * This trade UI implements some offers of Fisherman in Bedrock Edition
	 * @see https://minecraft.wiki/w/Trading#Fisherman_2
	 */
	private function sendTrade(Player $player) : void{
		// spawn villager entity 1.5 block above the player
		$villager = new Villager(
			Location::fromObject(
				$player->getDirectionVector()->multiply(1.5)->addVector($player->getPosition()),
				$player->getWorld()
			)
		);
		$villager->spawnToAll();
		$inventory = new TradeInventory("Trade Test", $villager, new TradeRecipeData([
			new TradeRecipe(
				buyA: VanillaItems::STRING()->setCount(20),
				sell: VanillaItems::EMERALD()->setCount(1),
				maxUses: 16,
				tier: 0,
				traderExp: 2
			),
			new TradeRecipe(
				buyA: VanillaItems::EMERALD()->setCount(3),
				sell: VanillaItems::COOKED_FISH()->setCount(1),
				buyB: VanillaItems::RAW_FISH()->setCount(6),
				maxUses: 16,
				tier: 0,
				traderExp: 1
			),
			new TradeRecipe(
				buyA: VanillaItems::RAW_FISH()->setCount(15),
				sell: VanillaItems::EMERALD()->setCount(1),
				maxUses: 16,
				tier: 1,
				traderExp: 10
			),
			new TradeRecipe(
				buyA: VanillaItems::EMERALD()->setCount(1),
				sell: VanillaItems::COOKED_SALMON()->setCount(6),
				buyB: VanillaItems::RAW_SALMON()->setCount(6),
				maxUses: 16,
				tier: 1,
				traderExp: 5
			),
		], 1));
		$player->setCurrentWindow($inventory);
	}
}

ShockedPlot7560 · 2024-03-26T08:01:34Z

Do we really want to force an API to everyone ? I can only think about the Form API. Wouldn't it be the function of a virion to do so?

Matze997 · 2024-03-26T08:34:46Z

On the other hand people will know as long as pocketmine gets updated this feature will be supported

alvin0319 · 2024-03-26T09:27:05Z

Do we really want to force an API to everyone ? I can only think about the Form API. Wouldn't it be the function of a virion to do so?

I don't think it is possible to implement this with virion so.

alvin0319 · 2024-04-01T14:40:45Z

Now ready to be reviewed.

IvanCraft623

A quick review, I still have to validate with BDS behavior

src/entity/VillagerV2.php

IvanCraft623 · 2024-04-02T02:03:46Z

src/entity/profession/ArmorerProfession.php

+final class ArmorerProfession extends VillagerProfession{
+
+	public function __construct(){
+		parent::__construct(VillagerProfessionTypeIds::ARMORER, "entity.villager.armor", VanillaBlocks::BLAST_FURNACE());


This is a translation key, make a PR in https://github.com/pmmp/Language to later use a constant

This is a client-side translation key, so I don't think PM-side translations are needed?

Avoid hardcode translations, this is not the only place where translation is only used client-side, e.g. names of enchantments.

IvanCraft623 · 2024-04-02T02:09:22Z

src/entity/profession/ArmorerProfession.php

+	}
+
+	/** @phpstan-return list<TradeRecipe> */
+	public function getRecipes(int $biomeId) : array{


In vanilla only a few recipes are selected per group, see https://github.com/Mojang/bedrock-samples/blob/main/behavior_pack/trading/armorer_trades.json, for more details. Probably with a function that generates recipes is not enough and will have to be extended to a dedicated class.

So as far as I understand, (also according to bedrock.dev wiki), recipes are separated something called "groups" and they are randomly selected whenever trade recipes are generated, right? Perhaps we could make a script that generates recipe class from behaviour pack.

Yes, it would be best if this is self-generated. I think that for now we only need to make an api to implement trade tables with their groups and leave the generation of recipes for another PR.

src/entity/trade/TradeRecipe.php

IvanCraft623 · 2024-04-02T02:18:32Z

src/entity/trade/TradeRecipeData.php

+
+namespace pocketmine\entity\trade;
+
+final class TradeRecipeData{


I don't like the name of the class, it gives me to understand that it contains the data of a single recipe, or that it's for data processing, maybe something like TradeRecipeManager, TradeRecipeGroup, or TradeRecipesHolder would be better.

I'm still not sure about this class name. TradeRecipeManager feels like it's too generic (which people might understand it manages all trade recipes), TradeRecipeGroup doesn't seem valid since it doesn't only contain trade recipes. TradeRecipesHolder might be good but I think we still need better name than these. (or separate other values like tier, exp to other class?)

src/event/player/PlayerItemTradeEvent.php

IvanCraft623 · 2024-04-02T02:25:14Z

src/inventory/EntityInventory.php

+use pocketmine\entity\Living;
+use pocketmine\network\mcpe\protocol\ClientboundPacket;
+
+abstract class EntityInventory extends SimpleInventory{


What other uses would this class have?

I'm not sure if there are any other use cases (including future), as far as I know trade is the only one which doesn't use ContainerOpen for opening inventory.

IvanCraft623 · 2024-04-02T03:35:20Z

The pull request is very bloated by the recipes generation, so perhaps it would be nice to leave them for a separate PR

ShockedPlot7560 · 2024-04-03T19:50:05Z

At the moment I'm still dubious about the fact that the API has been implemented right down to the NPC itself.

Is this a mission of pocketmine itself, or in the same way as the forms, we should let the community create libraries instead of imposing one.

alvin0319 · 2024-04-03T23:04:52Z

It isn't same with forms. Trade needs a lot of reflection hacks and closure hacks to work on plugins.

pandaa-be · 2024-04-04T06:14:06Z

At the moment I'm still dubious about the fact that the API has been implemented right down to the NPC itself.

Is this a mission of pocketmine itself, or in the same way as the forms, we should let the community create libraries instead of imposing one.

by the way, villager trading is not only an api but also implements the normal trades now, unlike before.

i find it nice that the pr also offer an api to be able to make trades, if there were several different plugins the support on the discord server would be annoying just like with scoreboard, bossbar and forms apis - it's annoying if someone uses jojo's api but you don't know the api yourself it would be better if there was one in the core that everyone uses

alvin0319 added 19 commits March 26, 2024 01:43

Initial work on implementing villager trade

5b9ca16

Fix misleading TODO comment

3f6c782

InventoryManager: Fixed phpstan, add some docs

a19845c

Merge branch 'minor-next' into feat/villager-trade

3c1d89f

Revert unrelated changes

2193493

CS

f5b3005

Here we go

a833b86

Make trade transaction consistent with existing transactions

4b98bed

TradingTransaction: Validate if recipe has been disabled

e978f78

...

7f0aa94

Allow accepting recipe data as a constructor parameter

f8fbdf1

rename variable name

d46ee5f

Validate tier on recipe construction

f17c37a

Revert Villager code

aabf56d

Revert unrelated changes on InventoryManager

8588ff8

Here we go

c79e6ba

Allow setting trade name

f407a9c

Fix validating input items

41fc528

Move tierExpRequirements after tier, set default values

ff46449

alvin0319 added 4 commits April 1, 2024 22:39

Implement villager v2

d1a92ef

CS

b3840be

Copilot mess & CS

f4c4b56

Finalize FishermanProfession

9425f9d

alvin0319 changed the title ~~Implement villager trade API~~ Implement Villager v2 and trade functionality Apr 1, 2024

alvin0319 marked this pull request as ready for review April 1, 2024 14:40

IvanCraft623 added the Category: API Related to the plugin API label Apr 1, 2024

IvanCraft623 added Category: Gameplay Related to Minecraft gameplay experience Category: Core Related to internal functionality Requires translations Type: Enhancement Contributes features or other improvements to PocketMine-MP labels Apr 1, 2024

IvanCraft623 reviewed Apr 2, 2024

View reviewed changes

alvin0319 added 2 commits April 2, 2024 12:29

VillagerV2: Account for babies

d0aec76

VillagerV2: Add line separator for better understanding

fe45da9

alvin0319 added 5 commits April 2, 2024 12:39

Change random profession selection mechanism

073f7b5

TradeRecipe: Constify default values, make NBT Tag constants private

3f7f8d3

...

7167322

Add (very) helpful document for plugin developers

fe16eaf

Profession: Remove recipes

75df228

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Villager v2 and trade functionality #6310

Implement Villager v2 and trade functionality #6310

alvin0319 commented Mar 26, 2024 •

edited

ShockedPlot7560 commented Mar 26, 2024

Matze997 commented Mar 26, 2024

alvin0319 commented Mar 26, 2024

alvin0319 commented Apr 1, 2024

IvanCraft623 left a comment

IvanCraft623 Apr 2, 2024

alvin0319 Apr 2, 2024

IvanCraft623 Apr 2, 2024

IvanCraft623 Apr 2, 2024

alvin0319 Apr 2, 2024

IvanCraft623 Apr 2, 2024

IvanCraft623 Apr 2, 2024

alvin0319 Apr 3, 2024

IvanCraft623 Apr 2, 2024

alvin0319 Apr 2, 2024

IvanCraft623 commented Apr 2, 2024

ShockedPlot7560 commented Apr 3, 2024

alvin0319 commented Apr 3, 2024

pandaa-be commented Apr 4, 2024


		namespace pocketmine\entity\trade;

		final class TradeRecipeData{

Implement Villager v2 and trade functionality #6310

Are you sure you want to change the base?

Implement Villager v2 and trade functionality #6310

Conversation

alvin0319 commented Mar 26, 2024 • edited

Introduction

Relevant issues

Changes

API changes

Behavioural changes

Backwards compatibility

Follow-up

Tests

ShockedPlot7560 commented Mar 26, 2024

Matze997 commented Mar 26, 2024

alvin0319 commented Mar 26, 2024

alvin0319 commented Apr 1, 2024

IvanCraft623 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanCraft623 commented Apr 2, 2024

ShockedPlot7560 commented Apr 3, 2024

alvin0319 commented Apr 3, 2024

pandaa-be commented Apr 4, 2024

alvin0319 commented Mar 26, 2024 •

edited