Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce Item Lock API mainly for Client-Side inventory action prevention #5333

Open
wants to merge 15 commits into
base: minor-next
Choose a base branch
from

Conversation

yuyaprgrm
Copy link
Contributor

@yuyaprgrm yuyaprgrm commented Oct 12, 2022

Introduction

Currently, PocketMine-MP have no API for item lock api provided via item's NBT(minecraft:item_lock).
The purpose of this PR is to provide an API for item locking and implement logic for server-side validation

Relevant issues

N/A

Changes

API changes

  • add Item::setLockMode(?ItemLockMode $lockMode) : self
  • add Item::getLockMode() : ?ItemLockMode
  • add ItemLockMode enum

Behavioural changes

N/A

Backwards compatibility

N/A

Follow-up

Currently, value used in NBT is hardcoded. This value may be defined in BedrockProtocol.

Tests

Check whether it works correctly with ScriptPlugin below.

<?php

namespace famima65536\ItemLockTest;

use pocketmine\event\EventPriority;
use pocketmine\event\player\PlayerJoinEvent;
use pocketmine\item\ItemLockMode;
use pocketmine\item\VanillaItems;
use pocketmine\plugin\PluginBase;

/**
 * @name ItemLockTest
 * @main famima65536\ItemLockTest\Main
 * @version 0.1.0
 * @api 4.9.0
 * @description A plugin makes sure that Item::setLockMode() and Item::getLockMode() works well.
 * @author famima65536
 */


class Main extends PluginBase{

	protected function onEnable(): void{
		$this->getServer()->getPluginManager()->registerEvent(PlayerJoinEvent::class, function(PlayerJoinEvent $event) : void{
			$event->getPlayer()->getInventory()->setContents([
				VanillaItems::STICK()->setLockMode(ItemLockMode::SLOT()),
				VanillaItems::APPLE()->setLockMode(ItemLockMode::INVENTORY()),
			]);
		}, EventPriority::NORMAL, $this);
	}
}

image
image

@jasonw4331 jasonw4331 added Category: API Related to the plugin API Type: Contribution labels Oct 12, 2022
Copy link
Member

@dktapps dktapps left a comment

Choose a reason for hiding this comment

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

The server-side validation for this is incomplete. Since the current transaction system does not allow actions to be tracked when moving from one inventory to another, validation for this can't be easily implemented. This means that this effectively becomes a cosmetic-only change.

src/inventory/transaction/action/SlotChangeAction.php Outdated Show resolved Hide resolved
src/inventory/transaction/action/DropItemAction.php Outdated Show resolved Hide resolved
src/item/Item.php Outdated Show resolved Hide resolved
src/item/Item.php Outdated Show resolved Hide resolved
src/item/Item.php Outdated Show resolved Hide resolved
src/item/Item.php Outdated Show resolved Hide resolved
src/item/ItemLockMode.php Outdated Show resolved Hide resolved
@dktapps dktapps added Type: Enhancement Contributes features or other improvements to PocketMine-MP and removed Type: Contribution labels Nov 26, 2022
@yuyaprgrm
Copy link
Contributor Author

Progress: this feature looks to be implementable with new inventory transaction system in current minor-next branch.
May I work this PR with rebasing and force-pushing?

@dktapps
Copy link
Member

dktapps commented Mar 24, 2023

The core transaction system remains unchanged, and implementing core logic in the network layer is pretty dubious

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

dktapps commented Nov 15, 2024

Blocked waiting on #6505

@dktapps dktapps added the Status: Blocked Depends on other changes which are yet to be completed label Nov 15, 2024
@dktapps
Copy link
Member

dktapps commented Nov 29, 2024

I had a think about this, and realized it can probably be done without a revamp of the whole system.

Basically, this code would have to split $needItems and $haveItems into $needItemsByInventory and $haveItemsByInventory according to which inventory they came from (if they came from a SlotChangeAction).

Then, this code would have to be adapted to only look in the subset of items that are locked to the same inventory.

This way, the transaction would only balance if the same number of inventory-locked items were consumed & created by the same inventory.

Seems doable but not for the faint of heart.

Never mind, seems I misunderstood the function of the inventory lock. Apparently it allows moving from a chest -> player's inventory but not the other way round. It also allows moving from cursor <-> player inventory.

@dktapps dktapps removed the Status: Blocked Depends on other changes which are yet to be completed label Nov 29, 2024
@dktapps dktapps requested a review from a team as a code owner November 29, 2024 16:44
dktapps
dktapps previously approved these changes Nov 29, 2024
this behaves the same as inventory-lock up until it's placed into a slot, in that it can be taken from a chest (but not returned), and placed down into the player's inventory.
the only special behaviour of slot lock is that it can't be _removed_ from the slot.
transaction balancing & SlotChangeAction validation should stop this from happening anyway
@dktapps dktapps requested a review from a team November 29, 2024 18:43
@dktapps
Copy link
Member

dktapps commented Dec 2, 2024

PR is basically done but I think the terminology needs to be updated. ItemLockMode doesn't give much away, particularly since the intended function is actually to lock items specifically into a player's inventory. Without improvement, people might think this feature works on general inventories, like I did.

@dktapps dktapps self-assigned this Dec 2, 2024
@dktapps dktapps added the Status: Incomplete Work in progress label Dec 2, 2024
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 Status: Incomplete Work in progress Type: Enhancement Contributes features or other improvements to PocketMine-MP
Projects
Development

Successfully merging this pull request may close these issues.

3 participants