Client Component Division

[rj00a]

Description

Divides the Client component into a set of smaller components as described by #199 (with many deviations). McEntity will be dealt with in a future PR.

• Divide Client into smaller components (There's a lot to look at).
• Rename McEntity to Actor to avoid confusion between ECS entities and Minecraft entities.
• Move common components to component module.
• Remove Username type from valence_protocol because the added complexity wasn't adding much benefit.
• Clean up the inventory module.

I've stopped worrying about the "Effect When Added" and "Effect When Removed" behavior of components so much, and instead assume that all components of a particular thing are required unless otherwise stated.

Blocked on:

• Update to Bevy 0.10 #264

Test Plan

Steps:

1. Run examples and tests.

A large number of tweaks have been made to the inventory module. I tried to preserve semantics but I could have made a mistake there.

[dyc3]

I very much don't like renaming McEntity to Actor. I feel like the confusion between Entity and McEntity can be very easily remedied with proper doc comments. Do we really get questions/see confusion about it often?

[qualterz]

I very much don't like renaming McEntity to Actor.

Yeah, Actor may sound too abstract but on the other hand McEntity too verbose.

[qualterz]

Maybe confusions should be avoided by leveraging namespaces like mc::Entity and ecs::Entity?

[dyc3]

McEntity is not too verbose. Using namespaces like that almost certainly makes the problem worse.

[AviiNL]

In my humble opinion, the valence project interfaces at least for a user to minecraft first, bevy last, so i would probably use Entity for a minecraft entity, and re-export the bevy entity to something like BevyEntity. Or have them both as Entity and leave it up to the user to qualify it, either with an as keyword in the use statement, or qualify in-line where needed.. point being, both are entities, so why not just call them that?

[qualterz]

Also, the invention of new terms would be better to avoid, because it's additional cognitive load for newcomers.

[rj00a]

I'll just revert the McEntity -> Actor change.

In my humble opinion, the valence project interfaces at least for a user to minecraft first, bevy last, so i would probably use Entity for a minecraft entity, and re-export the bevy entity to something like BevyEntity.

Not sure that's feasible with the way imports/exports work in Rust. We're re-exporting bevy_app and bevy_ecs at the root of valence, so the BevyEntity name would only be available in our prelude? But then we wouldn't be able re-export the bevy prelude in our prelude easily without something like Haskell's hiding.

Or have them both as Entity and leave it up to the user to qualify it

That would make using valence::prelude unpleasant IMO.

[dyc3]

I'm getting some ridiculous rubber banding when running all of the examples. The packet inspector shows really big client bound packets at regular intervals. I think that chunks keep getting sent to the client when they aren't needed? Not really sure

[rj00a]

I'm getting some ridiculous rubber banding when running all of the examples.

This is caused by modifications to player position, yaw, pitch, in the packet handler triggering change detection for those components. A system later picks up on those changes and sends a teleport packet. We aren't distinguishing changes made by clients and changes made by users.

Some possible solutions:

1. Bypass change detection in the packet handler. To detect movement you need to listen for player move events instead. I'm not happy with this because:
   • Potential footgun when using Changed<Position> in user code.
   • A lot of systems that listen for movement likely only care about the final position of an entity or client and not any of the intermediate steps that tick. Emulating that with events is painful.
   • Position components on entities and position components on players cannot be treated uniformly in some situations.
2. Store a boolean in the Position, Yaw, Pitch components indicating when a change is made by users. Guard the inner value with a setter. This works, but now you've got useless booleans on every entity when it's only needed on clients. The boolean gets included in your queries even if you don't need it, degrading query performance.
3. Store the client's desired position and direction in separate components from its real position and direction. Compare real and desired at the end of the tick. If they don't match, then desired := real and a teleport occurs.

Option 3 seems like the best but maybe there's a better way. What do you think?

[dyc3]

I concur, I think option 3 seems best.