Skip to content

add Squash The Creeps tutorial #4

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

FrankCasanova
Copy link

image


#[godot_api]
impl INode for MainScene {
fn init(base: Base<Node>) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO auto-derrived init (i.e #[class(init, base=Node)] should be preferred

Copy link
Contributor

Choose a reason for hiding this comment

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

Reasoning – it simplifies all the boilerplate, and allows one to initialize&add new fields without bunch of refactors. It makes easier to spot default values as well

fn ready(&mut self) {
self.to_gd();

//$UserInterface/Retry.hide()
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, add a space between // and actual comment text 😅

mob.bind_mut()
.initialize(mob_spawn_location.get_position(), player_position);
//mob.squashed.connect($UserInterface/ScoreLabel._on_mob_squashed.bind())
mob.connect(
Copy link
Contributor

Choose a reason for hiding this comment

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

we can use typed signals here 🤔.

Comment on lines 8 to 16
#[derive(GodotClass)]
#[class(base=CharacterBody3D)]
pub struct Mob {
//Minimum speed of the mob in meters per second.
min_speed: f32,
//Maximum speed of the mob in meters per second.
max_speed: f32,
base: Base<CharacterBody3D>,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

#[derive(GodotClass)]
#[class(init, base=CharacterBody3D)]
pub struct Mob {
    /// Minimum speed of the mob in meters per second.
    #[export]
    min_speed: f32,
    /// Maximum speed of the mob in meters per second.
    #[export]
    max_speed: f32,

    base: Base<CharacterBody3D>,
}

Comment on lines 9 to 29
#[derive(GodotClass)]
#[class(base=CharacterBody3D)]
pub struct Player {
//How fast the player moves in meters per second.
speed: f32,
//Vertical impulse applied to the character upon jumping in meters per second.
jump_impulse: f32,
//Vertical impulse applied to the character upon bouncing over a mob in meters per second.
bounce_impulse: f32,
//The downward acceleration when in the air, in meters per second.
fall_acceleration: f32,
//The target velocity of the character (node property)
target_velocity: Vector3,

base: Base<CharacterBody3D>,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#[derive(GodotClass)]
#[class(base=CharacterBody3D)]
pub struct Player {
//How fast the player moves in meters per second.
speed: f32,
//Vertical impulse applied to the character upon jumping in meters per second.
jump_impulse: f32,
//Vertical impulse applied to the character upon bouncing over a mob in meters per second.
bounce_impulse: f32,
//The downward acceleration when in the air, in meters per second.
fall_acceleration: f32,
//The target velocity of the character (node property)
target_velocity: Vector3,
base: Base<CharacterBody3D>,
}
#[derive(GodotClass)]
#[class(init, base=CharacterBody3D)]
pub struct Player {
/// How fast the player moves in meters per second.
#[export]
speed: f32,
/// Vertical impulse applied to the character upon jumping in meters per second.
#[export]
jump_impulse: f32,
/// Vertical impulse applied to the character upon bouncing over a mob in meters per second.
#[export]
bounce_impulse: f32,
/// The downward acceleration when in the air, in meters per second.
#[export]
fall_acceleration: f32,
/// The target velocity of the character (node property)
#[export]
target_velocity: Vector3,
base: Base<CharacterBody3D>,
}

//var collision = get_slide_collision(index)
let collision = self.base_mut().get_slide_collision(index).unwrap();
//if collision.get_collider().is_in_group("mob"):
if let Some(collider) = collision.get_collider() {
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use let Some(collider) = collision.get_collider() else { continue }; here (and bellow as well)

@@ -0,0 +1,98 @@
There are some guidelines you can rely on (which you probably already know):
Copy link
Contributor

Choose a reason for hiding this comment

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

(which you probably already know) doesn't add something here – it is just noise. I would advise to remove it

Shouldn't this file be .md instead of .txt?

Both the godot documentation and the gdext-rust bindings specify which are const and which aren't, the gdext bindings specify that through method signatures (eg: fn get_position(&Node2D) vs fn add_child(&mut Node)).

Non-const methods
Calling can be done with either <&mut MyClass>::base_mut().method() or <&mut Gd<MyClass>::method().
Copy link
Contributor

@Yarwin Yarwin Mar 27, 2025

Choose a reason for hiding this comment

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

Calling what (built-in, inherited methods)?

I would use snippets instead of mental shortcuts there (and further on), for example:

let mut obj: Gd<MyClass> = MyClass.new_alloc();

// To call method with &self receiver 
obj.bind().method();

// To call method with &mut self receiver 
obj.bind_mut().method();

<Type> has some specific meaning in rust, so it is misleading. (it can be used as <T>::method(&mut receiver... args);)

Let's analyze this snippet from your player.rs:

```rust
if let Some(collider) = collision.get_collider() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I made a comment earlier, but let Some(...) else {continue }; should be preferred in such cases

if let Some(node) = collider.try_cast::<Node3D>().ok() {
if node.is_in_group("mob") {
//var mob = collision.get_collider()
let mut mob = collision.get_collider().unwrap().cast::<Mob>();
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a case in which collider can be null 🤔? I would use expect(some reasoning) instead of unwrap here 🤔 .

@Yarwin
Copy link
Contributor

Yarwin commented Mar 27, 2025

I'll give the proper look (and A/B testing with both squash the creeps) tomorrow; in meanwhile make sure to deal with clippy errors https://github.com/godot-rust/demo-projects/actions/runs/14109802627/job/39525244314?pr=4

EDIT: Done, control scheme is weird, gdextension file is wrongly configured, exports are missing (ofc), outside of that it is fine

@Yarwin
Copy link
Contributor

Yarwin commented Mar 29, 2025

Also add "squash-the-creeps/rust" to members in our "main" Cargo.toml.

@Bromeon Bromeon added the new-demo New demo or content added label Mar 29, 2025
@Bromeon Bromeon linked an issue Mar 29, 2025 that may be closed by this pull request
@FrankCasanova
Copy link
Author

@Yarwin take a look at it when u have time and feel like it

Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Thanks a lot so far!

I'm not sure if writing the GDScript code on top of each Rust line is helpful. In fact, it becomes distracting pretty quickly; the need to use base() etc. becomes apparent quite faster after the first few times. A translation table / cheatsheet in the book would be more expedient in that regard.

If you absolutely want to do this, then please limit it to cases where GDScript and Rust are vastly different and it's very hard to find out the Rust code from the GDScript one. And please prefix the comment with // GDScript: <code>, so it's clear what it means. But avoid it, whenever reading the Rust code makes obvious how the GDScript code could have looked.

@@ -131,4 +131,5 @@ grow_horizontal = 2
grow_vertical = 2
text = "Press enter retry"

[connection signal="hit" from="Player" to="." method="on_player_hit"]
Copy link
Member

Choose a reason for hiding this comment

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

Please define all signal connections in Rust code.

Some may still need the "manual" API but it's much easier to migrate that, than some scene files. They're also less brittle -- scene connections tend to be messed up when loading with a wrong Godot version.

Copy link
Author

Choose a reason for hiding this comment

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

thanks a lot for those guidelines, and about this, i dont know if i know how to do it exactly (im noob), but anyway, if i dont know tomorrow ill ask

Comment on lines 1 to 14
#!/bin/sh
# Copyright (c) godot-rust; Bromeon and contributors.
# This Source Code Form is subject to the terms of the Mozilla Public
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at https://mozilla.org/MPL/2.0/.

# Must be in dodge-the-creep's rust directory in order to pick up the .cargo/config
cd `dirname "$0"`

# We build the host gdextension first so that the godot editor doesn't complain.
cargo +nightly build --package dodge-the-creeps &&
cargo +nightly build --package dodge-the-creeps \
--features godot/experimental-wasm,godot/lazy-function-tables \
--target wasm32-unknown-emscripten -Zbuild-std $@
Copy link
Member

Choose a reason for hiding this comment

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

This is building the wrong package.

mod scorelabel;
use godot::prelude::*;

struct Scripts;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
struct Scripts;
struct SquashTheCreeps;

Comment on lines 102 to 84
// We apply gravity every frame so the character always collides with the ground when moving.
// This is necessary for the is_on_floor() function to work as a body can always detect
// the floor, walls, etc. when a collision happens the same frame.
if !self.base().is_on_floor() {
// velocity.y -= fall_acceleration * delta
self.target_velocity.y -= self.fall_acceleration * _delta as f32;
}
// moving the Character
let velocity = self.target_velocity;
self.base_mut().set_velocity(velocity);
// move_and_slide()
self.base_mut().move_and_slide();
Copy link
Member

Choose a reason for hiding this comment

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

General style guide:

  • Comments should whenever possible be proper sentences. Even keywords should ideally start with uppercase and end with period.
  • Separate groups of lines that belong together with an empty line, so they're visually more recognizable.
  • As mentioned, avoid obvious GDScript comments.

The first comment here is perfect, here's a suggestion for the remaining parts:

Suggested change
// We apply gravity every frame so the character always collides with the ground when moving.
// This is necessary for the is_on_floor() function to work as a body can always detect
// the floor, walls, etc. when a collision happens the same frame.
if !self.base().is_on_floor() {
// velocity.y -= fall_acceleration * delta
self.target_velocity.y -= self.fall_acceleration * _delta as f32;
}
// moving the Character
let velocity = self.target_velocity;
self.base_mut().set_velocity(velocity);
// move_and_slide()
self.base_mut().move_and_slide();
// We apply gravity every frame so the character always collides with the ground when moving.
// This is necessary for the is_on_floor() function to work as a body can always detect
// the floor, walls, etc. when a collision happens the same frame.
if !self.base().is_on_floor() {
// velocity.y -= fall_acceleration * delta
self.target_velocity.y -= self.fall_acceleration * _delta as f32;
}
// Move the character.
let velocity = self.target_velocity;
self.base_mut().set_velocity(velocity);
self.base_mut().move_and_slide();

In this particular case, it's also possible to store base_mut() in a variable, to reuse the guard and avoid repeated locking. Although it's not that important for a basic example.

let base = self.base_mut();
if base.is_on_floor() ... {

}
...

base.set_velocity()
base.move_and_slide()

@FrankCasanova FrankCasanova requested a review from Yarwin June 9, 2025 21:33
@FrankCasanova FrankCasanova requested a review from Yarwin June 10, 2025 12:18
@Yarwin Yarwin added this pull request to the merge queue Jun 14, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 14, 2025
@Bromeon
Copy link
Member

Bromeon commented Jun 14, 2025

Ci failed on macos-arm with:

libc++abi: Pure virtual function called!
./.github/other/check-example.sh: line 38:  9109 Abort trap: 6           timeout "$EDITOR_TIMEOUT"s "$GODOT4_BIN" -e --headless --path "$dir" --quit
Error: # DEMO hot-reload | Godot editor failed to open in time.
Error: Process completed with exit code 1.

Very likely unrelated to this PR, our nightly CI hit the same error a few days ago. I asked about it on RocketChat, but seems like no one has seen this. I wonder if it's a problem or in Godot -- but it seems to happen only since recently, indicating it's a recent Godot in-dev version.

Retrying might work, but we should probably sooner or later address this properly 🙂

@Yarwin
Copy link
Contributor

Yarwin commented Jun 14, 2025

@Bromeon we encountered it earlier actually: https://github.com/godot-rust/demo-projects/actions/runs/15544817340/job/43763899820 but I discarded it as an artifact of github-related issues 🤔. It seems to be "random".

I don't have an access to MacOS device as well, thus I'm similarly limited when it comes to doing any debugging >:[

@Bromeon
Copy link
Member

Bromeon commented Jun 14, 2025

With these issues I'm very hesitant to spend much time debugging. Especially without macOS device, it's a big effort and tiny reward.

Maybe we should just use retry.sh and move on with life... (outside this PR)

@TitanNano
Copy link

@Bromeon @Yarwin I can take a look at it on my local machine. As I understand, this is something that just sporadically occurs in CI, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-demo New demo or content added
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Squash The Creep Tutorial
4 participants