-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Added a simple maze game #53
Conversation
Code looks quite good (yeah the semicolons are optional ... so I don't care 😜). Thanks for all the nice comments! Unfortunately I couldn't get it to run. After clicking the START button I get the following error: //EDIT: |
Thanks @pwab! I'm glad my comments helped. I wasn't sure if I added too many, but I figured it was best to be as precise as possible. I totally forgot to add a project icon, whoops 😅 |
I don't like the semicolons. They are optional and can be used on your projects, but I prefer to keep them out of official demos. |
I'll remove them. I just add them to keep my semicolon habits sharp for C++. |
b764f82
to
06e29d6
Compare
I removed the semicolons and added a project icon |
Coding style issues:
should be avoided in demo projects. Idiomatic GDScript should also avoid unnecessary comparisons of boolean vars. This reads much better:
becomes:
It's there now, why not use it?
|
One other style issue: There is a lot of mixed naming of nodes - some are CapWords, some are snake_case, some are Capitals_With_Underscores (is there a name for that?). Godot convention is CapWords. |
Ok, I'll fix these soon.
|
No! Variable names should be snake_case. I was referring to Node names. |
Oh, my bad. |
Indeed they are, and this is a matter of endless debate, of which there is no resolution. However, for demos, we must stick to a consistent standard.
I think it has its place, but I'm open to either way. I prefer it to littering the top of the script with a bunch of |
I agree that indentation is a matter of endless dabate, and I will stick to the standard. I'm going to keep the get_node calls, because I find I personally find them easier to read and understand which makes the demo (in my opinion) easier to read. |
697a1aa
to
81fa0d8
Compare
Alright, I've made the changes. I think I have the indentation correct now, being tab as four spaces, but I couldn't set it to default from the editor so I had to Google it and I found an issue saying that it was tab with four spaces, so hopefully it's correct now 😅 Also, I didn't find any (Thanks for review cbscribe 👍 ) EDIT: I figured it wasn't worth making a new comment so I thought I'd just edit this comment instead. |
Oh, I thought I had noticed a couple, but it was only this one, in
Great work, btw. I still need to dig into the code itself - it's worth at least a pass or two to see if there are any bits that can be simplified. I only have 3.0alpha1 at the moment so I can't run the project. The last snapshot build I grabbed didn't work, so I have to figure out what's going on there. |
b380565
to
5cbb829
Compare
f3d138a
to
a7bc266
Compare
Updated the project to work with the recent changes in master (mainly changing _fixed_process to _physics_process) and now the player script uses the mouse sensitivity constant. |
in 3.0 beta the line in Key.gd and in Player.gd
to
Nice demo by the way. And creepy lightning :-) |
2dbd9f1
to
0ecff56
Compare
Thanks @colorcube! The demo is taken from a horror game I made and I guess the vibe kinda stuck 😆. I changed the files so they work with Godot 3 beta 1, and I also added a small note to the title screen saying that it may freeze while generating the maze. (Because on my computer generating the maze takes a good 30 seconds or more) |
Bumping this as it should probably be tested to work with the latest build. |
0ecff56
to
3825241
Compare
I fixed a couple things and now it works with Godot 3 RC1. |
Great - I'll take a look. |
Looks good, but I am getting a ton of errors in the debugger:
|
Yeah, I’m not sure why those errors are occuring. I’ll give it a look tomorrow and see if I can pin down the source. Thanks for looking over it! |
* Generated mazes based on a user inputted seed * Generated navmesh entirely from code * A simple player seeking AI agent * A simple first person character controller to navigate the maze
3825241
to
a9fc672
Compare
It seems the error is something with the cleanup code for areas... I'm not sure if there's anything I can do to fix them. It may be a bug in Godot. I changed a few more of the comments, as I found there was still some minor mistakes. |
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.
Assuming this is still desired, it will need to be updated to Godot 3.1 first.
3d/simple_maze_game/AI.gd
Outdated
|
||
|
||
|
||
func _process(delta): |
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's the policy on extra spacing like this? @vnen
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.
Yeah, this PR is pretty old, before Godot 3.0 if I recall correctly. I'm sure there are several things that need redoing to work with Godot 3.1. The code can probably be optimized too.
As far as the spaces, I am in favor of having only two spaces between functions, but I don't know what the policy/best-practices for GDScript is anymore. I'm fine with changing it however, or closing this PR if it is no longer desired/wanted.
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.
The style guide now suggests 2 spaces between methods as of godotengine/godot-docs#2505 and godotengine/godot-docs#2521
…moved a few white spaces and silenced a couple warnings
Alright, assuming this PR is wanted, then it has now been updated to work with the latest version of Godot, Godot 3.1.1. I also fixed a couple code style issues and silenced a couple warnings. Everything still works, though I should note that I didn't really look over the code, I just updated the project and fixed the issues as needed. |
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 think this does actually need a significant amount of rework. The reason this freezes during generation is that a single maze segment looks like this, and there are 200 of them:
There are many ways to optimize this. I suggest completely removing the individual floor and ceiling nodes from each maze tile. Instead, generate one floor and one ceiling mesh that covers the entirety of the map. Also, instead of having hundreds of StaticBody nodes for each floor section, just have one, and set the collision box shape to be extremely large. This will save you about 6 nodes per maze tile.
Once you do that, you can optimize the rest of it. Change the maze tile node to be StaticBody. Take all CollisionShape children of "Walls" and "Pillars" and put them as direct children of the maze tile node, this will save you about 8 nodes per maze tile, then you can combine the meshes together, which can save about another 5 nodes per maze tile.
Unrelated: I suggest moving all of the textures to their own folder, for organization.
3d/simple_maze_game/AI.gd
Outdated
|
||
|
||
func _ready(): | ||
get_node("Agent/VisionArea").connect("body_entered", self, "body_entered_vision") |
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.
Add #warning-ignore-all:return_value_discarded
3d/simple_maze_game/AI.gd
Outdated
to_walk *= SPEED_NORMAL | ||
|
||
|
||
var to_watch = Vector3(0, 1, 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.
As per the style guide, "Use one blank line inside functions to separate logical sections."
3d/simple_maze_game/Key.gd
Outdated
var ready = false | ||
|
||
func _ready(): | ||
get_node("Area").connect("body_entered", self, "body_entered_trigger") |
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.
Add #warning-ignore-all:return_value_discarded
3d/simple_maze_game/MazeGame.gd
Outdated
# all of the cells (the tiles we need to process) | ||
var cells = [] | ||
const DIRECTIONS = {"NORTH":Vector2(0, -1), "SOUTH":Vector2(0, 1), "EAST":Vector2(1, 0), "WEST":Vector2(-1, 0)} | ||
const OPPOSITE_DIRECTIONS = {"NORTH":"SOUTH", "SOUTH":"NORTH", "EAST":"WEST", "WEST":"EAST"} |
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.
Consider using enums instead.
3d/simple_maze_game/MazeGame.gd
Outdated
var pos = tile.get_transform().origin | ||
|
||
# Add the main floor, that is ALWAYS present | ||
quad = add_quad(pos + Vector3(0, 0, 0), Vector3(0.9, 1, 0.9), verticies, indicies) |
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.
Adding a zero vector is pointless.
… These changes improved performance dramatically. Fixed navigation mesh issue and changed the size of the navigation mesh so it is easier for the AI to navigate
Made changes based on feedback (thanks @aaronfranke!). The first and most important change is a dramatic increase in performance. I simplified the node structure and made some minor changes that, over 200+ tiles, made a big difference. See below for more details. I removed the navigation debug mesh and added a comment mentioning the "Visible Navigation" option in the debug menu, as it makes more sense to use what Godot provides by default for debugging navigation meshes. I also fixed an issue where navmesh generation wasn't working due to an offset in the AI. I moved the AI to origin and added a note explaining that if the AI agent is moved, an offset will need to be added so the navmesh is at the correct position, Now everything works and the AI navigates as expected. After reading what @aaronfranke said, I decided to take a look into simplifying the tiles for better performance. Initially I thought the biggest performance block was the After this, I decided to take a shot at simplifying the maze tiles, as per the suggestion. While I couldn't totally take the suggested route, I did make changes similar to the suggestions and performance has increased considerably. The changes I made to the tiles as follows:
I don't know for sure which change gave the most performance, as I made all of these changes in one go, but I can safely say that these changes improved performance dramatically and now the entire maze generates around 10x faster! Thanks to @aaronfranke for the suggestions on how to improve performance! The demo is now much better. |
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.
Indeed, this is vastly better than it used to be, but it can still be improved.
I suggest allowing higher difficulty. I find that I can win every time easily. Maybe just by adding multiple enemies to the map. As a side note, I think that AI
should be renamed to Enemy
.
For organization:
- Move images to their own folder.
- Consider making
Key
its own scene file. - Consider making
AI
(orEnemy
) its own scene file (useful for the above idea of multiple enemies).
Input.set_mouse_mode(Input.MOUSE_MODE_VISIBLE) | ||
|
||
# Connect the pressed signal to the chance_scene_to_title function | ||
connect("pressed", self, "change_scene_to_title") |
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.
Also need #warning-ignore-all:return_value_discarded
here.
3d/simple_maze_game/project.godot
Outdated
|
||
[application] | ||
|
||
config/name="simple maze game" |
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.
Capitalize
3d/simple_maze_game/MazeGame.tscn
Outdated
script/source = "tool | ||
extends Object | ||
func e(): | ||
return 1.2 |
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.
There are a ton of these e
functions, what do they do? They seem to be placed on tons of different objects and return seemingly random numbers. I can't figure out where they are being called (if anywhere). I tried deleting some of the functions and everything seems to still work.
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.
They don't do anything, and I have no idea why they appear. I delete them and they just randomly come back, despite being completely deleted from both the node and the script editor.
If there is a fix for this, that would be great, because it is highly annoying and the scripts do not do anything other than get in the way. Strangely, it only happens for this project, all of my other Godot projects haven't had this issue.
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.
Actually, they appear in this PR too: https://github.com/godotengine/godot-demo-projects/pull/299/files#diff-de99a9f547384d26056c5251ac8a454bL50
I think this mystery is best solved before any merging.
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.
Strange, those files/scripts didn't used to appear, and I didn't see any additional scripts when I was working on the PR.
Will need to investigate!
3d/simple_maze_game/WinScreen.tscn
Outdated
border_width_top = 0 | ||
border_width_right = 0 | ||
border_width_bottom = 0 | ||
border_color = Color( 0.8, 0.8, 0.8, 1 ) |
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.
Make sure to open and save all scenes with Godot 3.1. These lines are no longer saved when the settings are the same as default.
3d/simple_maze_game/Player.gd
Outdated
vel.x = hvel.x | ||
vel.z = hvel.z | ||
|
||
vel = move_and_slide(vel,Vector3(0,1,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.
Vector3.UP
and add spaces after commas. Follow the style guide.
3d/simple_maze_game/Player.gd
Outdated
# We rotate the camera holder on one axis so the eular angles do not get messed up. | ||
# If we rotate the camera on both the X and Y axis, then the camera rotates strangely. | ||
camera_holder.rotate_y(deg2rad(event.relative.x * MOUSE_SENSITIVITY * -1)) | ||
camera.rotate_x(deg2rad(event.relative.y * MOUSE_SENSITIVITY * -1)) |
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.
Scale MOUSE_SENSITIVITY
instead of using deg2rad
. Better to use radians everywhere.
3d/simple_maze_game/Player.gd
Outdated
# other than that, everything is basically the same. | ||
|
||
# Member variables | ||
var grav = -9.8 |
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.
To be honest, I don't really see why this 3D maze game needs jumping or gravity in the first place. It doesn't actually serve any gameplay purpose, you can't jump over objects or get a better view by jumping. If you get rid of gravity you can also get rid of the floor CollisionShape.
But if it's desired, set via grav = -ProjectSettings.get_setting("physics/3d/default_gravity")
to make this value consistent with project settings.
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.
Good idea, will delete jumping and gravity, as it is not needed for this demo. I think it just left overs from a previous project, if I recall correctly.
I might be able to do this, but right now the code isn't really setup to generate a navigation meshes for multiple AI, or at least I don't think it is. Worse case, increasing the speed of the AI should make it harder. As for renaming, I don't really care either way, but from a git history point of view, it might be better to leave the name as is so the AI.gd filename still makes sense (or change the file name, but I've had issues with doing that with git in the past)
Easy enough to do, though I am not sure if that would follow the style set in the rest of the demos. For example, the material_testers and platformer, and kinematic_character demos all have their textures stored within the project file. If the general vote is to move images to their own folders for easier organization, then I am all for it and will make the change.
Good idea, will do! I have also replied to some of the code comments. Thanks for looking through this @aaronfranke! I will try to make some changes once I a chance! |
It's arguably vastly easier to understand how a project works when it's organized. The kinematic character demo only has two extra PNG files so it's fine to not put them in a folder. However, this demo project has 9 JPG files, so it makes a lot of sense to organize them. I am aware that many of the projects have a flat directory structure (#280), but I'm trying to get this changed, at least in the cases where there are many of the same type of file and it makes sense to do so. For reference, the following demos currently have subfolders: 2D DTC, FSM, GBM, NavAstar, Physics Platformer, Platformer, RPG, SS Shaders, 3D Truck Town, GUI translation, Misc Window Management, and Mono DTC, Many of which organize the art into its own folder subfolders ( My PR for updating IK moves the FPS code to a folder and my PR for adding the 2.5D demos have the projects organized into a folder hierarchy. I plan to update the material tester demo as well, though I was waiting for #220 and #274 before doing so. |
I am not disagreeing that better organization would be great for this project. If this was my repository, I would have no issue with having the textures within their own folder. Last I knew none of the demos had subfolders, but evidently that has changed since I last worked/visited the GitHub repository. I'll see about moving the textures into a subfolder called The reason I was hesitant to make the change is because I didn't/don't know where the Godot team stands on this matter. I don't want to make the change and move them to their own folder, to later be informed that the official style is keeping them all in the project folder, requiring the changes to be reverted. That said, if the other demos are using subfolders, than I suppose it shouldn't be an issue to use them here either. |
…useless features and functionality, and reorganized the project folder
Sorry about the long delay in implementing these changes! Life got in the way. I implemented a bunch of changes. Now images are in their own folder, the player cannot jump (and gravity is removed), the floor collision shapes are removed, and a bunch of code warnings have been dealt with. |
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.
Looking really good! Could also use a rebase and/or squash, but it's not necessary.
3d/simple_maze_game/project.godot
Outdated
|
||
[application] | ||
|
||
config/name="Simple Maze Game" |
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.
Demo projects are always going to be simple, so this could be renamed "3D Maze Game" (along with the folder name being maze_game
).
3d/simple_maze_game/AI.tscn
Outdated
script/source = "tool | ||
extends Object | ||
func e(): | ||
return 1.25 |
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.
This mystery still needs to be solved before merging.
…he strange tool script being included for all resources within the project. Made minor visual adjustments as I redid all of the resources for the 3d maze game
Okay, this PR should be ready to go now! I renamed the project and fixed the strange tool scripts being attached to all resources within the game. |
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'm reviewing this as Approve because it's already merge-worthy IMO, but there are some editor/display_folded
lines which could be cleaned up.
Also note that there are still false positives for GDScript warnings, but that's a Godot bug.
material/0 = SubResource( 9 ) | ||
|
||
[node name="GrabArea" type="Area" parent="Agent"] | ||
editor/display_folded = true |
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.
Please remove all editor/display_folded
lines. Here's a bash alias that you can add to ~/.bash_aliases
:
alias tscnnoeditor="find . -name '*.tscn' -exec sed -i '/^editor\//d' {} \;" # Remove "editor/display_folded"
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.
Where do the images come from? Are they under copyright? Do you have the legal ability to include them in this repo and place them under the MIT license?
get_node("Meshes/Pillar_NE").visible = true | ||
else: | ||
get_node("Meshes/Pillar_NE").visible = false | ||
get_node("StaticBody/Pillar_NE_CollisionShape").disabled = true |
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.
Instead of .visible = false
and .disabled = true
, you should use .queue_free()
so that we can just delete the nodes.
Then, we can also just make everything enabled by default, remove the .visible = true
lines, and only take action if we need to delete the nodes.
I think the images come from CC0Textures.com or TextureHaven.com, but honestly I don't remember anymore. It was a 2+ years ago. I think they can legally be added to the repository, but again, I'm not sure. Edit: Actually, looking at it, I believe they come from the Material testing demo here on the demo repository. I have no idea what license those images have, nor where they are from (might be worth looking into). To be 100% honest, I have very little interest in maintaining this project anymore, simply because I no longer am familiar with the project. My coding style and experience in game development has changed dramatically since I created this project. I believe it would be easier to recreate the demo rather than maintain it at this point. I'd say, unless you guys are willing to make the changes and maintain the demo, it is best to close this PR. Especially with the navigation system in Godot being redone/revamped, I'm not sure this demo would be useful outside of Godot 3.2 anyway. |
@TwistedTwigleg Thank you for your honesty, and you're right that navigation will be reworked in the near future, and I don't particularly feel like fixing up and maintaining this either. Also, I appreciate you sticking with it for so long, I'm sure now you know much better what kind of best practices the Godot team expects, and also how to make your code run faster (I remember it was super slow at first). If you submit more pull requests, you're more than welcome to. I plan on frequently reviewing these, so you shouldn't have to deal with PRs that sit around for 2 years anymore. The review time should be closer to a week, hopefully... :) |
Thanks @aaronfranke! |
I added a simple maze game that has the following features:
This should fully close issue 4301 in the Godot engine repository, as this demo shows how to make a navmesh entirely from code.
If you guys see anything out of place, or something that should/could be changed, please let me know!
(I should probably remove all those semicolons...)