-
-
Notifications
You must be signed in to change notification settings - Fork 207
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
[WIP] Add Multiplayer Example #905
Conversation
…w. Hoping that converted the rest to Rust will fix it
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-905 |
I just realized that instead of randomly choosing a spawn location, I could have just saved the spawn point the player originally came from, and then just make the player respawn there on death. Though, that wouldn’t be as fair… |
Thanks a lot for all your work on this, a little multiplayer demo sounds like an amazing addition 🙂 I'll give it more detailed review when out of draft, but some initial feedback already:
I haven't run it yet, is the game currently already playable? |
In order
Also, as mentioned earlier, this is playable right now! It’s just a little buggy for reasons I can’t quite grasp yet. I wrote up instructions in an earlier reply. |
Do you think you could merge the 2 managers to 1? Maybe I can then see if there's a way to architect it differently. Cool to hear it's playable! I don't know when I get around to test it out, but probably not today 🙂 |
Actually, I just realized that it’s totally doable. GameManager is essentially just a wrapper class around a hash map, and I could move that hashmap into SceneManager itself! |
Alright, did some cleanup, and GameManager is now removed! Horray! we just pass around the hashmap containing all the player data now. Only problem is, the connection is still really inconsistent. I think its because the test scene spawns at different times for all the different clients, which leads to desyncs. Not really sure how to solve this, will need to look into this more. |
Some more assorted thoughts: |
…dd callback for hitting bullet on player
@@ -13,7 +13,7 @@ members = [ | |||
"itest/rust", | |||
"itest/repo-tweak", | |||
"examples/dodge-the-creeps/rust", | |||
"examples/hot-reload/rust", | |||
"examples/hot-reload/rust", "examples/multiplayer-lan/rust", |
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.
you forget newline
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 left some comments. I'm not too experienced when it comes to networking with godot so I focused purely on godot-rust side of things. My main issue – why don't we just use one of godot's examples instead of frozen-in-time youtube video? Who is going to maintain this example and make sure it is up-to-date with latest changes and best practices?
|
||
const LOCALHOST: &str = "127.0.0.1"; | ||
const PORT: i32 = 8910; | ||
#[derive(GodotClass, Clone)] |
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.
Why does it derive a clone? Shouldn't it be passed around using Gd smart pointer?
Self { | ||
address: LOCALHOST.into(), | ||
port: PORT, | ||
game_scene: PackedScene::new_gd(), |
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.
… do we really want to create PackedScene::new_gd() instead of using an option? Not filling this property might lead to weird behaviours
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.
fixed this, you're right
} | ||
|
||
#[derive(GodotClass)] | ||
#[class(base=Control)] |
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.
does it need custom init really? Can't we use #[init(default = …]
?
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.
ill be honest, i dont know what you mean by this
address: GString, | ||
port: i32, | ||
#[export] | ||
game_scene: Gd<PackedScene>, |
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.
Why not option? What will happen if this property won't be filled? Why isn't it commented to inform user about your design decision?
|
||
#[godot_api] | ||
impl MultiplayerController { | ||
// called when a new "peer" gets connected with the server. Both client and server get notified about this |
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.
///
for docs.
|
||
/* | ||
#[func] | ||
fn respawn_player(&self, &mut player: Gd<Player>) |
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.
leftover/dead code
|
||
|
||
|
||
Largely adapted from FinePointCGI's Godot multiplayer tutorial: |
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.
why are we adapting youtube tutorial instead of one of godot's demo projects? https://github.com/godotengine/godot-demo-projects/tree/master/networking
ProjectSettings::singleton() | ||
.get_setting("physics/2d/default_gravity".into()) | ||
.try_to::<f64>(), | ||
"default setting in Godot", |
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.
…what does it error message do? What does it inform user about? "default setting in godot" informing about… not being able to cast given setting to float?
fn physics_process(&mut self, delta: f64) { | ||
// delete bullet once LIFETIME seconds have passed | ||
self.time_left -= delta; | ||
if self.time_left <= 0.0 { |
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 would rather use scene tree timers or at least SystemTime
} | ||
// have bullet fall down while flying | ||
if !self.base().is_on_floor() { | ||
self.base_mut().get_velocity().x += (self.gravity * 1. * delta) as f32; |
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.
get velocity returns Vector2 not &mut Vector2… https://godot-rust.github.io/docs/gdext/master/godot/classes/struct.CharacterBody2D.html#method.get_velocity. Have you ran this example?
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.
removed gravity since it seems unnecessary anyways
use crate::NetworkId; | ||
|
||
const LOCALHOST: &str = "127.0.0.1"; | ||
const PORT: i32 = 8910; |
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.
stupid question, but shouldn't it be unsigned int 32b?
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.
Port numbers are usually a u16
, e.g. std::net::SocketAddr::new
. In Godot it's passed as a string, though. Maybe it'd be nice if we changed that string for a SocketAddr
at one point.
CI/CD to run this example (same as we do with dodge the creeps) would be cool too I'll build against this example and check it later |
@@ -0,0 +1,57 @@ | |||
[gd_scene load_steps=2 format=3 uid="uid://btw6ethmr2246"] |
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.
Is this tmp file necessary to check in?
Stopping here for now. I took some advice from the server, and decided to start from scratch and port the multiplayer bomber example instead. However, I ran into the same problem I had when I was working on v1 of this example: namely, the signal/callback api. So, my current plan is to wait until a later version of gdext when that is more user-friendly to continue with this PR. If anyone still wants to see how using multiplayer with gdext would work, I would advice to look at this commit: 043db66 Good luck to anyone who tries to follow in my footsteps, here be dragons! |
Closing this PR in favor of #910, since it hews much closer to the official godot demo multiplayer project. |
Add multiplayer example!
heavily inspired by -> https://youtu.be/e0JLO_5UgQo?si=cauEVPrbe6ThtDTh
Fixes #903
pretty simple, but i do want to add some quality of life stuff
Also, we need to replace the old assets with better ones, since the ones I used have a weird license