-
-
Notifications
You must be signed in to change notification settings - Fork 67
Reworked chunk format #208
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some things i noticed
)); | ||
|
||
let old_chunk = (position.x as i32 >> 4, position.z as i32 >> 4); | ||
let old_chunk = IVec2::new(position.x as i32 >> 4, position.z as i32 >> 4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have been using the position of the chunkcolumn in the chunk that is closest to (-inf, -inf) for easier and faster conversion. also, i don't know if bitshifting on an i32 is rounding towards -inf or 0. if it rounds to 0, it is wrong (because the pos (-7, -9) and (5, 8) for example, would be rounded to (0, 0) even though they are in different chunks)
/// use bevy_math::IVec2; | ||
/// | ||
/// let chunk = Chunk::new(IVec2::new(0, 0), "minecraft:overworld".to_string()); | ||
pub fn new(pos: IVec2, dimension: String) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think having the dim in a chunk is usefull; if you really need the dim, you should be able to obtain it from the container of the chunks (and please make it an enum or at least make constants so you don't have to type out the strings). i also would like to have the pos in a wrapper over having it in the core of the chunk impl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I have the dim in the chunk is so that the database layer can handle saving to correct table without needing data from other places. And the reason I have the dim as a string is so custom dimensions can be a thing, tho I guess we could have it as an enum for the vanilla dims and have a Custom(String)
variant for custom dimensions.
Also what do you mean have the pos as a wrapper?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by the pos in a wrapper i mean having a wrapper struct like
struct PosChunk(ChunkPos, Chunk);
so you don't have to store the pos on each chunk if you don't need it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's going to create a lot of conflict with what types functions and such will expect. And since a lot of different things will also need the chunk's pos, we'd end up passing PosChunk around most of the time anyway. Is there a specific issue with having the position in the chunk impl?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its fine. does it also have store the height of the chunk? or how does it decide if it is 256 or 384 blocks high?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chunks are always -64 to 320 since 1.18 so it'll always be 384
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not in the nether?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. We could either store it if you'd like or infer it from the dimension? Up to you what you want to do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's the same either way because of custom dims
Our chunk format is kinda garbage. Not any more tho. Maybe.
Description
Motivation and Context
How has this been tested?
Screenshots (if appropriate):
Types of changes
Checklist: