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.

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

Remake the 2d platformer demo #405

Merged

Conversation

NathanLovato
Copy link
Contributor

This Godot project is a complete rewrite of the official Platformer 2D demo. This is part of a proposal to improve and harmonize the official demo projects. We want to:

  • Teach good programming practices with Godot.
  • Bring demos to a higher quality standard.
  • Keep unifying the code in the demos, a big task that @aaronfranke has been working on lately.

See the open issue for more information.

This new demo showcases features from the original and works with Godot 3.2.
This is the same demo from https://github.com/GDQuest/godot-demo-projects-remake, coded by @johnnygossdev, marked as a co-author of the commit, first reviewed by me, and then by @aaronfranke.

We hope you'll like it!

This Godot project is a complete rewrite of the official Platformer 2D demo. This is part of a proposal to improve and harmonize the official demo projects. We want to:

- Teach good programming practices with Godot.
- Bring demos to a higher quality standard.
- Unify the code in the demos.

See the [open issue](godotengine#390) for more information.

This new demo showcases features from the original, and works with Godot 3.2.

Co-authored-by: Johnny Goss <me@johnnygoss.dev>
@NathanLovato NathanLovato changed the title Remake the 2d platformer demo from scratch Remake the 2d platformer demo Feb 5, 2020
@NathanLovato
Copy link
Contributor Author

I can write a README for the demo if you'd like, I just need to know if there's a format or anything, in particular, that you need there. The source code contains quite a lot of comments for learning purposes.

Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

Enum style should be discussed: godotengine/godot-docs#3154

Can you add support for the left stick for movement on controllers?

I noticed that the background fails fairly quickly when you change the project's resolution, both when making it wider and making it taller. The background should be extended or somehow grow.

Screenshot from 2020-02-09 03-44-58

When it comes to a README file, we don't have a hard template yet. I have a (fairly empty) discussion here. I just added a comment with some examples of README files. I would like to get some README files into the repo that work well for those particular demos, then we can figure out what works generally and make a standardized template. So for now, write what you think works best. Sounds good?

2d/platformer/project.godot Outdated Show resolved Hide resolved


export var speed = Vector2(400.0, 500.0)
export var gravity = 3500.0
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to instead set the gravity in the project settings and then use onready var gravity = ProjectSettings.get_setting("physics/2d/default_gravity"). This way, the gravity can be changed for the entire project at once, and would also include 2D physics objects like rigidbodies.

Right now, this value is overridden to be 1800 for both Player and Enemy, so it would be easy for a user to accidentally change it here in the script and have it do nothing, or they would accidentally change it for one of them and have inconsistent gravity between the Player and Enemies.

2d/platformer/src/UserInterface/PauseMenu.gd Outdated Show resolved Hide resolved
@NathanLovato
Copy link
Contributor Author

Thanks, somehow I didn't get or missed the notification! I'll address the requested changes ASAP.

Remove unused spawn input action
Add support for the gamepad's left stick for movement
Use the project settings to store the gravity value project-wide
Remove type hints
@NathanLovato
Copy link
Contributor Author

NathanLovato commented Feb 11, 2020

I've just addressed everything except for the background for now. I'm trying to get the game window to stay within supported screen ratios:

  • I'd rather not have black bars on the game window
  • You don't want the user to be able to resize to any window sizes with the expand stretch mode as they can see a lot more of the level, thus break the game/level design.

Do you know how to do that well? I have some code that does keep the game within size ratios, but there are bugs as I'm forcing a window resize every time the viewport resizes.

Here's the current code that's causing bugs. I'd mostly like to know if this can work or if resizing like this isn't supported?

# Only allow window size ratios between 4:3 and 2:1, which are common display ratios
export var screen_ratio_range = Vector2(4.0 / 3.0, 2.0 / 1.0)

[...]

func _on_SceneTree_screen_resized():
	var window_size = OS.get_window_size()
	var ratio = window_size.x / window_size.y
	var ratio_clamped = clamp(ratio, screen_ratio_range.x, screen_ratio_range.y)
	# Use the is_equal_approx function to compare two decimal values, as computers 
	# don't store floats with absolute accuracy.
	var is_ratio_in_range = is_equal_approx(ratio_clamped, ratio)
	
	if not is_ratio_in_range:
		OS.window_size = (window_size / ratio * window_size.x).round()

The other approach I see is to not allow resizing the window - as some games do, and to use a menu to let the user pick the resolution.

Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

Personally, I think the black bar approach is fine, maybe even better. Artificially limiting the window's actual aspect ratio to 2:1 would make it impossible to maximize on 7:3 monitors (aka 21:9). As long as the black bars only show up on such extreme aspect ratios, and not just all but one (which would be a fixed aspect ratio).

2d/platformer/README.md Outdated Show resolved Hide resolved
2d/platformer/README.md Outdated Show resolved Hide resolved
Use the keep_height aspect ratio option as in the original demo. This prevents
seeing the clear color when making the window tall
Change the ParallaxBackground scene so mirrored sections don't disappear on wide windows
@NathanLovato
Copy link
Contributor Author

I copied the settings from the original demo. So now there are black bars at the top and the bottom of the window when you make it taller than the base ratio. The last commit also prevents the background from blinking, and I extended it so you don't see the clear color when the background is wide.

I also tweaked the markdown.

Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

With the latest commit applied, the background color now works correctly, but the other background elements like the clouds and platforms don't work anymore. The clouds only appear on the right side of the screen, the platforms never appear at any resolution.

With the commit before the latest commit, the background elements show up, but do break at bit at wide aspect ratios.

onready var _pause_menu = $InterfaceLayer/PauseMenu


func _init() -> void:
Copy link
Member

Choose a reason for hiding this comment

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

🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi! I'm just jumping in. What's the issue here? Is it the use of -> void:?

I've just tested and the platforms in the background are visible for me but the clouds are indeed to the right. This should be a simple fix of offsetting the ParallaxBackground.

I've made some tweaks and I can't find issue with wide resolutions.
platformer-wide

I can make other touch ups and commit if that's all good @NathanLovato

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sure, by the way @aaronfranke you have write access to my branch and PR, so when you find a typo or something, you can remove it right away - it'll take you less time than asking us to remove one.

I'm jumping between many projects every day, all but this one use type hints and the related completion. I don't even notice them to be honest. I'll check if I can't add a local pre-commit commit hook to run the script that cleans type hints for future contributions. Or at least I should be able to do that on save in Emacs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I can confirm the platforms do show up here - and yes the clouds are offset, I'll move them to the left. I'll also try to see what could make the platforms disappear.

Screenshot from 2020-02-12 07-40-31

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a way to change editor settings per-project? Type hints seems like something that could be overriden per file or per project.

Copy link
Member

@Calinou Calinou Feb 14, 2020

Choose a reason for hiding this comment

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

Is there a way to change editor settings per-project? Type hints seems like something that could be overriden per file or per project.

Not yet, although I suppose we could move the type hint setting from the Editor Settings to the Project Settings. If we really need it, we could add a project setting, while keeping the editor setting as an enum with 3 values: "Project Default", "Always", "Never".

(This would be a good idea for settings like V-Sync too.)

@NathanLovato
Copy link
Contributor Author

Here, like yesterday, the platforms show at every window size. I fixed the clouds, but also tried to change the parallax layer setup so they don't get culled - they had their origin in the top-left of the game window, maybe this can create some edge cases?

I also removed the type hint.

Screenshot from 2020-02-12 07-50-38

@johnnygossdev I took care of this one because you don't have write access to my branch and it just took a few minutes. It's a matter of adding the final touches at this point.

@johnnygossdev
Copy link
Contributor

Sure no problem! 👍

@NathanLovato
Copy link
Contributor Author

Just added some clouds for when making the window too long

Screenshot from 2020-02-12 07-54-17

@aaronfranke aaronfranke self-requested a review February 12, 2020 18:00
@NathanLovato
Copy link
Contributor Author

Ah, I had forgotten to change the code order, thanks for cleaning this up!

Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

I made a few tweaks, but otherwise this is fantastic and works perfectly, and I can't find anything else to improve. Thank you very much for all this work, it's a much appreciated improvement.

I extended the background far enough that the play area stops working first when making the window wider.

Once concern is that it might be over-documented in a few places, but I think that's fine since I would expect this demo to be viewed by beginners who need this information.

@NathanLovato
Copy link
Contributor Author

Once concern is that it might be over-documented in a few places, but I think that's fine since I would expect this demo to be viewed by beginners who need this information.

Yes, so that was the idea. We assumed that this would be a go-to demo for beginners as it's the kind of thing a newcomer may want to do with Godot.

Also, we've designed it with the idea of not writing and having to maintain a long text-based, step-by-step tutorial, which is difficult to keep up to date. So all the explanations are inline.

Thanks for taking the time to review and make fixes!

@@ -0,0 +1,26 @@
extends Control
Copy link

Choose a reason for hiding this comment

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

Seems odd that this file is entirely uncommented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not the core of the demo, that's about 2D platforming. I'd recommend following a tutorial dedicated to pausing, as half of the work is happening in the inspector, with the pause mode on the nodes.

Copy link

@Sslaxx Sslaxx Feb 19, 2020

Choose a reason for hiding this comment

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

I meant within the context of the demos, not tutorials or other documentation. If the actual pause demo is going to be updated as well, at least giving a pointer to it might here be useful.

In general, if something is going to be demo-ed somewhere else in more detail, pointing to that in another demo might be helpful. You can't assume everyone is going to look at every demo, or in any particular order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I may not get what you mean. You mean we could add a comment telling people to go look at a dedicated demo about pausing? If so sure, anyone can commit something like that anytime.

Copy link

Choose a reason for hiding this comment

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

Telling people which other demo would demonstrate a feature in more detail, yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants