Readonly inventory

[maxomatic458]

Objective

• ability to make inventories read only (player should be able to click things in the inventory and still emit a click event, this can be useful for creating inventory menus.
• closes read-only inventories #427
• related Inventory Menus #307 Very rough Inventory Menu prototype #355

Solution

• adds a public readonly: bool field to the Inventory component, that will make any interactions with this item impossible (includes: moving, shift moving, hotbar moving, dropping) if a player inventory is readonly, then the player will also not be able to drop items (even when not in the inventory), so the drop event will not be emitted (this could be changed if requested)
• when implementing this i discovered a bug where a player is not able to put a item from a open inventory in the offhand (by hitting F) that will cause a desync. On the client the item will be in the offhand, but if you try to interact with that it dissapears. (unrelated to this PR and will not be fixed in this PR)

[lukashermansson]

Hey nice work, just chiming in with a couple of thoughts,

I read what RJ wrote in the draft for inventory menus in the draft pr #355 it is possible to implement readonly behavior in user space. So I think exploring implementing this as a separate crate we maintain is worthwhile.

The alternative approach is to not bloat up the inventory crate with the readonly stuff, it should be implementable as its own separate crate with the approach to listen to the ClickSlotEvent, DropItemStackEvent and CreativeInventoryActionEvent and modify the inventory back to its previous state based on the information presented in the event. (setting the modified slots back to how they where, and clearing the cursor_item)

With this approach it should be possible i think to have another create valance_readonly_inventory that could be optionally added along with its own "read only" marker component (to know when these actions should be reverted by the readonly inventory plugin)

One footgun with the described approach (the separate crate one) is that for example a user implementing a feature might be listening to the DropItemStackEvent and spawning an item entity https://minecraft.fandom.com/wiki/Item_(entity) without also properly filtering out the Readonly inventories, would be easy to forget/not know about. If its in the same crate I guess its simpler to not emit the events unless "real stuff happend", so I guess that should be considered before picking one approach over the other. (This might not be the first place such a problem exists?)

I don't really have any strong argument for one approach or the other myself currently. I think its worthwile to bring up this approach too :D

[dyc3]

I'm not entirely opposed to having a readonly flag on Inventory. Should make inventory menus easier.

There needs to be some tests though.

[maxomatic458]

im currently making some tests

and i made this test here:

#[test]
fn test_prevent_hotbar_item_click_container_readonly_inventory() {
    let ScenarioSingleClient {
        mut app,
        client,
        mut helper,
        ..
    } = ScenarioSingleClient::new();

    // Process a tick to get past the "on join" logic.
    app.update();
    helper.clear_received();

    // player inventory is not read-only
    let mut player_inventory = app
        .world_mut()
        .get_mut::<Inventory>(client)
        .expect("could not find inventory for client");

    // 36 is the first hotbar slot
    player_inventory.set_slot(36, ItemStack::new(ItemKind::Diamond, 1, None));

    let open_inv_ent = set_up_open_inventory(&mut app, client);

    let mut open_inventory = app
        .world_mut()
        .get_mut::<Inventory>(open_inv_ent)
        .expect("could not find inventory for client");

    // Open inventory is read-only
    // open_inventory.readonly = true;
    open_inventory.set_slot(0, ItemStack::new(ItemKind::IronIngot, 10, None));

    let inv_state = app.world_mut().get::<ClientInventoryState>(client).unwrap();
    let state_id = inv_state.state_id();
    let window_id = inv_state.window_id();

    // The player hovers over the iron ingots in the open inventory, and tries
    // to move them to their own (via pressing 1), which should swap the iron
    // for the diamonds. However the opened inventory is read-only, so nothing 
    // should happen.
    helper.send(&ClickSlotC2s {
        window_id,
        state_id: VarInt(state_id.0),
        slot_idx: 0,
        button: 0, // hotbar slot starting at 0
        mode: ClickMode::Hotbar,
        slot_changes: vec![
            // First SlotChange is the item is the slot in the player's hotbar.
            // target slot.
            SlotChange {
                idx: 0,
                stack: ItemStack::new(ItemKind::Diamond, 1, None),
            },
            SlotChange {
                // 54 is the players hotbar slot 1, when the 9x3 inventory is opnened.
                idx: 54,
                stack: ItemStack::new(ItemKind::IronIngot, 10, None),
            },
            // The second one is the slot in the open inventory, after the ClickSlot action
            // source slot.
        ]
        .into(),
        carried_item: ItemStack::EMPTY,
    });

    app.update();

    let sent_packets = helper.collect_received();

    // 1 resync for each inventory
    sent_packets.assert_count::<InventoryS2c>(2);

    // Make assertions
    let player_inventory = app
        .world_mut()
        .get::<Inventory>(client)
        .expect("could not find client");

    // Opened inventory is read-only, the items are not swapped.
    assert_eq!(player_inventory.slot(36), &ItemStack::new(ItemKind::Diamond, 1, None));

    let open_inventory = app
        .world_mut()
        .get::<Inventory>(open_inv_ent)
        .expect("could not find inventory");

    // Opened inventory is read-only, the items are not swapped.
    assert_eq!(open_inventory.slot(0), &ItemStack::new(ItemKind::IronIngot, 10, None));
}

this test here will pass if the items were not swapped (as they usually should)

for some reason, this always passes (even when the inventory is not readonly, and this also happens on the latest master branch)

The event being sent is the exact same that is also sent when i test it with a game client and replicate this (and that works fine, no desync)

Any ideas why the inventory slots are not updating in the inventory component?

Edit:
This works fine if thats regular item movement (like by using the mouse), that just seems to happen with Shift/Hotbar moving

[maxomatic458]

ive added all the tests ive made so far, as i said in the message above some of them do not work (because of the strange behavior where the inventory content does not update). Ive commented them out for now.
Let me know if you have any idea why those are not working as expected.

[dyc3]

IIRC, We have some basic anti cheat validation on slot_changes that ignores changes that would result in item duplication. The items need to exist in the inventories before they can be moved. I can't give you a code link at the moment, but I would look in that direction.

[lukashermansson]

After a lot of trial and error i finally figured this out

This test below is an implementation that can properly swap two items (NON-read only) should be easy enough to make it pass in the read only case.

there was an app.update() missing, so the items did not really "exist" yet at the time the client packet got sent.

#[test]
fn test_hotbar_item_swap_container() {
    let ScenarioSingleClient {
        mut app,
        client,
        mut helper,
        ..
    } = ScenarioSingleClient::new();

    // Process a tick to get past the "on join" logic.
    app.update();

    let mut player_inventory = app
        .world_mut()
        .get_mut::<Inventory>(client)
        .expect("could not find inventory for client");

    // 36 is the first hotbar slot
    player_inventory.set_slot(36, ItemStack::new(ItemKind::Diamond, 1, None));

    let open_inv_ent = set_up_open_inventory(&mut app, client);

    let mut open_inventory = app
        .world_mut()
        .get_mut::<Inventory>(open_inv_ent)
        .expect("could not find inventory for client");

    open_inventory.set_slot(0, ItemStack::new(ItemKind::IronIngot, 10, None));

    // This update makes sure we have the items in the inventory by the time the
    // client wants to update these
    app.update();
    helper.clear_received();
    let inv_state = app.world_mut().get::<ClientInventoryState>(client).unwrap();
    let state_id = inv_state.state_id();
    let window_id = inv_state.window_id();

    // The player hovers over the iron ingots in the open inventory, and tries
    // to move them to their own (via pressing 1), which should swap the iron
    // for the diamonds.
    helper.send(&ClickSlotC2s {
        window_id,
        state_id: VarInt(state_id.0),
        slot_idx: 0,
        button: 0, // hotbar slot starting at 0
        mode: ClickMode::Hotbar,
        slot_changes: vec![
            // First SlotChange is the item is the slot in the player's hotbar.
            // target slot.
            SlotChange {
                idx: 0,
                stack: ItemStack::new(ItemKind::Diamond, 1, None),
            },
            SlotChange {
                // 54 is the players hotbar slot 1, when the 9x3 inventory is opnened.
                idx: 54,
                stack: ItemStack::new(ItemKind::IronIngot, 10, None),
            },
            // The second one is the slot in the open inventory, after the ClickSlot action
            // source slot.
        ]
        .into(),
        carried_item: ItemStack::EMPTY,
    });

    app.update();

    let sent_packets = helper.collect_received();

    // No resyncs beacuse the client was in sync and sent us the updates
    sent_packets.assert_count::<InventoryS2c>(0);

    // Make assertions
    let player_inventory = app
        .world_mut()
        .get::<Inventory>(client)
        .expect("could not find client");

    // Swapped items sucessfully
    assert_eq!(
        player_inventory.slot(36),
        &ItemStack::new(ItemKind::IronIngot, 10, None)
    );

    let open_inventory = app
        .world_mut()
        .get::<Inventory>(open_inv_ent)
        .expect("could not find inventory");

    assert_eq!(
        open_inventory.slot(0),
        &ItemStack::new(ItemKind::Diamond, 1, None)
    );
}

what finnaly helped me figure everything out was setting
RUST_LOG=debug as an env variable and running
cargo test tests::inventory::test_hotbar_item_swap_container

the output gave a lot away, the resyncs where also a bit suspicious, as when i looked at what happens with a client and server (with the packet inspector) in this instance no re-syncs where observed

[maxomatic458]

should be ready now!