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

[ECS] Inventory interactions #189

Merged
merged 38 commits into from
Jan 22, 2023
Merged

Conversation

dyc3
Copy link
Collaborator

@dyc3 dyc3 commented Jan 16, 2023

  • add helper to convert slot ids
  • implement system to handle click container events
  • update inventory_test example with block platform and some chests to open
  • add toggle_gamemode_on_sneak system to example
  • fix query filter that was causing clients to get spammed with OpenScreen
  • move state_id to Client component
  • only send modified slots when observed inventories are changed
  • force all click container packets to be handled before update packets are built and sent
  • mark inventories as dirty instead of just sending the contents
  • add handle_set_slot_creative to handle SetCreativeModeSlot events
  • exclude clients with currently open inventories from being updated by update_player_inventories

Test plan

  1. Start the inventory_test example
cargo run --example inventory_test
  1. Join the server
  2. Walk to the small brick square
  3. The copper, iron, and gold blocks have chests on top of them (they are invisible until we have block entities)
  4. Give yourself some items
  5. Place those items in one of the inventories
  6. Give yourself some more items
  7. Sneak to change game mode to survival
  8. Open the same inventory and move some items between your inventory and the chest's inventory.
  9. Repeat 5-9, but have another client join and observe the chest you're interacting with.
  10. The second client should be able to see the inventory update in real time.

@dyc3 dyc3 marked this pull request as ready for review January 16, 2023 19:15
Copy link
Member

@rj00a rj00a left a comment

Choose a reason for hiding this comment

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

Nice work. It's exciting to see this coming together.

Some issues:

  • Opening an inventory for the first time, the client is sent the SetContainerSlot packet for every occupied slot in addition to the initial SetContainerContent packet. This happens because the modified bits are only cleared when the inventory is open. This can be fixed by clearing the modified bits of all inventories unconditionally in a separate system that runs after update_open_inventories

    However, this wouldn't fix the problem if the inventory happens to be modified on the same tick it's opened. I could see this being a problem with code that wants to create and open an inventory at the same time. Solution: Don't run update_open_inventories on clients with newly added OpenInventory components by using ChangeTrackers? Or maybe combine update_client_on_open_inventory and update_open_inventories into one system?

    "Don't initialize and update on the same tick" seems to be a recurring pattern I'm noticing.

  • SetContainerSlot packets are sent for slots that the client itself modified, even though this is not necessary. I think this can be fixed by storing slot modifications in Client and filtering SetContainerSlot writes based on this information.

  • When validation is implemented, should we validate the ClickContainer event before it's sent so users don't need to worry about illegal events?

@dyc3
Copy link
Collaborator Author

dyc3 commented Jan 17, 2023

When validation is implemented, should we validate the ClickContainer event before it's sent so users don't need to worry about illegal events?

I don't think so, it might be useful to be able to read all of the events regardless of validity. Instead, maybe we could validate and then emit other events based on that? We are going to need to emit drop item events anyway.

@dyc3 dyc3 requested a review from rj00a January 17, 2023 17:33
Copy link
Member

@rj00a rj00a left a comment

Choose a reason for hiding this comment

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

  • In creative, move an item from the creative menu into your hotbar. Then switch to survival and try to click on it. You'll be sent a SetContainerContent packet.
  • Open an inventory and move an item from your inventory into the open inventory. Without closing the window, move the item back to your inventory in a different spot than where it was originally. Once the window is closed, you will be sent a SetContainerSlot packet.

@dyc3 dyc3 requested a review from rj00a January 18, 2023 19:01
Copy link
Member

@rj00a rj00a left a comment

Choose a reason for hiding this comment

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

I'm still getting Client state id mismatch, resyncing while switching between creative and survival. At one point, every interaction with the inventory in survival mode caused a Client state id mismatch, resyncing to be printed.

@dyc3
Copy link
Collaborator Author

dyc3 commented Jan 19, 2023

I'm not able to reproduce the state id mismatch you're talking about.

Copy link
Member

@rj00a rj00a left a comment

Choose a reason for hiding this comment

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

  • The player inventory is missing a slot for index 45 (the offhand). Need to increase the size from 45 to 46.
  • Shift-clicking the "Destroy Item" box in creative doesn't function correctly. The client is sending SetCreativeModeSlot packets to remove the items, but it's not actually clearing the items locally. After some testing with vanilla, it appears that we need to send all slot changes back to the client but only while the client is in creative mode. Mojang why??
  • Regarding "At one point, every interaction with the inventory in survival mode caused a Client state id mismatch, resyncing to be printed.", basically what happened is that the event.state_id was always 0 while client.inventory_state_id kept going up. I haven't reproduced the issue since fixing the a ^ b thing, so it might've been a consequence of that.

@dyc3 dyc3 force-pushed the ecs-inventory-interact branch from 27068bf to 5e150c7 Compare January 20, 2023 17:06
@dyc3 dyc3 requested a review from rj00a January 20, 2023 18:28
Copy link
Member

@rj00a rj00a left a comment

Choose a reason for hiding this comment

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

I am starting to lose my sanity.

@dyc3
Copy link
Collaborator Author

dyc3 commented Jan 21, 2023

We really need to be able to do automated e2e tests on this, because this is ridiculous.

@dyc3 dyc3 requested a review from rj00a January 21, 2023 17:12
@dyc3 dyc3 force-pushed the ecs-inventory-interact branch from ff3e307 to 8bb95dd Compare January 21, 2023 18:08
@rj00a rj00a merged commit cb0d2c6 into valence-rs:ecs_rewrite Jan 22, 2023
@rj00a
Copy link
Member

rj00a commented Jan 22, 2023

Still seeing some issues but yeah let's not drag this out forever.

@dyc3 dyc3 deleted the ecs-inventory-interact branch January 22, 2023 13:22
rj00a added a commit that referenced this pull request Feb 3, 2023
Motivation: There are a lot of edge cases we have to handle, and every
new line of code we write increases the risk of regressions. Regressions
slow down dev time: a perfect example of this is what happened in #189.

Unsolved issues:
- [x] `test_client_position` is flakey

### Test plan

```
cargo test -p valence_new
```

---------

Co-authored-by: Ryan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants