-
Notifications
You must be signed in to change notification settings - Fork 274
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
Food please/rewrite movement overworld cleanup #224
Conversation
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:
|
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. |
# 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() |
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'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.
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. |
No worries, I didn't take it like that at all! I'm just letting you know about my availability |
@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! |
This is a cleaned up branch from #223 to help with the code review. Here's a copy of the original PR description: