Dividing Clients and Entities into smaller components

[rj00a]

Brainstorming some ideas for how the monolithic Client and McEntity components could be split up into a set of smaller components. Here are some reasons you would want to do this:

• Improve query performance by increasing parallelism and useful data in the CPU cache.
• More modular design can split code into smaller understandable pieces.
• Integrate with Bevy's change detection.
• Share data between client and Minecraft entity when they share the same ECS entity. (Typically the client will have a player entity associated with it).
• Save memory by spawning entities without unused components.

Some reasons you wouldn't want to do this:

• Many corner cases and interactions that are not accounted for can lead to bugs.
• Potentially confusing and unintuitive.
• Invalid states are very much representable.

---

Component Name	Description	Effect When Added	Effect When Removed	Effect When Modified
Despawned	Any ECS Entities with this component are despawned at the end of the tick. This gives Valence an opportunity to perform deinitialization of components that can't be removed explicitly. In particular, McEntity and its other components.
Client	The main client component. Contains the TCP connection and packet buffers. Also contains immutable info such as username, UUID, and IP among other things.	Joins the client (Should only be done by Valence). Maybe we want to allow attaching Client to existing entities somehow?	No effect, but the client is disconnected when dropped. Removing only this component from an McEntity will allow you to take control.
Disconnected	Marker component to indicate that a client is disconnected.
Location	Contains the instance an Entity or Client is inside. Also contains the instance from the previous tick.		Disallowed at least for entities because we need it to clean up the instance.	Changes the instance a client or entity is in. Makes clients respawn even if old_instance == new_instance. Can be used to respawn the client after death.
Position	The XYZ coordinates (old and new) of a client or entity. Automatically updated by Valence as a client moves around.		Disallowed at least for entities because we need it to clean up the instance.	Teleports clients and entities when modified
Direction	Contains the pitch/yaw pair of an entity or client. Automatically updated by clients as they look around.		Disallowed(?)	Sets the pitch/yaw of entities and teleports clients to set their pitch/yaw. Can be combined with Position modifications to form a single teleport.
Velocity	Contains the velocity vector for clients and entities.			Causes clients to be launched. Unknown effect on entities. (Velocity is really weird in Minecraft).
OnGround	The on ground boolean for clients and entities. Automatically updated by clients.			Changes the on ground state for entities.
Vehicle	The vehicle entity a client or entity is riding.	TODO	TODO	TODO
DeathLocation	Contains a (DimensionId, BlockPos) pair. Used by respawning clients to set their recovery compass position			Can be modified, but won't have an effect until the client is respawned.
GameModeComponent (GameMode was taken)	Contains the game mode of a client.		Disallowed(?)	Sets the gamemode of the client it's attached to.
McEntity	The main component for Minecraft entities. Contains protocol entity ID and UUID.	Spawns the entity. Must have Location and Position to do so. Sets the protocol ID to the next unique value. Was initialized to zero when the component was created.	Definitely disallowed because we need the protocol ID to send despawn packets to clients. Also, we need the position+location to remove its ID from the instance it was in. Otherwise, there would be a memory leak.
EntityKindComponent (EntityKind was taken)	Contains the type of Minecraft entity (Blaze, Chicken, Player, etc).		Disallowed(?)	Respawns the entity as the new type. All metadata is reset. Protocol ID is reused(?)
*Metadata, where * is every entity type. Component and system definitions would be generated.	Contains the metadata (TrackedData) of a particular entity type. TrackedData is currently an enum, so this would save a lot of space. Not to mention easier because it's queryable and you wouldn't have to match on any enums.	Initializes the metadata. Would be nice if we allowed spawning entities without metadata to save memory.	Clears the metadata(?)	Updates the entity's metadata. Has no effect if component does not match the entity's type as determined by EntityKindComponent. When PlayerMetadata is modified on a client, the changes are sent to the client's own player entity.
ExpOrbCount	The amount of experience in an exp orb entity. Changes its visuals.			Only used when spawning exp orb entities. Modifications have no effect afterwards.
FallingBlockState	The BlockState of falling block entities			Only used when spawning falling block entities. Modifications have no effect afterwards.
Statuses	Bitset containing a bit for each entity status. Cleared at the end of the tick.	Triggers any set statuses for an entity.		Triggers any set statuses for an entity.
Animations	Bitset containing a bit for each entity animation. Cleared at the end of the tick.	Triggers any set animations for an entity.		Triggers any set animations for an entity.
Hitbox	Contains the AABB hitbox for an entity. Updated at the end of each tick.			No effect, but likely to be overwritten at the end of the tick.

