Skip to content

Food please/rewrite movement overworld cleanup #224

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

Merged
merged 23 commits into from
Jul 1, 2025

Conversation

NathanLovato
Copy link
Contributor

This is a cleaned up branch from #223 to help with the code review. Here's a copy of the original PR description:

I've never been fully happy with the movement code for the overworld characters. The main issues were that:
a) It was very complicated, which makes it hard for newcomers to understand and adapt the demo. This was exacerbated by imperfect design (looking back, there was lots to clean up) and heavy use of the physics engine (which had a number of edge cases that required odd solutions, such as when two gamepieces tried to move onto the same cell at the same physics tick, since the physics state doesn't update until the next physics frame).
b) There were a number of bugs, mostly also related to the physics engine. The player might find sometimes that they couldn't move to a cell that they should have been able to move to.

The following is from the changelog:

This update has aimed to greatly simplify the code responsible for moving Gamepieces around the Gameboard, while keeping the structure of the code roughly the same. Most scripts dealing with the overworld/field gamestate have been changed, but the main features are as follows:

  • Collision on the Gameboard is no longer based on physics-based collisions. The physics implementation was great, but introduced a host of edge cases (e.g. two gamepieces moving onto the same cell on the same physics tick) that had to be accounted for.
  • Rather, objects used to build the pathfinder use a registry pattern; GameboardLayers register themselves with the Gameboard, and Gamepieces register themselves with the GamepieceRegistry. The board and registry update automatically based on signals from their registered objects, and the user no longer needs to worry about emitting global signals such as "FieldEvents.terrain_changed". It all happens under the hood.
  • The removal of circular dependency issues that were present in early Godot 4.X has made life much easier. The Gameboard singleton allows anything to figure out where coordinates occur on the gameboard and if those coordinates are moveable (thanks to the single Pathfinder), and the GamepieceRegistry allows objects (cutscenes!) to easily find where everything and everyone is placed on the board. Gameboard and GamepieceRegistry are autoloads that allow designers to easily observe the global overworld state without having to worry (too much) about messing things up.
  • Many objects have had their responsibilities reduced, such as Gamepiece which now only deals with moving to a point (rather than also tracking cells, coordinates, movement paths, etc.), and GamepieceControllers which now are much more focused on helping a gamepiece follow a path in response to player and AI input (as opposed to previously, where they also dabbled in pathfinding, physics shenanigans, etc.)
    Overall, the code should now be simpler to understand at first glance, and much easier for newcomers to work through. Happy designing!

Basically, I've tried to simplify the code by making it much easier to follow. The Gameboard is built from TileMapLayers that have the GameboardLayer script attached. These layers may all be stacked on top of each other. These layers have an IsCellBlocked property painted onto the tileset. Cells that are blocked are not used for pathfinding. Importantly (and differently from the previous structure), the designer doesn't have to do anything to effect the change with the pathfinder, since the Gameboard is connected automatically to these layers and changes the pathfinder on the fly via Godot's signals. An example of this is seen with the "suspicious tree" interaction, where the player walks around a tree to find a key.

The GamepieceRegistry keeps track of where all gamepieces are. Similar to GameboardLayers, any time a Gamepiece changes its cell in the registry the Pathfinder is automatically updated. Previously, there was some work for the designers to sync everything up, especially if they were implementing new controllers (for example, a controller for party gamepieces following the player is on the dock, and should be much easier to implement now).

The important thing about this registry paradigm is that Gameboard and GamepieceRegistry perform all of their updates in response to signals, so they are also autoloads that allow designers to observe the gamestate without needing to pass a host of references around or without being able to easily "mess up" what's going on under the hood. It also means that there's much less going on. There's only one Pathfinder now, and all it does is find paths. Gamepieces don't have to worry about their movement paths, since they're only concerned about moving their animations around. Maps can have a much more fluid structure, since systems aren't looking for objects in specific parts of the scene tree. Etc.

Anyways, I hope that this update is worthwhile and simple to follow. Please let me know any thoughts or questions or feedback that you may have.

@food-please
Copy link
Contributor

So, just a few comments, now that I've been away from the code for a bit. A couple of notes on the rationale behind some of the code and how it may relate to the GDScript style guide/best practices:

  • I wanted to reduce dependencies. Most everything now uses the Gameboard or Pathfinder, but I was aiming to drop cross-class relationships as much as possible. I'm not sure if way too many things make use of the Gameboard, or what your feeling is towards such heavy use of (read only) autoloads.

  • There is some inconsistency between when I used lambdas, as opposed to a full callable (i.e. class level method). There were a few classes (_on_gamepiece_arrived, _on__gamepiece_arriving, etc. in the controller classes) that I did not use lambdas. First of all, the lambdas don't get registered in the method list of the editor. Secondly (and this is what makes the first an issue), I've had a couple of classes that balloon into nothing more than a monolithic _ready() that contains several callbacks, each many lines of code.

    So, all that to say that I tried to use lambdas when the callback was short "enough" to not require a full blown method for organizational clarity (which is the majority of cases).

@NathanLovato
Copy link
Contributor Author

Thank you very much for the comments! I did a first read through the other day and could definitely see the intention to reduce dependencies and try to make it all a bit easier to trace. Also, I could test that everything is working just as well as before. So great job on that!

I have a lot on my plate currently. So I haven't had the time to go through more carefully and give you comments and get ready to merge. But I'm not forgetting this excellent contribution, just so you know.

Comment on lines +10 to +13
# For some reason, Godot 4.4.1 won't connect to gameboard_properties signals here, so its
# done on _ready instead. This means that the scene may need to be loaded before the
# debug boundaries will automatically update.
_update_boundaries()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've noticed recently that in some cases like this, the engine does not go through the setter function initially when the property is set and you have to call it again in ready. So I got into a habit of calling pretty much every setter function again in _ready as of late.

@food-please
Copy link
Contributor

food-please commented Jun 27, 2025

I have a lot on my plate currently....

I understand, I hope this didn't come across as needing attention! There's no rush on my end, the day job is busy and I'll be on holiday for a week or two. I just wanted to head off any "what were you thinking?" questions, since I've had a post-it note on my desk to check with you about some of these things. Please don't feel pressured to get to this, you're doing great work with the curriculum development. I hope that it goes well and the business is able to scale well with it.

@NathanLovato
Copy link
Contributor Author

No worries, I didn't take it like that at all! I'm just letting you know about my availability

@NathanLovato NathanLovato deleted the food-please/rewrite-movement-overworld-cleanup branch July 1, 2025 09:08
@NathanLovato
Copy link
Contributor Author

@food-please Thank you very much for a great contribution once again! The refactor looked good. I didn't make any logical changes because you need to work directly with code that's fragmented/modular like this, by making a game demo for example, to really assess if more changes or simplifications can be made. So you're in the best place to do that right now.

For the next contribution, Could you please make sure to restart or rebase your branch from main? If you have already started making changes and you need a hand in doing that, you can open a pull request and I will make sure that you have a clean stream of commits.

Also, actually it's very useful to get the changes as you make them even if a branch is an early work in progress, seeing the incoming changes as they're made in small chunks makes it much easier for me to understand your work and review and integrate things without having to keep you waiting like over these past couple of weeks.

Thanks again, this project wouldn't be where it is now without your help!

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

Successfully merging this pull request may close these issues.

2 participants