introduce Position structs for the Chunk impl #284
Conversation
|
(currently can't do runtime tests) |
# Conflicts: # src/lib/world/src/edits.rs # Conflicts: # src/bin/src/main.rs # src/lib/net/src/conn_init/login.rs
# Conflicts: # src/bin/src/main.rs
|
i fixed (most of) the bugs with @NanCunChild |
|
;) remains a few bugs, cuz it is bug nest and hard to figure out what exactly happens. |
not quite @Mcrtin @NanCunChild
|
|
Nvm that was from an old world save, regenerated the world and it looks ok now. Will properly review the code tomorrow. |
ReCore-sys
left a comment
ReCore-sys
left a comment
There was a problem hiding this comment.
Looks fine, just a couple questions about some choices made.
| #[derive(Clone, Copy)] | ||
| pub struct BlockPos { | ||
| /// (i26, i12, i26) | ||
| pub pos: IVec3, |
There was a problem hiding this comment.
Would using the simd version be possible? Might allow for cleaner conversions from/to player positions
There was a problem hiding this comment.
there is a simd version?
|
|
||
| impl ChunkPos { | ||
| pub const fn new(x: i32, z: i32) -> Self { | ||
| assert!(x < 1 << 22); |
There was a problem hiding this comment.
What are these bitshifts doing?
There was a problem hiding this comment.
Regarding the bitshifts (1 << 22), I believe this aligns with the previous discussion about using 22-bit widths for world coordinates to enforce boundaries and optimize storage.
There was a problem hiding this comment.
a Network position may only be at most 64 bit (divided between xyz) where x is at most 24 bit iirc so the chunk x pos may only be 22 bit. this is important for the pack() feature ChunkPos provides where we assume these bounds.
|
|
||
| #[derive(Clone, Copy)] | ||
| pub struct BlockPos { | ||
| /// (i26, i12, i26) |
There was a problem hiding this comment.
Is using the packed format really worth it? I'd be worried about how that'd impact the CPU's native data type handling. This data type would largely be an intermediate bit of data so a few extra bytes won't make all that much of a difference.
There was a problem hiding this comment.
I agree with @ReCore-sys on the packed format concern. For BlockPos which is used heavily in logic calculations, the CPU overhead of unpacking might outweigh the memory savings. Maybe we should keep the packed format only for storage/networking structs, but use native types for internal logic?
There was a problem hiding this comment.
we aren't using the packed format... it just uses an ivec3 under the hood; the comment is just there to make the bounds clear, that we have to assure to be able to send it to the client via networkPosition
|
|
||
| impl ChunkColumnPos { | ||
| pub const fn new(x: u8, z: u8) -> Self { | ||
| assert!(x < 16); |
There was a problem hiding this comment.
In non-debug builds should we just truncate down to 0-15?
There was a problem hiding this comment.
i was wondering if we should but i want const evaluations to error at comp time and i couldn't really figure out a better way
3 participants
This is a preview of how the chunk impl could look like.
TODO:
-
current chunk gen tests aren't passing (so not that important xD)- i'd like to, instead of making changes on chunks, use chunk sections
-
net positions should get integrated into the newly added position structs- the position structs need docs more functions and overall a glowup
- Passing dims as strings sucks. (also, why do we not use &'static str instead of cloning?)
- why do we use arcs in thu chunk impl?
- why do we sanitize BlockStateId
- ...
for now this has to suffice.a
the actual changes include: