-
Notifications
You must be signed in to change notification settings - Fork 25
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
Implementations of suggested items QoL #462
Conversation
|
|
|
|
…charges and cooldowns
…, and from tower inventory to horadric cube. extend shift click on item: now also moves item from item stash to horadric cube. all these quick moves now force swap with last item slot of the target container if target container is at full capacity. update hints with logic of these controls
…ube (using HoradricCube._get_average_ingredient_level func) updated _on_horadric_stash_items_changed
…ty then levels and then ids. use exported variable for _horadric_cube_avg_item_level_label - connected in inspector
…s - should be faster, do not differentiate between consumables and oil types when sorting
…m stash - enables a view only on locked items
a76d276
to
3765906
Compare
Rebased onto main that changed history of the branch. |
8168903
to
3765906
Compare
Hi, could you try to finalize this PR without point 1 from #460 ? |
Hi, thank you for looking into this. I also think that point 1 from #460 should be left out. I'll try to summarize state of changes vs #459 and #460. Then I'll point out additional thoughts about the PR. This PR will:
My thoughts on use cases:
#460, point 1:
#460, point 2:
My thoughts on use cases:
#460, point 3:
My thoughts on use cases:
#460, point 4:
My thoughts on use cases:
#460, point 5:
My thoughts on use cases:
#460, point 6:
My thoughts on use cases:
|
Now, other points about the PR:
|
Thank you, I will try to review this PR during the weekend. |
The height of item stash menu is now different from height of tower stash and elements menu. This is because the new buttons in item stash menu increased the height. I tried making tower stash and elements menu taller to match height of item stash menu but it looked bad because tower stash looked too tall. I would like for these 3 menus to have the same height, as before, but couldn't come up with a solution at the moment. |
Remove unneeded TODO about get_player() vs get_local_player() - in this \ case need to use get_local_player() Remove unneeded nullcheck for selected_unit, "is Tower" check is enough Use belongs_to_local_player() Add InstantMoveType enum
# Conflicts: # src/game_scene/game_scene.gd # src/singletons/event_bus.gd
Almost done with review, didn't find any bugs so far but changed some things. |
Thank you for this work. I can see the overview of code structure changes. Did anything change functionally? |
No, final behavior is the same. Except that the "sort item stash" is now done through "Action", which means it gets routed through GameHost and GameClinet, but same behavior basically. |
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 checked new features in multiplayer and didn't find any issues.
- "Sort item stash" is implemented as Action - no multiplayer desync here
- Filtering out locked items in item stash doesn't affect gameplay - desync not possible, don't need this to be "Action"
- New item move operations are implemented as existing move/swap Actions - no desyncs here
Thank you for accepting the ideas and taking care of this. Even though its already merged - let me know if there's anything else related to this I should look into. |
Referring #459 and #460.
Initially: