Skip to content

Add position types #31

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

Merged
merged 2 commits into from
Mar 4, 2015
Merged

Add position types #31

merged 2 commits into from
Mar 4, 2015

Conversation

fenhl
Copy link
Member

@fenhl fenhl commented Mar 4, 2015

Do not merge yet: this could use some tests.

Adds a types::pos module with Protocol impls for [T; 3] where T: Protocol, meant to represent 3D coordinates (block positions as well as exact ones). Additionally, a BlockPos type encodes [i32; 3] positions as the Position protocol type.

@fenhl fenhl added this to the MC 1.8.3 Protocol milestone Mar 4, 2015
@toqueteos
Copy link
Contributor

On the phone. proto_len for [i32; 3] is 12 not 8.

@fenhl
Copy link
Member Author

fenhl commented Mar 4, 2015

@toqueteos: I'm assuming you're referring to BlockPos::proto_len? That is actually 8, since this is the “26 bits of x, 12 bits of y, 26 bits of z” type.

@toqueteos
Copy link
Contributor

@fenhl Yup, that's right. Android's GitHub app isn't that great for reviewinh, my bad. I'll clean up comments when I get home.

Seems good to merge but I'm worried about fixed point positions and those triplets multiplied by random values (gaussian, 8, ...)

@fenhl
Copy link
Member Author

fenhl commented Mar 4, 2015

We can always add Mul impls later if that's what you're talking about.

@toqueteos
Copy link
Contributor

@fenhl Those are encoding operations.
Example: Sound Effect (0x29) Encode does * 8, Decode does / 8.
Could this be automatic so we never forget to do it? We can always add some wrappers for read & write I guess...

@fenhl
Copy link
Member Author

fenhl commented Mar 4, 2015

ah, good point. Those are the fixed-point number types. We will have to implement all of these manually until integer-parameterized types land.

@toqueteos
Copy link
Contributor

Ready to merge.

@fenhl
Copy link
Member Author

fenhl commented Mar 4, 2015

I suppose we can add tests later.

fenhl added a commit that referenced this pull request Mar 4, 2015
@fenhl fenhl merged commit cd993f6 into PistonDevelopers:master Mar 4, 2015
@fenhl fenhl deleted the pos branch March 4, 2015 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants