Skip to content

item components + inventory sync#330

Open
Sweattypalms wants to merge 17 commits intomasterfrom
feature/item-components
Open

item components + inventory sync#330
Sweattypalms wants to merge 17 commits intomasterfrom
feature/item-components

Conversation

@Sweattypalms
Copy link
Member

@Sweattypalms Sweattypalms commented Jan 25, 2026

item components + inventory sync + inventory saving into database with components is fixed now.

ReCore-sys and others added 15 commits January 19, 2026 19:05
idk if float and list types are added or not, change if not.
Implements the complete Minecraft 1.21.x item component system with
NetEncode/NetDecode support for server-client item data serialization.

Key changes:
- Add all component types (IDs 0-95): custom data, enchantments, food,
  consumables, tools, weapons, potions, fireworks, entity variants, etc.
- Add helper enums: Rarity, DyeColor, EquippableSlot, ConsumableAnimation,
  AttributeModifierOperation, FireworkExplosionShape, and entity variants
- Add helper structs: PotionEffect, FireworkExplosion, SoundEvent,
  BlockPredicate, TrimMaterial, TrimPattern, Instrument, JukeboxSong, etc.
- Implement ID-or-inline patterns for registry references
- Fix IDSet codec with proper encode/decode implementations
- Add RawNbt wrapper for arbitrary NBT component data

Note: 4 components with NBT<TextComponent> fields use todo!() for decode
as TextComponent doesn't yet implement FromNbt.
- Add Component factory methods for cleaner item creation
  (custom_name, lore, max_stack_size, etc.)
- Add ItemBuilder fluent API following TextComponentBuilder pattern
- Create storage layer with StorageComponent, StorageInventorySlot
  for bitcode serialization
- Add StorageInventory, StorageEnderChest, StorageOfflinePlayerData
  with From/TryFrom conversions
- Re-enable player data persistence in world_sync, connection_killer
- Load saved player data (position, gamemode, abilities) on login
- Add streaming NBT reader for network format (read_nbt_bytes)
- Implement nameless NBT parsing for TextComponent decoding
- Add roundtrip tests for CustomName and multi-component slots
- Fix NbtTape usage that expected named NBT format

Note: Still debugging client-to-server NBT decode issue
…ocol

The Minecraft protocol uses asymmetric encoding for slot component data:
- Server → Client: ComponentID + Data (no length prefix)
- Client → Server: ComponentID + Length + Data (with length prefix)

Changes:
- Component::decode now reads length prefix before component data
- Parse component data from bounded buffer for safety
- TextComponent parsing updated to not double-read length prefix
- Lore elements use streaming NBT reader (no per-element length)
- Roundtrip tests marked ignored (asymmetric format by design)

This fixes the "Invalid NBT tag type: 15" error when client sends
slot data back to server (e.g., creative mode item duplication).
- Initial inventory sync: sends full inventory via SetContainerContent on join
- Equipment broadcast: other players see armor/held items via SetEquipment packet
- Join equipment exchange: new players see existing players' gear and vice versa
- Plugin hooks: InventorySynced, EquipmentChanged, HeldItemChanged messages

Also fixed timing bug in give_test_item.rs and join_equipment_exchange by
using Added<PlayerIdentity> instead of PlayerJoined message, ensuring
entities are queryable after Bevy commands are applied.
Kept both inventory sync components and master's emit_player_joined pattern.
The manual NetEncode impl was only writing payload data without the
packet ID, causing clients to misparse the packet as bundle_delimiter
(ID 0) and crash.

Solution: Move the equipment list encoding logic to a wrapper type
(EquipmentList) and let the derive macro handle proper packet framing.
- Remove debug logging from slot.rs that read entire streams into buffers
- Replace 18 unsafe transmutes with safe enum conversion functions
- Remove dead component_id_name() function
- Add helper methods to EquipmentState (non_empty_slots, is_empty)
- Gate give_test_item system behind #[cfg(debug_assertions)]
- Add clippy expect for complex query types

/// Gives new players a test item with components.
/// Uses `Added<PlayerIdentity>` to detect new players after commands are applied.
pub fn handle(
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be a command instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

If there is now a conversion between in-memory and disk format, can we use the unserializable Position here?

- Add limits.rs with MAX_NBT_SIZE (2MB) and MAX_NBT_DEPTH (512)
- Validate NBT lengths are non-negative before allocation
- Track and enforce depth limits on compound/list recursion
- Add size limit checks before all NBT allocations
- Fix negative VarInt cast in component decoding
- Add error logging for storage conversion failures
- Replace all todo!() panics with graceful error handling
- Add AsyncNotSupported error variants for unimplemented async paths

Prevents memory exhaustion attacks from malicious NBT data and
eliminates potential panics in production from unimplemented code paths.
- Convert give_test_item auto-join system to /givetest command
- Use Position type in OfflinePlayerData instead of tuple
- Keep (f64, f64, f64) in StorageOfflinePlayerData for serialization

Addresses feedback from @ReCore-sys on PR #330.
components_to_remove: None,
components_to_add_count: None,
components_to_remove_count: None,
components_to_add: Vec::new(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't these have the count or am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

Counts were intentionally removed. They're not derived from Vec::len() during encoding. Explicit count fields were redundant and a source of potential count/data mismatch bugs.

pub fn initial_inventory_sync(
mut commands: Commands,
state: Res<GlobalStateResource>,
query: Query<(Entity, &Inventory, &StreamWriter), With<NeedsInventorySync>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

The marker component is not removed/added in the same order every tick, you should probably use a normal component with a flag in it

Copy link
Member Author

Choose a reason for hiding this comment

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

Replaced NeedsInventorySync marker with Bevy's Added<Inventory> filter and added an explicit .after(emit_player_joined) ordering constraint. Eliminates the marker entirely and guarantees the system runs after ApplyDeferred flushes the entity.

/// Total bytes read so far
bytes_read: usize,
/// Current nesting depth (compound/list)
depth: usize,
Copy link
Contributor

Choose a reason for hiding this comment

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

If the depth is capped at 512 this should be a u16

Copy link
Member Author

Choose a reason for hiding this comment

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

usize is idiomatic for sizes/indices and avoids casts at every usage site. It's a stack-local value, the 6 bytes saved don't matter.

/// Tracks state during NBT reading for security validation.
struct NbtReadState {
/// Total bytes read so far
bytes_read: usize,
Copy link
Contributor

Choose a reason for hiding this comment

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

If size read is 2MB this should be a u32

Copy link
Member Author

Choose a reason for hiding this comment

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

usize matches what Vec::len() and slice methods return. Using u32 would just add as usize casts everywhere.


/// Reads the payload for an NBT tag (after the tag type byte).
///
/// This function is recursive for compound and list tags.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are stack overflows handled? The recursion depth is technically capped at 512, but is this deep enough to cause a stack overflow?

Copy link
Member Author

Choose a reason for hiding this comment

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

512 is the same limit Minecraft's own NBT parser uses. Stack usage per frame is a few small fixed buffers and pointer-sized locals, not a concern.


/// Storage-friendly ender chest for bitcode persistence.
#[derive(Clone, Debug, Encode, Decode)]
pub struct StorageEnderChest(pub StorageInventory);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as other storage formats, why?

Copy link
Member Author

Choose a reason for hiding this comment

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

see my reply on storage.rs

//! Item component types for the Minecraft protocol.
//!
//! This module implements all item components as defined in the
//! [Minecraft protocol documentation](https://minecraft.wiki/w/Java_Edition_protocol/Slot_data).
Copy link
Contributor

Choose a reason for hiding this comment

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

This URL should target 1.21.8, not the latest


#[derive(Debug, Clone, Hash, Default, PartialEq, Decode, Encode)]
/// Represents an inventory slot with item data and components.
/// See: https://minecraft.wiki/w/Java_Edition_protocol/Slot_data
Copy link
Contributor

Choose a reason for hiding this comment

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

This url should point at 1.21.8, not latest

Copy link
Member Author

Choose a reason for hiding this comment

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

Slot_data page isn't referenced by versions unlike the protocol page iirc

@@ -0,0 +1,78 @@
//! SetEquipment packet for broadcasting player equipment to other players.
//! Protocol: https://minecraft.wiki/w/Protocol#Set_Equipment
Copy link
Contributor

Choose a reason for hiding this comment

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

This URL doesn't point to anything helpful, and for the latest version

_writer: &mut W,
_opts: &NetEncodeOpts,
) -> Result<(), NetEncodeError> {
unimplemented!("Async encoding not needed for server-to-client packets")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you not have it implemented here but implemented in other files?

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.

3 participants