-
Notifications
You must be signed in to change notification settings - Fork 248
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
Rewrite initial movement demo in GDScript 4 #209
Rewrite initial movement demo in GDScript 4 #209
Conversation
Thank you for your work and patience as we were at the game developer conference. Unfortunately, my teammate came back sick, which is why you haven't gotten a reply earlier. I just took the time to go through the code. That's some really nice work you did there, names are clear, and you explained your structure and approach, which makes the review process smooth. I will let @Xananax check this too before saying too much, but there's one goal mentioned on the issue that's important to me:
The current code approach is designed with some level of flexibility and abstraction in mind. You mention trying to avoid coupling. In practice right now, it creates a certain amount of splits in your code that we could avoid without changing the game's behavior. This means that to understand the game logic and when debugging you will often have to jump around to different places and code files. I know that I taught things like these, and that it's pretty standard and considered clean, but these days I find it more productive to remove some of these separations. Concretely, here's one example: I'm not sure we need a LocalPlayer and a separate CharacterController node. Player controls are going to be very specific, and this demo is going to be single-player, so we could just have one script representing the player. Again I want to let my teammate do his review as he's been following this a little more closely than I did. I noticed a couple of instances I would change, and I imagine that the JRPG demo I coded a couple of years back very likely suffers from this too. You mention looking to follow it, and it'd be a fine starting point, but I'm pretty sure we could simplify it. Note that if we decide to make these changes, we can directly make them as part of this PR and the review process to show what and why we change and save some back and forth through comments and commits which can be frustrating when working on open-source projects. In any case, I'm really thankful for you taking the time to do this, and the community will definitely greatly appreciate it too! |
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.
Hey! Thank you for your patience. I've been through a bit of a rough patch, but I finally got to review this.
Thank you for your excellent submission! It's very well written, functions and variables are well named, there are comments everywhere where I needed them.
I have several suggestions or questions for this review, without clear recommendations or corrections yet. This is because this is a conversation, and I want to invite you to answer so we can find an implementation that still amuses you to make.
While this project needs to answer requirements of simplicity and flexibility, it also needs to be something you're proud of.
Once we agree on those high-flight decisions, and changes submitted, I can do a second review that is more detailed about specific functions, names, and so on.
I will ask you to consider my suggestions with an open mind, as they will often go resolutely against what is typically considered "good code". As you will no doubt notice, I mostly reject the notion of "best practices".
Once my first shot at a feature works, I go through my code, and make it less flexible, less smart, and more focused, discoverable, and dumb. I typically reduce it by 50% or more, and I think this is the kind of fat we can trim here too.
Will be awaiting your feedback. Good day!
Just wanted to say sorry to hear that you were sick and thanks for taking a look through the project. I hope I did not pester you too much as I was puttering around with github.
This is great. My goal has always been to make toys that people would want to play with, so I'm more than happy to submit to your experience! I also benefit from having extra eyes on the code as it makes me think hard about why I'm doing something.
I think I've responded to the conversation points. I look forward to diving into the review. I'll let you resolve the issues as the code is revised and re-reviewed. |
I apologize that this is so long. TLDR: I moved everything over to using built-in Godot physics and beat my head against a resolution issue for about a week. OK, I've added a commit for review (please, when you have a moment!). I have only updated assets and code, not the project docs. I'm assuming that once everything is copacetic I'll update the docs and squash all commits before finally merging. I'll also leave the conversations open above and let someone else resolve them once you think the issues have been addressed. Hopefully they are already addressed without introducing new issues. There are big changes here. Basically, I've discarded the previous system of tracking data via managers and now make extensive use of Godot's built-in physics system. The lynch pin is the CollisionFinder, a new object that is used to query the physics state of the game at a given cell. ANYTHING can figure out what is at a given position by creating a CollisionFinder and plugging in a few properties, such as which physics layers to scan. Consequently, most interdependencies are gone and the code should hopefully be much easier to follow. This has a few caveats, of course. Since the CollisionFinder looks at cells, I've had to rework gamepiece's a little. A gamepiece immediately moves into a cell it occupies, which allows it's collision shape to be placed at that cell (allows seeing who occupies that cell immediately). Anything following the cell moves with it, as illustrated below. Of course, we want the characters to walk, so each gamepiece has a Path2D and PathFollower2D that moves cell to cell. They are decoupled from the Gamepiece via a Node, so all points can be added to them with the origin as their reference. Finally, there are a few RemoteTransform2D anchors that other objects (in this case camera and graphics/GFX) can hook into to follow along smoothly. Note that I've also tried to decouple object types (e.g. Gamepieces from their animation) so the following is the same scene as above except with a GamepieceAnimation child scene instead of a Sprite2D. Controllers are probably the most convoluted/complex objects right now. Basically, a Gamepiece can move to any cell. It's a dumb object. If you want to issue orders for it to follow that's the responsibility of something else. In this case, I've written a player controller and a simple AI controller that follows a path, as per the issue above. Controllers are very specific to a given game or character (e.g. a flying character will be controlled in a different manner), so I'm not sure how generalized they should be. A particular edge case that I'll mention here is when two gamepieces try to move into the same cell in the same frame. Since the physics server updates a frame later, neither registers that the other has moved and both move to the cell next frame. To resolve this I've allowed the FieldEvents autoload to cache movement requests on a per-frame basis. Is it a bad idea for autoloads to have "private" data? It should be accessed in a read-only method. Anyways, took a bit to figure out what was going on with the collisions. I know there's lots of changes here, so hopefully we're forwards rather than backwards. Please let me know your thoughts and I'll get back to you ass soon as I can. Thanks again for your patience. I probably should mention that much of the time was spent trying to figure out why movement was "jittery". I think I rewrote the pathing code 3 times. A week later I finally figured out that I was testing at a resolution that didn't scale correctly so the sprite was bouncing around on my monitor. I learned something! |
This is lovely; I like it a lot! It was a pleasure to go through the most interesting bunch of code I've read in a long while. There are a lot of neat non-traditional solutions to traditional problems. Would you like to update the documentation and such before I review it, or would you instead I comment on the current code? |
Hey, thanks! That's encouraging. I hope that it's not too unorthodox for newcomers to the project.
It probably makes most sense for me to do so at the end to capture any last-minute changes. If it's not too presumptuous I also plan to rewrite the scenario/plot and update the banner and icon. I think that it makes most sense to do all of that as the last step before merging. Thanks again! |
I started, but I'd like to push some changes (faster than commenting). Would you rather add me to the repo so I can push here, or fork and PR over your PR? The first seems logistically simpler to my eyes but you may feel different. |
Sounds good. I've sent an invitation.
…On Wed, 3 May 2023 at 14:26, Xananax ***@***.***> wrote:
I started, but I'd like to push some changes (faster than commenting).
Would you rather add me to the repo so I can push here, or fork and PR over
your PR? The first seems logistically simpler to my eyes but you may feel
different.
—
Reply to this email directly, view it on GitHub
<#209 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A22FBGNUATHAV6OZSG5N2RTXEKIL5ANCNFSM6AAAAAAWUBKQUA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
I've simplified what I could simplify; there are a few things that I'm not sure you need; feel free to revert if it's the case. I could axe more, but I'd like to see more of a vertical slice before I do (dialogue, combat, whatever you feel you need to add). I may remove things you need because I do not imagine how they would be used well enough. Overall, this is very nice, and my recommendation is to move on the next stage before optimizing this overly much (writing some documentation for this -- if needed, and adding the other systems). Great work! |
Looks great to me. I would only ever revert something if I felt that it was essential for program flow. I tend to generalize and complicate, so I find it helpful to have the KISS principle applied.
Thank you! I have a handful of questions for you, if you don't mind. By documentation, do you mean in-code documentation/doc strings or the readme, license, etc.? I'll pick away at this next. At what point would you like commits to be squashed? Seeing your work I much prefer smaller, more specific commits and will attempt to do so from here on out. Just to be clear, is this a good time to begin incorporating other features? At the moment, the following order makes most sense to me:
I've been enjoying the project very much, thanks for all the help. |
I mean anything necessary for people to use the software. For now, I think docstrings is sufficient. I would focus on anything that seems not crystal-clear, as long as it's "hot" so to speak (have the details in your head still). Anything else can be done later. If you don't deem anything too difficult to document later, then it's fine to skip that step now.
Whenever you prefer, it doesn't matter to me. Feel free to open a new PR based on this PR if that keeps it cleaner. @NathanLovato do you have an opinion about this?
Be careful, the mini-commits fever might bite you! I love small commits for the documentation they create. I tend to commit several times per hour. If I am doing something more experimental and have to make sure it all works, I then try to mentally roll back and still commit each thematic part individually for documentation purposes.
Yes. I agree with your low-hanging fruit and subsequent steps. I don't know how complete this demo should be, but inventory might also be a feature? |
Squashing is not necessary, basically we can do it upon merging but we can also do a regular merge keeping the commits. Either is fine. It's more on Godot that they try to squash commits because the commit log is so big and they need to use the commit history and bisect (find a commit without a bug and jump to the middle commit recursively to find which commit introduced the bug) to find regressions. If you're looking to create separate PRs I´d say the working movement could already be merged and you can keep working in the same PR. Mostly to restart from a clean github conversation/review state. If you want to do that please just let us know when this is ready to merge. |
I've updated the in-editor documentation as well as that of the Github project. The documentation is a draft, but it should preserve the intent and theme of the classes included with this release. There are a few outstanding items to change in the project docs (the banner/aesthetic, story & characters, etc.), but that will need to happen after a chat about assets and combat design. That being said, I believe the project is ready for merge. I do think separate PRs will be more clear. Once this one is merged I'll draft another PR for the next phase of the project. |
Whoops, forgot to mention that the following commit renamed Grid to Gameboard. That's pretty major if you've been reviewing the project. |
Thank you very much for the hard work! I went ahead and merged into the master branch, now renamed to main. Great job so far. There are a couple of things we may tweak after the merge, mainly code style guidelines we're still figuring out with Godot 4, now that it allows us to write code differently. It's not something you have to worry about but for example, we generally try to keep related code grouped together to avoid having to jump around too much when reading code, and so you can connect signals like this now, with inline functions: func _ready() -> void:
FieldEvents.player_path_set.connect(
func(gamepiece: Gamepiece, destination_cell: Vector2i) -> void:
gamepiece.arrived.connect(hide, CONNECT_ONE_SHOT)
position = gameboard.cell_to_pixel(destination_cell)
show()
) Sharing this also so you can note the CONNECT_ONE_SHOT flag, which disconnects the signal after one callback. Anyway those are details, nothing too important to worry about. Are you looking to continue with other game systems? If so, can we help you in any way? |
My pleasure; thanks for the guidance/help so far.
Sounds good to me! I look forward to the updated guidelines.
I would love to continue, if it's not too slow a pace for you all. I'd like to get conversations/scripted events going in the next several days. I plan on having a much quicker turn-around now that we have a solid base to work off of. I did have a couple of high-level questions:
Thanks again. |
It's great!
What do you and @Xananax think of using Dialogic here? Dialogues in general are an area well suited for using a plugin, they're quick to do something basic that doesn't scale but really slow and tedious to do something that scales well with the game's content. Dialogic just so happens to cover about all the needs you'd have making an RPG, and it's open-source.
I'd say something iconic and not too complicated like old Final Fantasy (turn based) or RPG maker-like would be great. We have an unreleased demo showing how to do a pokemon-like combat system. A slightly old-school turn-based system like this is good because it's a suitable foundation for many different variations, it's relatable for most users, and it's not too complex to code so hopefully it's accessible for people to study and remix.
It's fine, some parts could probably be simplified, but you can use it as a reference. A general area of improvement compared to that course would be avoiding splitting code and data into too many individual files. I did split this much because back with Godot 3.2 or something it was super inconvenient to do things like editing arrays in the inspector, you couldn´t directly reference nodes or export custom resources, and so on. So I'd say try to see where Godot 4 features can help simplify things as you go. If you need a hand, anytime, you can push your progress and we'll be there.
The tweet you linked is a style our artist came up with just a couple of days ago, and it'd probably be good to use here. It's a style designed specifically to make art pretty quick to produce and accessible to many people while remaining appealing. We can go with something like that. Thibaud, our artist, has a lot on his plate at the moment, but if you're fine working with placeholders (aka "greybox" or "programmer art") we could consider replacing the current sprites with a bunch of colored shapes. |
I'm ambivalent. On one hand, I'd like the demo to stand alone and be a reference implementation of all the pieces necessary for an old-school RPG. On the other, it's true that dialogue alone could be a good portion of the complexity if we want to make it production-like, and it may not be worth it.
As Nathan says, I would try to keep it as uncomplicated as possible, in the vein of Final Fantasy 1. That still leaves some interesting open questions, such as how to handle order (do characters have an initiative stat? Is it always "all the party then all the enemies"?); but also leaves the implementation pure enough that it is fairly simple to add a timer and have an FF7-style combat without changing much. I don't know if you want to go as far as magic, items, and their different potential effects and targets. However, one interesting challenge, if you feel like it, would be to handle how an increase in the luck stat affects critical hits, loot, etc. This is part of almost every RPG, and is absent from almost any tutorial or reference implementation. |
I think a plugin is a great idea. I have no experience with Dialogic. I'm a little leery from my first look, since it seems very heavy (as in, it seems quite robust and complex. Also, it has game saving routines?) for what we want to accomplish. Dialogic 2 also seems sparse on documentation. I'll take a closer look at it over the next few days.
Is this in-the-works or an old project? Sounds great, would love to poke through at some point.
Love the style, I'm going to attempt to emulate it. I would prefer to have full scale sprites since the x5 scale value is hidden on one of the nodes. This may be asking too much, but has Thibaud published any workflow details (Inkscape, animation, etc.)? I'm presuming the gif mentioned above is put together using Skeleton2D, which seems more complicated than a sprite sheet.
Definitely agree with as simple as possible. Thanks for all the feedback here. |
A feature-complete plugin will often come with some things you don't need. This one's fairly intuitive, I think most of the code is their editor. And well, they have their own data format too for git and diff-friendly files. The interesting aspect about it is that it would probably fit the needs of most Godot users trying to do an RPG. The biggest need when it comes to dialogues and quests is efficient tools to author content. But these are very time consuming to build.
The pokemon demo is relatively complete, just the repository is private right now, as with many demos we've worked on for Godot 4. We just don't manage to catch up and release them to the public with videos and all.
The animation is indeed done in Godot for the character's legs. It's much more efficient to do this way than through a sprite sheet. Right now, we don't have a guide on that. Also, Thibaud doesn't use Inkscape. If you have anything that could be useful to you, I could prepare some guide on the topic. For the team we'd only have like one slide with the art guidelines as a bullet list because professional artists each have and know their workflows and tools. But we can work on a guide for the community and contributors. Having specific questions from you or, I don't know, having a call or email conversation would be useful for that though, to understand what info people would need and not miss important bits. |
Please check if the PR fulfills these requirements:
I wanted to finally open the WIP PR for the initial demo of the Open(J?)RPG. As discussed on the 4.X rewrite issue, the goal of the initial demo was to show off the player moving around a simple gameboard.
h020_walk_loop.mp4
It turns out that's more complicated than I had initially thought and I almost certainly made more work for myself than I had expected. I'm pretty happy with the results so far. There is buttery-smooth movement of the characters and camera, the player can control any character on the map, and I'm pretty happy with the little animated guy that I drew up.
The player is able to move with the arrow keys, gamepad directions, the mouse, or a touchscreen. You can click on a character to walk right up to them. Character animations are component-based which should be nice since they can be swapped around and the code may be re-used for combat.
I've extended the concept of the Grid resource from the Tactical RPG Movement course and included Pathfinder and Directory objects, so the game-world is essentially composed from these 3 'databases' that can be passed around explicitly to the objects that need them to function (objects use the grid to place themselves on the gameboard, the player controller uses the pathfinder and directory to move the player's 'focus' around, etc). I've tried to add comments to what I thought to be confusing, and hopefully I haven't gummed up the style guide too much.
Although, there's a lot more code in here than I expected it is broken up into 6 main categories:
I fully expect this to be the most complex pull request, since this lays the groundwork for the entire game. Demoing the combat state may come close, but it will be very similar to the JRPG combat demo found in the 2D Secrets course and so shouldn't contain any surprises.
As an aside, I may have written typed assignments differently than the style guide. I have been in the habit of using : = when declaring variables instead of := but looking over the style guide I realize that we may want to change this (I always preferred typing that was closer to casting; i.e. var cell: = Vector2.ZERO differs from the following only by the casted object var _anim: Animation = $Anim. I find that := looks very similar to boolean comparisons).
Thanks for your time, I know you're all flat out. Please let me know if you'd like changes or if I should keep going further with the project (next up: conversations and events!).
Note: for now I've forked off the feature/code-rewrite branch since I can't seem to create a pull request from an entirely different commit history. Hopefully that' doesn't ping too many people.