Load item and arrow entities from chunk save (#102)

[aramperes]

#102

• Load item NBT
• Load arrow NBT
• Spawn item and arrow into world
• Add tests

[codecov]

Codecov Report

Merging #125 into develop will decrease coverage by 0.2%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop     #125      +/-   ##
===========================================
- Coverage    58.76%   58.55%   -0.21%     
===========================================
  Files           60       61       +1     
  Lines         8230     8259      +29     
===========================================
  Hits          4836     4836              
- Misses        3394     3423      +29

Impacted Files	Coverage Δ
core/src/save/entity.rs	0% <0%> (ø)
core/src/save/region.rs	0% <0%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02dab41...e9d86c5. Read the comment docs.

[caelunshun]

What's your idea for spawning the loaded entities?

[aramperes]

I'm not decided yet, but here's what I'm thinking:

Since level deserialization is in feather-core, I figure the entities would be stored in the Chunk struct. When ChunkLoadSystem (in feather-server) receives a Reply with a loaded chunk, it fires a ChunkLoadEvent. There could be another system in feather-server that listens on ChunkLoadEvent and spawns the entities depending on their data.

However, consider the case where an an entity is stored in Chunk A. It is added to the world when ChunkLoadEvent is fired for Chunk A. The entity then crosses to Chunk B. Chunk A is later unloaded, and then loaded again. The entity would then be duplicated in the world. Not too sure how to prevent that.

[caelunshun]

I'm not decided yet, but here's what I'm thinking:

Since level deserialization is in feather-core, I figure the entities would be stored in the Chunk struct. When ChunkLoadSystem (in feather-server) receives a Reply with a loaded chunk, it fires a ChunkLoadEvent. There could be another system in feather-server that listens on ChunkLoadEvent and spawns the entities depending on their data.

However, consider the case where an an entity is stored in Chunk A. It is added to the world when ChunkLoadEvent is fired for Chunk A. The entity then crosses to Chunk B. Chunk A is later unloaded, and then loaded again. The entity would then be duplicated in the world. Not too sure how to prevent that.

Storing entities in Chunk isn't ideal in my opinion, since they aren't needed past when the chunk is loaded (all entities are stored in the World). It would probably be preferable to add a Vec<EntityData> field to chunkworker::Reply.

As for the entity duplication issue, I don't think there's a clean way around this until world persistence is implemented. I plan to do this within the next few weeks, so the issue shouldn't be present for that long. Might want to add a comment that notes this, though, so we don't forget.

[codecov-io]

Codecov Report

Merging #125 into develop will decrease coverage by 0.2%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop     #125      +/-   ##
===========================================
- Coverage    58.76%   58.55%   -0.21%     
===========================================
  Files           60       61       +1     
  Lines         8230     8259      +29     
===========================================
  Hits          4836     4836              
- Misses        3394     3423      +29

Impacted Files	Coverage Δ
core/src/save/entity.rs	0% <0%> (ø)
core/src/save/region.rs	0% <0%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02dab41...d7d5c05. Read the comment docs.

[codecov]

Codecov Report

Merging #125 into develop will decrease coverage by 0.09%.
The diff coverage is 47%.

@@            Coverage Diff             @@
##           develop     #125     +/-   ##
==========================================
- Coverage    58.76%   58.66%   -0.1%     
==========================================
  Files           60       61      +1     
  Lines         8230     8324     +94     
==========================================
+ Hits          4836     4883     +47     
- Misses        3394     3441     +47

Impacted Files	Coverage Δ
core/src/lib.rs	9.8% <ø> (ø)	⬆️
core/src/save/entity.rs	0% <0%> (ø)
server/src/chunkworker.rs	24.17% <0%> (ø)	⬆️
core/src/save/region.rs	0% <0%> (ø)	⬆️
server/src/entity/mod.rs	100% <100%> (ø)	⬆️
server/src/chunk_logic.rs	63.93% <60%> (-0.16%)	⬇️
server/src/entity/chunk.rs	97.94% <93.47%> (-1.39%)	⬇️
server/src/player/movement.rs	31.55% <0%> (+1.06%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02dab41...d790293. Read the comment docs.

[aramperes]

Would it make sense to add the Vec<EntityData> to ChunkLoadEvent, or firing a separate event? I'm thinking the code to spawn the entities should be its own system.

[caelunshun]

A separate system makes the most sense in this case, yes.

You should probably also use Util::spawn_xx for spawning the entities, come to think of it, since this will also handle sending them to clients.

[aramperes]

You should probably also use Util::spawn_xx for spawning the entities, come to think of it, since this will also handle sending them to clients.

Yeah that's what I'm doing atm, though it's fetching the Util resource lazily. Would that still be beneficial in a separate system, or should I just use Read<'a, Util> directly?

[caelunshun]

Still a separate system, I think, because the match expression will grow very long with more entities being added. Also, the behavior is separate from chunk loading itself, so using another system would be cleaner.

[aramperes]

Oh yeah I'm for sure separating it, I was just wondering if there'd be a purpose in using LazyUpdate in that new system, instead of referencing Util directly. I guess I'm not clear on what LazyUpdate actually does.

[caelunshun]

Oh, sorry for misunderstanding.

LazyUpdate::exec causes the provided closure to be execute lazily, i.e. at the end of the tick. Its benefit is that you can access any resource or component from the world without specifying system data, but you also lose concurrency by doing this.

If you use the separate system, there shouldn't be a need to use it. The main motivation for utilizing it is if you need access to tons of components which would be unclean to write in a SystemData. (In the issue, I wrote that LazyUpdate should be used, but this was before util::spawn_xx was implemented.)

[aramperes]

Hmm so if I add Vec<EntityData> to ChunkLoadEvent, I get an error since Vec doesn't implement the Copy trait. Does that mean I can't pass a Vec in an event?

[caelunshun]

You should just be able to remove the Copy derive on ChunkLoadEvent (unless there's some reason it's needed).

[aramperes]

Ready for review

[caelunshun]

Thanks!