Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement server-side messages #14

Merged
merged 2 commits into from
Mar 30, 2016
Merged

Implement server-side messages #14

merged 2 commits into from
Mar 30, 2016

Conversation

nabijaczleweli
Copy link
Member

This change is Reviewable

@nabijaczleweli
Add authoring notice to server/README
ec6f39a
@nabijaczleweli
Implement Message (single-enum)
63eabdd 
Add relevant section of Protocol spec to message mod and variant docs

Depend on serde{,_json}. Dev-depend on rand

Credit Katt in Cargo.toml
@nabijaczleweli nabijaczleweli added this to the MVP milestone Mar 28, 2016
@nabijaczleweli
Copy link
Member Author

Still not sure what to do with world_state without official Player and Bullet types

@TheCatPlusPlus
Copy link
Member

Reviewed 4 of 5 files at r1.
Review status: 4 of 5 files reviewed at latest revision, 3 unresolved discussions.

server/src/message.rs, line 302 [r1] (raw file):
Avoid unwraps. This doesn't have to be inside to_string.

server/src/message.rs, line 308 [r1] (raw file):
Serde seems to support #[derive], not sure if all the manual fiddling is necessary.

server/src/message.rs, line 710 [r1] (raw file):
This &*& is really silly and unnecessary :v &json_txt should be enough, &json_txt[..] if compiler needs help with inference.

Comments from the review on Reviewable.io

@TheCatPlusPlus
Copy link
Member

Still not sure what to do with world_state without official Player and Bullet types

They're defined in the spec.

@nabijaczleweli
Copy link
Member Author

Review status: 4 of 5 files reviewed at latest revision, 3 unresolved discussions.

server/src/message.rs, line 302 [r1] (raw file):
to_stringing serde_json::Value cannot fail, though.
As for being inside to_string, see L308 discussion.

server/src/message.rs, line 308 [r1] (raw file):
I was thinking about abstracting serde[_json] out of this entirely, and Rust stdlib provides all the right tools.

Comments from the review on Reviewable.io

@nabijaczleweli
Copy link
Member Author

Review status: 4 of 5 files reviewed at latest revision, 3 unresolved discussions.

server/src/message.rs, line 710 [r1] (raw file):
Done.

Comments from the review on Reviewable.io

@TheCatPlusPlus
Copy link
Member

Review status: 4 of 5 files reviewed at latest revision, 2 unresolved discussions.

server/src/message.rs, line 302 [r1] (raw file):
Well if it's guaranteed to not fail then fine. Maybe add a note.

server/src/message.rs, line 308 [r1] (raw file):
Serde already is an abstraction, I don't think it's worth to be able to switch serialisation frameworks (as long as this one is reasonably well-written; if not, then let's switch immediately). It's better to use Serialize/Deserialize and their annotations to control behaviour instead of ToString/FromStr.

Comments from the review on Reviewable.io

@nabijaczleweli
Copy link
Member Author

Review status: 4 of 5 files reviewed at latest revision, 1 unresolved discussion.

server/src/message.rs, line 308 [r1] (raw file):
Nota Bene: if we're supporting Rust Stable, then we don't have custom_derive, which means we don't benefit from automatic de-/serialization macros.

Comments from the review on Reviewable.io

@TheCatPlusPlus
Copy link
Member

Review status: 4 of 5 files reviewed at latest revision, 1 unresolved discussion.

server/src/message.rs, line 308 [r1] (raw file):
https://github.com/serde-rs/serde#using-serde-with-stable-rust-syntex-and-serde_codegen (custom_derive is only syntax sugar anyway, the real necessary feature is compiler plugins)

Comments from the review on Reviewable.io

@TheCatPlusPlus
Copy link
Member

Review status: 4 of 5 files reviewed at latest revision, 2 unresolved discussions.

server/src/message.rs, line 308 [r1] (raw file):
We're leaving that for now.

server/src/message.rs, line 203 [r2] (raw file):
Marking todo.

Comments from the review on Reviewable.io

@TheCatPlusPlus
Copy link
Member

Reviewed 3 of 4 files at r3.
Review status: 6 of 7 files reviewed at latest revision, 1 unresolved discussion.

server/src/message/mod.rs, line 210 [r3] (raw file):
Marking todo.

Comments from the review on Reviewable.io

@thecoshman
Copy link
Contributor

Are we going to go for squashing commits down before PR is finished?

@TheCatPlusPlus
Copy link
Member

Might fuck up review status so we'll see after it's all reviewed.

thecoshman
thecoshman reviewed Mar 29, 2016
View reviewed changes
server/src/message/mod.rs
/// - `move_x` (f32) — current movement vector X of the bullet
/// - `move_y` (f32) — current movement direction vector Y of the bullet
/// (movement direction vectors MUST be normalised, i.e. their magnitude MUST be equal to 1)
WorldState,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slacker

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm doing it

@nabijaczleweli
Copy link
Member Author

Review status: 4 of 8 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

server/src/message/mod.rs, line 210 [r3] (raw file):
Done.

Comments from the review on Reviewable.io

@nabijaczleweli
Copy link
Member Author

Not sure why Travis is failing, looks like clang-3.8 package magically stopped containing Clang binaries?

@nabijaczleweli nabijaczleweli force-pushed the наб/message branch 4 times, most recently from af20160 to 63eabdd Compare March 29, 2016 22:37
@TheCatPlusPlus
Copy link
Member

Seriously don't rebase shit before review is over, or it's hard to tell what was already read and what wasn't. (And impossible to tell what actually changed from iteration to iteration)

Okay I can actually get normal diffs, but don't anyway, it can be done afterwards.

@TheCatPlusPlus
Copy link
Member

👍

Reviewed 1 of 3 files at r4, 3 of 3 files at r5.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

Comments from the review on Reviewable.io

@thecoshman
Copy link
Contributor

👍

@nabijaczleweli
Copy link
Member Author

2 upboats, merging

@nabijaczleweli nabijaczleweli merged commit 3241b1b into master Mar 30, 2016
@nabijaczleweli nabijaczleweli deleted the наб/message branch March 30, 2016 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
feature server
Projects
None yet
Milestone
MVP
Development

Successfully merging this pull request may close these issues.
4 participants
@nabijaczleweli @TheCatPlusPlus @thecoshman @andyprowl