Questions

• How granular should the components be? Should there be Location/OldLocation and Position/OldPosition, or is that kind of thing too cumbersome?
• What about miscellaneous data in Client like is_hardcore, is_flat, etc?
• Should UUID be shared between clients and entities as a component? Should it not be a component because it's immutable?
• Should the protocol ID of entities be a component? This would be immutable too.
• Should metadata components be broken up further to match Minecraft's class hierarchy? That seems very challenging.
• Working with metadata internally when there are a million different component types might be challenging. Probably need to cache metadata initialization bytes in McEntity.
• What about the inventory stuff for clients?
• What about poses?
• Does McEntity need to exist? It's kind of an ugly name.
• What is the most effective way to disallow removing components? Log an error? Panic in debug mode?

[dyc3]

This will be great to have. Can we get an ecs rewrite label on this?

Location seems like a bad name because it can be easily confused with Position.

For block entities: I would like to have a BlockPos component in addition to the Position component. Position would be able to handle sub-block positions (eg. x=3.5) and BlockPos is just for integer coordinates (eg. x=3)

Another thing that would be nice is to have a better way to inject received packets and assert sent packets for unit tests, ideally without having a serialization pass both ways. #198 currently requires a round trip for injecting packets, and a round trip for asserting read packets. This might be possible if we had client packets come in and be processed via an EventReader and out via an EventWriter. Maybe we could have traits PacketStreamSource and PacketStreamSink

Edit: This is no longer relevant, we've made the changes to make unit tests work.

Could look like this?

erDiagram
    PacketStreamSource ||--|| Client : injects
    PacketStreamSink ||--|| Client : extracts

Loading

classDiagram
    class PacketStreamSource {
        +recv()
    }

    class PacketStreamSink {
        +send()
    }

    class MockPacketStreamSource {
        queue
        +recv()
    }

    class RealPacketStreamSource {
        socket
        +recv()
    }

    RealPacketStreamSource --|> PacketStreamSource
    MockPacketStreamSource --|> PacketStreamSource

    class MockPacketStreamSink {
        record
        +send()
    }

    class RealPacketStreamSink {
        socket
        +send()
    }

    RealPacketStreamSink --|> PacketStreamSink
    MockPacketStreamSink --|> PacketStreamSink

Loading

Not a comprehensive diagram by any means, but just brainstorming

---

Regarding some of your questions:

Should there be Location/OldLocation and Position/OldPosition, or is that kind of thing too cumbersome?

That sounds incredibly jank.

Working with metadata internally when there are a million different component types might be challenging. Probably need to cache metadata initialization bytes in McEntity.

We might be able to do something with traits: https://bevy-cheatbook.github.io/patterns/generic-systems.html#using-traits

What about the inventory stuff for clients?

The Inventory and OpenInventory kinda already implement this design philosophy, so we don't need to worry about that. Regarding the inventory-related properties on Client, maybe have a new component named ClientInventoryMeta or similar?

Does McEntity need to exist? It's kind of an ugly name.

Yes, definitely. It should denote that the Entity it's attached to is directly correlated to a Minecraft entity.

What is the most effective way to disallow removing components? Log an error? Panic in debug mode?

The most effective way would be to put an API layer over all the ecs stuff that doesn't allow that stuff. This seems incredibly impractical, but certainly would be effective.

