gdquest-demos / godot-3-beginner-2d-platformer

Learn to create a 2d platform game with the Godot game engine. This is a beginner programming tutorial.
MIT License
365 stars 96 forks source link

Add Player movement #10

Closed henriiquecampos closed 5 years ago

henriiquecampos commented 5 years ago

Alright, this is the approach I'm taking for the Player's movement.

Close #2

razcore-rad commented 5 years ago

OK, so I played around with this PR which is all the time I had to do so far and I have a few notes.

As a general note please try to follow our own guidelines: have 2 spaces between functions etc. I know it's easy to overlook but it makes stuff much more uniform and as we like: standardized :)

First on folder structure. It should be like we have in the metroidvania 2D project: we have the actual game folder and inside of it we have src. Within src we put all ,but only *.gd & *.tscn files. Next to src we have the assets folder. Try to replicate the folder structure in metroidvania 2D is what I'm saying really. So actors & utility folders should go in src.

For the code I'd go with a simple calculation based on what we need. I guess you're going for an "it matters how much time you press on the jump button" approach. If you press for a short amount of time it doesn't jump so high as if you'd press for longer time. With that in mind we can simplify the code in my view using a more maths oriented approach instead of a bunch of if statements:

func _unhandled_input(event: InputEvent) -> void:
    if not (event.is_action("move_left") or event.is_action("move_right") or event.is_action("jump")):
        return

    var is_jump_interrupted: = event.is_action_released("jump") and _linear_velocity.y < 0.0
    var direction: = get_direction()
    _linear_velocity.x = movement_speed * direction.x
    _linear_velocity.y = (
            jump_impulse * direction.y
            if direction.y != 0.0 else 0.0
            if is_jump_interrupted else _linear_velocity.y)

func get_direction() -> Vector2:
    var out: = Vector2(
            Input.get_action_strength("move_right") - Input.get_action_strength("move_left"),
            -Input.get_action_strength("jump")
    )
    out.y = out.y if is_on_floor() else 0.0
    return out

If statements generally increase complexity because of the branching factor. Taking this in account also prefer to use the ternary operator whenever possible instead of bare if statements. The advantage of the ternary operator is that it covers all branching options while the bare if statement gives you the option to omit branch paths. Not having all branch paths covered is a pretty sure way of overlooking stuff and creating bugs. For a small project like this it might not matter, but since it's a simple concept to learn and teach and it's pretty much a best practice, let's use it whenever we can.

Thanks!

henriiquecampos commented 5 years ago

As a general note please try to follow our own guidelines: have 2 spaces between functions etc. I know it's easy to overlook but it makes stuff much more uniform and as we like: standardized :)

Do you mean the Geometry2D.gd? It was copy-pasted from another project. I will actually remove it from this project in the final version, and use .png or .svg files instead, as mentioned in #1:

You can use debug scripts or anything for yourself, but please don't add any extra autoload or utilities to the final game.

For the folders structure, I'd rather have every file the closest to related files they can be. I will throw the current ones in res://src as suggested, but further, if they are meant to be used together, they can go in the same folder. e.g.: body.png, Player.gd, Player.tscn, jump.wav can all go in res://src/actors/player/

With that in mind we can simplify the code in my view using a more maths oriented approach instead of a bunch of if statements

I was aware of that approach, but it seems very "overengineered", besides simple. It involves concepts that, for the matter of what was presented in #1 the audience is not familiar with.

For the sake of scope purposes, we can leverage the knowledge on if states, instead of introducing new concepts. What I mean is: we can show what they can do with what they already know, instead of teaching new techniques and making the ones they already have feel like insufficient.

If statements generally increase complexity because of the branching factor. Taking this into account also prefer to use the ternary operator whenever possible instead of bare if statements.

Same as above, for educational purposes, I think is better to cut as many "new concepts" as we can, and just focus on "with your toolkit, you can already do this". This was the premise I was taking, correct me if I'm wrong, but the project aims to put in practice the concepts presented in #1 synthesizing them into a simple game project, instead of building new ones on top of them.

That said, is there a way to implement what you proposed leveraging on the viewers' knowledge? I agree the if statements look ugly and are bug prone, we should work on that.

henriiquecampos commented 5 years ago

Also, could you add the reviews to the files they refer to? It makes easier to understand the context :sweat_smile:

NathanLovato commented 5 years ago

We've talked a lot about code education with Razvan, but also Lars, Chris, and others. You can trust Razvan to not suggest changes for the sake of complicating them but to teach good programming practices. We have already talked a lot about the way we wanted to teach and why. Razvan's our lead dev (true for me as well). For the sake of keeping work smooth and efficient - writing long posts like these is time-consuming - I'd like you to follow his lead. t Which of course doesn't mean you can't ask for explanations or we can't talk about code education at length, especially on Discord - I created a channel just for that, discussion's always welcome - but we've really spent a lot of time thinking, debating, and taking decisions about what we want to teach and how. GDQuest's moto is "become a better game developer" and I've always promised to teach the techniques professionals use to create games, or best practices, even and especially to beginners.

Below are some longer explanations for Razvan's suggestions.

but it seems very "overengineered", besides simple.

It's uncommon and uncomfortable at first glance because most teaching resources go for the easy if/elif/else route. Most common = most comfortable.

We should recommend using simple calculations over conditionals overall. Showing people to use lots of conditions leads to having them reproduce that pattern, producing nested blocks of code that become more and more specific. I can see now how there were problems I didn't know how to solve with my code in the past, and I've been able to greatly simplify code, e.g. in power sequencer, by replacing conditionals with general calculations.

More importantly, conditions are a go-to tool for programming beginners, as you've certainly noticed, and our role is to help them grow past what they already know. Even and especially if it means we have to challenge them a little.

Don't worry about the difficulty, it can be easily broken down and explained. I'd indent the code this way as well, which is a little easier to the eye to me. Also, what matters here is the principle more so than the code.

    var is_jump_interrupted: = event.is_action_released("jump") and _linear_velocity.y < 0.0

    var direction: = get_direction()

    _linear_velocity.x = movement_speed * direction.x
    _linear_velocity.y = (
        jump_impulse * direction.y
        if direction.y != 0.0 else 0.0
        if is_jump_interrupted else _linear_velocity.y
        )

For the folders structure, I'd rather have every file the closest to related files they can be.

We have a folder structure that works well for our projects and for consistency I would like us to keep working that way.

The rationale is that we use the same structure across projects so that it's consistent for the learners, and because we have something that generally works with projects of growing sizes.

The longer explanation for this choice: you can organize files however you want for small projects and it'll work but separating the source code and the art assets are necessary as soon as you need to reuse assets. And even if you didn't need to do that, it scales well when you're working in a growing team. That's why it's the go-to in many Unity or Unreal projects (also true of many game codebases) you see that kind of structure where the audio, the art assets, the shaders, and the code are separate. And of course, if you're creating separate reusable modules or plugins, each can have this kind of structure of its own.