1. The forums will be archived and moved to a read only mode in about 2 weeks (mid march).

RFC Improve inventory API to support extending PlayerInventory class

Discussion in 'Contributing & RFCs' started by Eternalharvest, Jul 10, 2017.

  1. Eternalharvest

    Eternalharvest Spider

    Messages:
    11
    GitHub:
    eternalharvest
    Hi, there!
    I'm new to this forum, and i'm not sure if here is the correct place to post this...
    please tell me if i was wrong.

    I'm currentry working on BigBrother plugin to implement inventory related features.
    I think there are lot of problem about PlayerInventory class for pc users because of the difference of hotbar implementation between PC and PE.
    So, I tried to create new DesktopPlayerInventory class which extends PlayerInventory in BigBrother plugin.
    Because some code manipulate the inventory via PlayerInventory#inventory member variable or PlayerInventory#getInventory() method.
    I think there are many plugin which manipulate inventory through this API too.
    So, i want to make newly created DesktopPlayerInventory class compatible with those existing code.

    So I wrote a patch for BigBrotherPlugin to instantiate DesktopPlayerInventory class and initialize it.
    Please see patch below.

    https://github.com/eternalharvest/B...e7dd689#diff-7c176b9f07b8ceb0fe9b6e67a6a7d019

    I copied Human#initEntity() method from PMMP, and modify it (original code is commented out) because I want to instantiate DesktopPlayerInventory class instead of PlayerInventory class.
    But i think this method is too big, and it has parent::initEntity() call, so i need to call Living::initEntity() instead (but i think it is dangerous).
    And it seems Human#initEntity() method is updated frequently, so I don't want to copy and paste it.

    So, I wish the Human#initEntity() is devided into several part of method.
    For example, I'm happy if Human#initInventory() method is added.
    It's very simple solution but i'm not sure whether these kind of changes are acceptable or not.
    And i'm not sure if my approach to extends PlayerInventory is correct.

    Thank you for taking the time
    And I appreciate any help.
     
    SOFe likes this.
  2. Eternalharvest

    Eternalharvest Spider

    Messages:
    11
    GitHub:
    eternalharvest
    I'm sorry for my dirty post.
    It's may bit confusing because my english is not so well...

    I just want to know whether if PlayerInventory class can be used for MCPC without extends the inventory API (I mean extends PlayerInventory class).
    Or, if I could'nt use inventory api for MCPC clients, I want to know how to implment a Inventory feature which compatible with MCPC.

    I think, I need to extends PlayerInventory class to implement inventory related features compatible with MCPC clients.
    PlayerInventory#getHotbarSlotItem() always returns empty item (Item::AIR with count 0) if hotbar link is not configured, so i need to override this method.
    And, I think I need to make hotbar link to fixed index.

    I found the following comment before the PlayerInventory#setHotbarSlotIndex() method declaration.

    > Do not change hotbar slot mapping with plugins, this will cause myriad client-sided bugs, especially with desktop GUI clients.

    I know that hotbar link changes cause some problem in MCPC client.
    But even if any plugin would'nt call this method, inventory feature not works correctly without extending the PlayerInventory class or apply the some patches for PlayerInventory class I think...
    I think I must not make a patch for PlayerInventory class in PMMP project.
    So, I want to add new DesktopPlayerInventory class in the plugin which extends PlayerInventory class.

    And I'm not sure if it is correct that overriding Human#initEntity() in plugin to instantiate extended PlayerInventory class.
    Is theare any plan to refactor the inventory related APIs?
    Or is there any discussions about inventory related features?
    It seem that there is some api3/* branch, but api3/inventory branch is not exists.

    Any opinion is welcome!
    Thank you for reading such a long and dirty post...
     
    SOFe likes this.
  3. Awzaw

    Awzaw Zombie Pigman Poggit Admin

    Messages:
    726
    GitHub:
    awzaw
    From what I understand there is an inventory rewrite coming quite soon to the clients, so if I were you I'd wait until that's done.
     
    SOFe likes this.
  4. Eternalharvest

    Eternalharvest Spider

    Messages:
    11
    GitHub:
    eternalharvest
    Thank you for your reply!!

    > From what I understand there is an inventory rewrite coming quite soon to the clients, so if I were you I'd wait until that's done.
    How did you know that?
    Is there any discussion forum or working group?
     
  5. HBIDamian

    HBIDamian HBIDamian Staff Member

    Messages:
    365
    GitHub:
    HBIDamian
    You know you can press reply at any point of your text to get someone else’s quote... you don’t have to copy and paste it.
     
  6. SOFe

    SOFe Administrator Staff Member PMMP Team Poggit Admin

    Messages:
    1,968
    GitHub:
    sof3
    This thread is the right place for it :)
     
  7. Eternalharvest

    Eternalharvest Spider

    Messages:
    11
    GitHub:
    eternalharvest
    Thank u for the information!
     
  8. Eternalharvest

    Eternalharvest Spider

    Messages:
    11
    GitHub:
    eternalharvest
    when i am working on BigBrotherPlugin to implement inventory feature, i found a behaviour of PlayerInventory class in PMMP.
    I was trying to translate armor slot change packet from PE to PC, i found the problem in PlayerInventory#onSlotChange() method.
    Please see the code below.

    https://github.com/pmmp/PocketMine-...tmine/inventory/PlayerInventory.php#L257-L269

    Correct implementation of PlayerInventory#onSlotChange() as follows.



    I don't know whether if this is a bug or not, I think this code sends useless packet.
    if this is a really bug, i have to make a issue on github?
     
  9. Eternalharvest

    Eternalharvest Spider

    Messages:
    11
    GitHub:
    eternalharvest
    I'm sorry it seems already fixed in api3/block branch.
     
  10. Eternalharvest

    Eternalharvest Spider

    Messages:
    11
    GitHub:
    eternalharvest
    I finally implement some inventory related features in the BigBrother plugin. bit buggy and dirty though...
    My working branch is located at following link.
    Currently, PlayerInventory, Chest (include DoubleChest) and Furnace works.

    https://github.com/eternalharvest/BigBrother/tree/develop

    In the process of implementation of inventory feature, I noticed the lacks of some function.
    Here is the list of features I want PMMP to implement.

    MIDDLE PRIORITY
    • BaseInventory#canSetItem(int $index) : bool; # subclass may override this
    • BsseInventory#getSlotType(int $index) : int; # which returns SlotItem::* consntants value.
    Since PC user can click any slot, we need to implement a method to detect if user can set item in specified slot index. Ofcourse, we can implement it within the BigBrother plugin without any problem, but I think it's make sense to implement in PMMP.

    If it necessary, I will write a change-set to implement these methods and send a pull-request to PMMP project on github. But I think we need to discuss about this.

    And, I want to add this method.

    LOW PRIORITY
    • PlayerInventory#setHotbarSlotItem(int $hotbarSlotIndex, Item $item) : bool;
    It seems PlayerInventory#getHtobarSlotItem(int $hotbarSlotIndex) : Item; is in the PMMP code base, but there is no setter method corresponding to it. Is theare any reason? I think there is no reason not to add this method...
     
  11. Eternalharvest

    Eternalharvest Spider

    Messages:
    11
    GitHub:
    eternalharvest
    "which returns SlotType::* consntants value." was correct, sorry.
     
  1. This site uses cookies to help personalise content, tailor your experience and to keep you logged in if you register.
    By continuing to use this site, you are consenting to our use of cookies.