Logging would probably be the preferred way to go about it. People get angry when their services shit themselves and die. Panics should be reserved for states that are absolutely and totally unrecoverable.

It might not be worth the effort at all. Consider that it may be significantly easier to just make it so that doing these things are undesirable compared to whatever is "best practice".

[rj00a]

Think I'm going to start working on this in earnest after #184 is merged.

Location seems like a bad name because it can be easily confused with Position.

Agree. Also I believe Minecraft has a concept of "location" which is an XYZ coordinate paired with a dimension (hence DeathLocation), so my use of location would be inconsistent with that. But I'm not sure what else to call it.

For block entities: I would like to have a BlockPos component in addition to the Position component. Position would be able to handle sub-block positions (eg. x=3.5) and BlockPos is just for integer coordinates (eg. x=3)

The position of a block entity would be known by its position in the Chunk data structure, so I don't think a BlockPos component is needed. It's useful if you need to know where a block entity is during a query, but it isn't anything that Valence needs to know about strictly speaking.

Another thing that would be nice is to have a better way to inject received packets and assert sent packets for unit tests, ideally without having a serialization pass both ways. #198 currently requires a round trip for injecting packets, and a round trip for asserting read packets. This might be possible if we had client packets come in and be processed via an EventReader and out via an EventWriter. Maybe we could have traits PacketStreamSource and PacketStreamSink

Originally clients were designed to work exactly like this. Clients had a pair of channels for sending and receiving packets. Encoding and decoding of those packets were handled elsewhere by a tokio task in another thread. Simple as that.

Problem is, this is measurably less efficient than the current approach for a few reasons:

• Using a channel (Or EventReader/EventWriter) means the packets need to be 'static. For instance, writing a packet that contains a &str requires us to copy the string into a heap allocated String before we can actually encode it. This isn't a problem for received packets because we need to put the data in a client event anyway, but for sent packets it makes a difference.
• Using channels means we need to buffer packets in a homogeneous list. We need to use an enum of every possible packet. The size of that enum is the size of the largest variant, so that's a lot of wasted space. A stream of bytes is much more compact. We still have packet enums, but those are ephemeral and only used during decoding.
• Impossible to cache clientbound packets. We always need to send actual packet objects and not just raw bytes that we know are valid packets. Packet caching is what really makes Valence fast.

Here is what I propose: Add a public constructor to Client that takes the byte channels to read from and write to. We could even take this a step further and wrap all of Valence's tokio and networking code in an optional Bevy plugin. Not only is this good for testing, but Valence can then be used as an integrated server for a singleplayer experience. Pretty cool.

We might be able to do something with traits: https://bevy-cheatbook.github.io/patterns/generic-systems.html#using-traits

I guess in theory we could make all of our internal systems that work on entities generic and register hundreds of monomorphized systems, but that's obviously not very practical. Even then I think there would have to be some major algorithm changes.

Combining EntityKindComponent and the metadata components into a single enum (as it exists currently) seems like the way to go. Best to use a marker component if you need to filter entity types while querying.

Yes, definitely. It should denote that the Entity it's attached to is directly correlated to a Minecraft entity.

How about the name ProtocolEntity?

Logging would probably be the preferred way to go about it. People get angry when their services shit themselves and die. Panics should be reserved for states that are absolutely and totally unrecoverable.

It might not be worth the effort at all. Consider that it may be significantly easier to just make it so that doing these things are undesirable compared to whatever is "best practice".

Panics in debug mode with warnings in release mode sound like the way to go. I don't think this would be very difficult to set up actually. All we need is a check_component_invariants system that runs right after the user stage. If Archetype Invariants become a thing we could use that.

[dyc3]

Also I believe Minecraft has a concept of "location" which is an XYZ coordinate paired with a dimension

Ah, I see. Location seems fine then.

How about the name ProtocolEntity?

I think that's less clear.