valence-rs / valence

A Rust framework for building Minecraft servers.
http://valence.rs/
MIT License
2.7k stars 142 forks source link

fix so cursor packet is sent to client with open inventories #653

Open lukashermansson opened 3 hours ago

lukashermansson commented 3 hours ago

Objective

Attempts to solve the issue shown in discord https://discord.com/channels/998132822239870997/1292890441011957830 where doing has no effect in a "open inventory"

Looks like an oversight in the system to update the open inventories, no attempt to send any packet to the user was made when the CursorItem component was updated by any system

Playground to reproduce the issue solved by this pr.

use valence::interact_block::InteractBlockEvent;
use valence::inventory::ClickSlotEvent;
use valence::log::LogPlugin;
use valence::prelude::*;

#[allow(unused_imports)]
use crate::extras::*;

const SPAWN_Y: i32 = 64;
const CHEST_POS: [i32; 3] = [0, SPAWN_Y + 1, 3];

pub(crate) fn build_app(app: &mut App) {
    app.insert_resource(NetworkSettings {
        connection_mode: ConnectionMode::Offline,
        ..Default::default()
    })
    .add_plugins(DefaultPlugins.build().disable::<LogPlugin>())
    .add_systems(Startup, setup)
    .add_systems(
        EventLoopUpdate,
        (toggle_gamemode_on_sneak, open_chest, select_menu_item),
    )
    .add_systems(Update, (init_clients, despawn_disconnected_clients))
    .run();
}

fn setup(
    mut commands: Commands,
    server: Res<Server>,
    biomes: Res<BiomeRegistry>,
    dimensions: Res<DimensionTypeRegistry>,
) {
    let mut layer = LayerBundle::new(ident!("overworld"), &dimensions, &biomes, &server);

    for z in -5..5 {
        for x in -5..5 {
            layer.chunk.insert_chunk([x, z], UnloadedChunk::new());
        }
    }

    for z in -25..25 {
        for x in -25..25 {
            layer
                .chunk
                .set_block([x, SPAWN_Y, z], BlockState::GRASS_BLOCK);
        }
    }
    layer.chunk.set_block(CHEST_POS, BlockState::CHEST);

    commands.spawn(layer);

    let mut inventory = Inventory::new(InventoryKind::Generic9x3);
    inventory.set_slot(1, ItemStack::new(ItemKind::Apple, 64, None));
    inventory.set_slot(2, ItemStack::new(ItemKind::Diamond, 40, None));
    inventory.set_slot(3, ItemStack::new(ItemKind::Diamond, 30, None));
    commands.spawn(inventory);
}

fn init_clients(
    mut clients: Query<
        (
            &mut EntityLayerId,
            &mut VisibleChunkLayer,
            &mut VisibleEntityLayers,
            &mut Position,
        ),
        Added<Client>,
    >,
    layers: Query<Entity, (With<ChunkLayer>, With<EntityLayer>)>,
) {
    for (mut layer_id, mut visible_chunk_layer, mut visible_entity_layers, mut pos) in &mut clients
    {
        let layer = layers.single();

        layer_id.0 = layer;
        visible_chunk_layer.0 = layer;
        visible_entity_layers.0.insert(layer);
        pos.set([0.0, f64::from(SPAWN_Y) + 1.0, 0.0]);
    }
}

fn open_chest(
    mut commands: Commands,
    inventories: Query<Entity, (With<Inventory>, Without<Client>)>,
    mut events: EventReader<InteractBlockEvent>,
) {
    for event in events.read() {
        if event.position != CHEST_POS.into() {
            continue;
        }
        let open_inventory = OpenInventory::new(inventories.single());
        commands.entity(event.client).insert(open_inventory);
    }
}
fn select_menu_item(
    mut clients: Query<(Entity, &mut CursorItem)>,
    mut events: EventReader<ClickSlotEvent>,
) {
    for event in events.read() {
        let Ok((_player, mut cursor_item)) = clients.get_mut(event.client) else {
            continue;
        };

        // this does not work properly
        cursor_item.set_if_neq(CursorItem(ItemStack::EMPTY));
    }
}
// Add more systems here!

Solution

Copied the approach to sync the CursorItem state from the update_player_inventories system to the update_open_inventories where it was previously left out (my assumption is that it was left out by mistake).

dyc3 commented 2 hours ago

The tests will need to be updated.

lukashermansson commented 2 hours ago

This has a test failure(as you also already spotted), don't merge this just yet, will need to get an implementation that does not send a packet to the client when it was the client that updated it! thanks for the quick review!