-
Notifications
You must be signed in to change notification settings - Fork 12
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
#88 Inventory system #94
Conversation
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 there are things to change. See my comments for that. Please also comment your code, using double #
See the CONTRIBUTING.md for some information about that.
Will the .tmp files stay ? Or was that just for testing ? If it's for testing, I would prefer having them in a dedicated folder instead of putting everything at the root of the project
Do you plan to write some tests ?
Uneeded script that I forgot to remove earlier
Renamed consumables_limit to max_consumable_number for clarity Renamed ConsumableSlotCompontent to InventoryHUDConsumableSlotComponent for clarity Renamed function prepare_consumable_array to _update_consumable_limit for clarity and move the places where it is called for simplicity
Removed that this class extended from node as it is uneccessary
Removed uneccesary lines
Removed pass call
Removed pass call
Renamed variables for clarity
Made the variables that hold the various items in the inventory components private to make sure noone changes them from other scripts in the fututre
Make it so players don't lose the consumable if they lose the slot it's in and they still have open slots
Added unit tests
Fixed some of they typoes you pointed out and made some changes for clarity. Also I tried to make the GUT test, please look at it in case I did something wrong there |
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 tests seem good to me; i think there is still doc missing, mainly for class description and class variables
Okay github decided to split my review in 3, but that's one single review 🤷 |
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.
A little something on test to change
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.
Did you look at the render inside Godot's editor when editing the doc ?
Not sure if everything is clear now, please tell me if you want me to comment on something |
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.
Typos
After testing in local it seems to work as expected. The buttons are not very aligned and some fonts are being squished in the button, as well as the actual HUD display being a box on the side instead of a bar at the top, but this is more a question of UI, it can and will be changed once we have assets to put there and an actual design in mind. Good job 👍 |
Description
Added a new manager to the godot auto load called "InventoryManager". It's spilt into 5 part, the main script called "InventoryManager", and four component parts which are responible for keeping track of gold, torches, consumables, and relics respectively. They all have helper functions to add and remove items from the inventory. They all also have signals for changes in the inventory
Added a Inventory HUD scene with a script which is toggleable with a function in the InventoryManager.
Added Relic and Consumable Placeholder scripts, they are both resources with a few variables and no functionality other than empty functions
Related issue(s)
#88
List of Changes