MagicStorage icon indicating copy to clipboard operation
MagicStorage copied to clipboard

New filters, sorting and organization, ItemCheckList integration, item testdrive, restock and other QoL improvements

Open AqlaSolutions opened this issue 7 years ago • 47 comments

  1. Sorting by Quantity replaced by sorting by item Value,
  2. Only recipes from known ingredients are shown (if ItemCheckList is installed), non-directly available recipes (through intermediate crafting) are also considered in this check, you can mark items as not new with right click on currently selected recipe when in the "Only new items" mode.
  3. Instead of "Available recipes" and "All recipes" buttons there are 3 new buttons: "New recipes" (showing never crafted recipes), "All known recipes" and "Hidden recipes"
  4. You can (un)hide recipes with ctrl+left click,
  5. Gem cost is 2 times lowered,
  6. 30 crafting stations instead of 10,
  7. Added sorting by DPS,
  8. Added DPS calculator to the crafting menu and to weapons tooltip (based on damage, defence and use time though it's not enough for accurate values),
  9. Search text can be canceled with right click on text field,
  10. Arrow keys, home, end, delete work in search field,
  11. Added more filters for weapons and equipment,
  12. Added "Recent items" filter that shows last 100 deposited items which were not deposited in this heart before, they are shown with "shinyAndNew" decoration too,
  13. Mods are selected with left-right click instead of searchbox typing,
  14. Recipe item icon is displayed on top instead of "Selected recipe",
  15. You can open ingredient crafting by right clicking it into recipe,
  16. Right click on Deposit button (now named "Transfer All") to Restock,
  17. And Left Click to Quick Stack (with Alt to Deposit All),
  18. Favorites support in storage and recipes (alt+click),
  19. A toggle to show only favorited items,
  20. You can freely take new weapons, tools, armor and equipment "for demo" but only once (it's not in the "new" list afterwise). Those items are removed on death and while you have them with you incoming damage is 5x multipled. Also they cost 0 coins if you try to sell them. Use right click on "Craft" button. The last feature is available only in single player.
  21. There is a hot key to check whether you've already seen (if ItemCheckList is installed) an item and how much of it you have in latest opened storage,

I don't expect you to accept this whole pull request but please feel free to use any parts of it to enhance your great mod. In my branch I changed author and version info, feel free to edit it before merging.

AqlaSolutions avatar Aug 11 '18 18:08 AqlaSolutions

Please see commits instead of diff, it looks like line endings broke things.

AqlaSolutions avatar Aug 11 '18 18:08 AqlaSolutions

The hero we need but don’t deserve. Seriously though, thank you for all of this. I hope this gets accepted.

tiemonl avatar Aug 13 '18 13:08 tiemonl

Can this be used as a replacement for the original mod in its current state or not? In singleplayer it seemed fine but it seems broken in multiplayer. If you deposit items into the system, and try to take those items out afterwards, it "duplicates" the item. It leaves behind a fake version of the item that doesn't go away until world/server restart. Crafting doesn't work at all. I'm not sure if this is normal behavior or not; Does this mod support multiplayer?

ParticlePop avatar Aug 19 '18 00:08 ParticlePop

@rel00p I would not use it in multiplayer in the current state. It would take much more effort to make this work in multiplayer. But for single player it seems to be pretty stable.

AqlaSolutions avatar Aug 19 '18 15:08 AqlaSolutions

@blushiemagic may I publish this fork on the forum in a separate mod topic? Of course I will clarify that it's based on yours.

AqlaSolutions avatar Aug 20 '18 21:08 AqlaSolutions

@blushiemagic may I publish this fork on the forum in a separate mod topic? Of course I will clarify that it's based on yours.

@AqlaSolutions you could also ask for repo rights and take it over?

tiemonl avatar Aug 20 '18 22:08 tiemonl

@tiemonl I'm not that kind of developer. I just program QoL things that I need for myself while I play and share them as is. But I'm not going to support the mod for a long time or fix multiplayer.

AqlaSolutions avatar Aug 20 '18 22:08 AqlaSolutions

@AqlaSolutions sounds like merging this fork if @blushiemagic accepts it, would be the better solution. Thanks for all your work. Really impressed by it. 👍

tiemonl avatar Aug 20 '18 22:08 tiemonl

@AqlaSolutions, I'm build and use your forked version almost every time when you update it. I don't sure after which commit things what @rel00p spoke of start happening in multiplayer, but when I'm build your version when this commit was the last, everything in multiplayer worked perfect.

ghost avatar Aug 22 '18 11:08 ghost

@sendreu ah, that's pretty old commit already. I just don't use multiplayer so I don't have a big interest in investigating it but if you or @rel00p point me exact commit where I broke it I might be able to fix it.

AqlaSolutions avatar Aug 22 '18 13:08 AqlaSolutions

@AqlaSolutions, I found when that things start happening. Everything works here, but doesn't work here.

ghost avatar Aug 23 '18 16:08 ghost

@sendreu, thanks, I think I fixed it but I didn't test it because I don't use multiplayer. Feel free to check it.

AqlaSolutions avatar Aug 23 '18 17:08 AqlaSolutions

@blushiemagic I resolved merge conflict except that your commit "Fixes #53 (#58)" had to be reverted as there was too many GUI changes to merge it.

AqlaSolutions avatar Aug 23 '18 17:08 AqlaSolutions

@AqlaSolutions, tested it about ten minutes in multiplayer, seems everything works like it should. Thanks for the quick fix. 👍

ghost avatar Aug 23 '18 18:08 ghost

@rel00p, you might want to check it too.

Build https://github.com/AqlaSolutions/MagicStorage/releases/download/v0.4.100.2/MagicStorage.tmod

AqlaSolutions avatar Aug 23 '18 21:08 AqlaSolutions

A single PR with 20 features is quite a difficult thing to try to make sense of for a github project maintainer, especially one with several files that seem to be completely changed just to change tabs to spaces. (The diff for Components/TECraftingAccess.cs in particular seems off. It looks like every line was changed but the content seems the same.)

Blushie hasn't replied here yet, but I know shes a bit busy. It might be worth it to make a PR with just a single feature that is easy to verify via the diff. (Basically something easier to test and verify)

JavidPack avatar Aug 24 '18 00:08 JavidPack

Pull requests aren't really meant for huge sweeping changes. Some changes are good, some are breaking, and some don't fit in, and with a single pull request it's hard to pick out the parts of the code that are good and the parts that aren't (especially when I barely have time). As jopo said, pull requests with single features work better.

blushiemagic avatar Aug 24 '18 02:08 blushiemagic

@JavidPack, @blushiemagic , I agree it would be better to have them separate. Normally I would do it that way but this time it was about adding things I need as quickly as I can so I can continue playing. I wasn't about to submit a pull request at that time. Testing them in different branches and later in another merged branch would require a special effort which was against my goal. This pull request is created just because of my decision to share what I have done for myself in case it may be useful for other people. Can you please try out new and changed features in the current state and provide me with things that don't fit in or should be changed before merging? If it's not too big work I could just undo them. Yeah, that tabs/spacing/line endings issue always bothered me in git. In the most used tool in the world the conflict resolver can't treat tab, spaces and different line endings the same way. I just use Visual Studio with default spacing and line endings settings...

AqlaSolutions avatar Aug 24 '18 07:08 AqlaSolutions

First of all, I don't have much time to test, which is why I rely on seeing the changes to the code. Which isn't possible when everything's mashed together like this. But carrying on with the concepts themselves:

  1. It doesn't really make sense to remove sorting by quantity.
  2. It's intentional that all recipes are shown. For example, it's useful when you know a specific item you want to craft and want to start farming for (like the Shield of Ankh). What is meant by new item here?
  3. This mod's meant to make storage easier, and keeping track of "new" recipes doesn't really fit in (especially on multiplayer). And not sure why it's necessary to remove available recipes.
  4. If I'm understanding correctly this kind of ties in with the previous two features.
  5. The gem costs are mostly there for balance reasons, and not something I'll change. (Maybe one day I might add things similar to Shadow Diamonds though.)
  6. The 10 crafting stations thing is for several reasons. One, there isn't much space in the UI, and I seriously hate when buttons are tiny. Two, balance reasons; the player's meant to craft multiple crafting interfaces.
  7. This is good, I like it. However, there isn't much space in the UI, and I hate tiny buttons. I was planning to make some dropdown-menu-type thing to allow for more filters and sorting options.
  8. This honestly doesn't fit in with the whole concept of "easy storage".
  9. This is very good and needed a lot.
  10. This is also very good and much-needed.
  11. See 7.
  12. The shiny and new thing doesn't make much sense for storage. Someone earlier did suggest sorting by the most recent added though, maybe that could replace this.
  13. I personally hate clicking through mods buttons over and over and over again until I finally find the correct one, only to accidentally click away to the next one.
  14. What do you mean here?
  15. What do you mean here?
  16. I'll have to see how the UI looks; don't really have much time for testing.
  17. What do you mean here? Left click what?
  18. I could see this being useful to many people. Seems good to me.
  19. Same as 18.
  20. No, no, no, this mod is not meant to affect combat in any way. If people want to test out items they can use Cheat Sheet in a test world. Just no.
  21. Iirc the author of Item Checklist is already working on Magic Storage integrations. It would probably be more efficient to add Call support instead to make things easier.

blushiemagic avatar Aug 25 '18 17:08 blushiemagic

@blushiemagic ,

  1. It doesn't really make sense to remove sorting by quantity.

What are the use cases? I think only one - when you want to cleanup a storage and find small stacks.

  1. What is meant by new item here?
  2. This mod's meant to make storage easier, and keeping track of "new" recipes doesn't really fit in (especially on multiplayer). And not sure why it's necessary to remove available recipes.

New item is never crafted item AND not checked in ItemCheckList if it's installed. But both lists (all and new items) show only recipes that you can craft from ever obtained items (known through ItemCheckList). I'm not that type of player who knows every item. I installed multiple mods and I want to see what has became available for me with loot I just found so I can either go farm more or craft that thing and try it immediately. There are much more recipes that I can't craft (not knowing all ingredients) than which I can and they clutter the list a lot with multiple mods installed (check it with calamity+thorium+spirit). Also they are much ahead of my current progression (some of them are post moon lord!) and seeing them is a kind of a big spoiler. Especially for a player who likes exploration, finding new items and seeing what is possible to make from it (instead of going to wiki and planning). I don't like playing with wiki. I like that I can suddenly find some item that allows me to make new things I never heard about.

The 10 crafting stations thing is for several reasons. One, there isn't much space in the UI, and I seriously hate when buttons are tiny. Two, balance reasons; the player's meant to craft multiple crafting interfaces.

Oops, I didn't think of that. I thought only one crafting interface per storage heart.

I personally hate clicking through mods buttons over and over and over again until I finally find the correct one, only to accidentally click away to the next one.

Left click - next, right click - previous.

  1. Recipe item icon is displayed on top instead of "Selected recipe",

I mean that instead of text "Selected recipe" and item name it's displayed as item icon with tooltip (so can be opened in recipe browser by its hotkey).

  1. You can open ingredient crafting by right clicking it into recipe,

In the required items sheet you can right click on any to set it as selected recipe and craft right away.

  1. Right click on Deposit button (now named "Transfer All") to Restock,
  2. And Left Click to Quick Stack (with Alt to Deposit All),

It means that instead of Deposit button there is a Transfer button which does Quick Stock on left click and Restok on right click. You can also do alt+left click for the legacy "Desposit All" action.

No, no, no, this mod is not meant to affect combat in any way. If people want to test out items they can use Cheat Sheet in a test world. Just no.

When I have so many mods installed there are too many items to test and recording their names each time to check in test world is a nonsense. Often I test like 15-20 new weapons to choose from and it happens each few hours of gameplay. This is where the "new recipes" list is very handy - I can keep track of what to test. So I decided to make it possible in single player but to restrict so it can't be used to go further in game progression like kill a boss. The current way is multiplying incoming damage by 5 while any of such test items is present in inventory. Also limiting recipes to only known restricts testing possibilities so you can test only an item that you are able to craft.

  1. The shiny and new thing doesn't make much sense for storage. Someone earlier did suggest sorting by the most recent added though, maybe that could replace this.

ShinyAndNew allows to see which really new items (never deposited before) you found recently. Often I go home, deposit everything and go back to fight. After everything is done I want to see what I actually found. Also I added a filtering mode that shows only recent items and they are sorted by most recent added in default sorting mode.

  1. The gem costs are mostly there for balance reasons, and not something I'll change. (Maybe one day I might add things similar to Shadow Diamonds though.)

I couldn't use your mod until Skeletron. What's the point of it? I installed storage mod so I want to be able to store things there right from start! That was my reaction when I've seen the prices. You can keep prices the same but at least boost the start so a player don't need to play half of the game without the storage mod he installed to store things in.

4.You can (un)hide recipes with ctrl+left click,

It's a separate feature - recipe blacklisting. Use case - when I decide that some item become useless and I'm never going to craft it anymore in this playthrough. Of course it doesn't make any sense if your recipe list is clattered of things you can't craft so every time you have to search.

I hate tiny buttons, icons and text too so I play in lower resolution and everything is fine)

AqlaSolutions avatar Aug 25 '18 18:08 AqlaSolutions

@blushiemagic , just finished editing my previous message

AqlaSolutions avatar Aug 25 '18 18:08 AqlaSolutions

What are the use cases? I think only one - when you want to cleanup a storage and find small stacks.

Cleaning up storage is exactly the use case; I found myself doing it a ton back when I had time to play.

New item is never crafted item AND not checked in ItemCheckList if it's installed. But both lists (all and new items) show only recipes that you can craft from ever obtained items (known through ItemCheckList). I'm not that type of player who knows every item. I installed multiple mods and I want to see what has became available for me with loot I just found so I can either go farm more or craft that thing and try it immediately. There are much more recipes that I can't craft (not knowing all ingredients) than which I can and they clutter the list a lot with multiple mods installed (check it with calamity+thorium+spirit). Also they are much ahead of my current progression (some of them are post moon lord!) and seeing them is a kind of a big spoiler. Especially for a player who likes exploration, finding new items and seeing what is possible to make from it (instead of going to wiki and planning). I don't like playing with wiki. I like that I can suddenly find some item that allows me to make new things I never heard about.

I think this kind of stuff would fit in well in a different mod. Magic Storage is not Item Checklist; they are two separate mods. One meant for storage, the other meant for discovery and completionism. And I'm not sure how the new item system helps against preventing spoilers; if anything, it shows you only the spoilers.

Left click - next, right click - previous.

That still doesn't really fix the problem of having to click through mods a long time, wondering when you'll find what you're looking for. After some thinking though, I feel like a dropdown menu would work better than both the search bar and the clicking button.

I mean that instead of text "Selected recipe" and item name it's displayed as item icon with tooltip (so can be opened in recipe browser by its hotkey).

Hmm, I kinda like clarifying that the second window shows which recipe's selected. I do think showing the item in the second window is a good idea though.

In the required items sheet you can right click on any to set it as selected recipe and craft right away.

Ooh. I was actually planning on doing this with double-clicking. Right-click works too though.

It means that instead of Deposit button there is a Transfer button which does Quick Stock on left click and Restok on right click. You can also do alt+left click for the legacy "Desposit All" action.

I kinda like the idea of having restock. Though I am kinda worried about the efficiency of quick stock. I like having Deposit All as the default thing though, since usually I just want to deposit everything that isn't favorited anyways. I think the main problem would be communicating to the user which kind of clicking does what.

When I have so many mods installed there are too many items to test and recording their names each time to check in test world is a nonsense. Often I test like 15-20 new weapons to choose from and it happens each few hours of gameplay. This is where the "new recipes" list is very handy - I can keep track of what to test. So I decided to make it possible in single player but to restrict so it can't be used to go further in game progression like kill a boss. The current way is multiplying incoming damage by 5 while any of such test items is present in inventory. Also limiting recipes to only known restricts testing possibilities so you can test only an item that you are able to craft.

That doesn't change how absolutely game-breaking this is. Magic Storage should not allow the user to cheat. I'm not even sure how this is even related to Magic Storage. And I'm not sure how it would help either when you still need to find the item in the first place. This is the single thing I strongly dislike about this pull request.

It's a separate feature - recipe blacklisting. Use case - when I decide that some item become useless and I'm never going to craft it anymore in this playthrough. Of course it doesn't make any sense if your recipe list is clattered of things you can't craft so every time you have to search.

Hmm, this seems kinda nice. Though the recipe list being too cluttered to actually manage is a big problem.

I hate tiny buttons, icons and text too so I play in lower resolution and everything is fine)

The problem with lower resolutions is that you can't see the stuff happening around you, making boss fights and invasions much more difficult. And I already have a lower resolution than many people.

All in all, this still doesn't change the fact that having a single pull request for many different things makes things impossible to manage and destroys the chances of it being accepted.

blushiemagic avatar Aug 25 '18 20:08 blushiemagic

@blushiemagic

And I'm not sure how the new item system helps against preventing spoilers; if anything, it shows you only the spoilers.

It shows only items that you can craft from things you've found. If you need A and B to craft C then you can't see C until you discover both A and B (but you may need to find more of them to actually craft). It's not a spoiler. Another example - if chlorophyte is required to craft its weapons you don't know about them until you mine your first chlorophyte ore block. It also helps with the "recipe list being too cluttered to actually manage" problem.

I think the main problem would be communicating to the user which kind of clicking does what. Right, if I had more time I would make separate buttons with icons instead of text so that they take less place.

Quick stock is quite useful because you immediately see what new items you acquired though "recent items" filters is also suitable. Having recipe browser you can immediately check those items to find out what you can do with them (like with Guide NPC). You don't need to think whether you already had this item before or not.

That doesn't change how absolutely game-breaking this is. Magic Storage should not allow the user to cheat. I'm not even sure how this is even related to Magic Storage. And I'm not sure how it would help either when you still need to find the item in the first place. This is the single thing I strongly dislike about this pull request.

You don't have to find this exact item in the first place, only all its ingredients (or their ingredients). I agree, it's doesn't fit good for the storage mod. But it helps me a lot to deal with mod content.

All in all, this still doesn't change the fact that having a single pull request for many different things makes things impossible to manage and destroys the chances of it being accepted.

We are now deciding what features you consider suitable and then I'll check what I can do to revert things you don't like and split other things into separate PR and how much effort it would take. If it's a few hours or so I'll do it. At least I will try to fix the tab-spacing-line-endings issue and revert what you don't like.

AqlaSolutions avatar Aug 25 '18 21:08 AqlaSolutions

@blushiemagic, about cleaning up a storage - usually it's enough to filter for things that don't stack e.g. weapons and armor. Especially when throwing weapons are in a separate filter.

AqlaSolutions avatar Aug 25 '18 21:08 AqlaSolutions

I agree with @blushiemagic's assessment. I hope this doesn't get merged.

Some of these changes are nice background enhancements, like the improvements to the search bar (someone please add a "X" button to quickly clear search results, like this, a hidden "right-click on text field to clear it" that no one can see and just need to know about isn't good UI design) and more filters so I'm not looking at a crafting window with 2,000 weapons in it trying to find Bard weapons while playing Thorium (split Accessories and Armor, split weapon types, split placeables between walls, blocks, furniture, bars, etc) kind of stuff is sorely needed and doesn't affect balance. You went a little too far with it. I think blushie and other users (myself included) would be more comfortable with objective improvements instead of so many subjective ones.

GregariousJB avatar Aug 26 '18 00:08 GregariousJB

@blushiemagic, I fixed line endings and reverted all of these:

1.Sorting by Quantity replaced by sorting by item Value [sort by Price is still there!], 5.Gem cost is 2 times lowered, 6.30 crafting stations instead of 10, 8.Added DPS calculator to the crafting menu and to weapons tooltip (based on damage, defence and use time though it's not enough for accurate values), 13.Mods are selected with left-right click instead of searchbox typing, 20.You can freely take new weapons, tools, armor and equipment "for demo" but only once (it's not in the "new" list afterwise). Those items are removed on death and while you have them with you incoming damage is 5x multipled. Also they cost 0 coins if you try to sell them. Use right click on "Craft" button. The last feature is available only in single player. 21.There is a hot key to check whether you've already seen (if ItemCheckList is installed) an item and how much of it you have in latest opened storage,

Also reverted "Selected recipe" text but leaved the icon in

Recipe item icon is displayed on top instead of "Selected recipe",

What do you think, which other things really require separate pull request?

AqlaSolutions avatar Aug 26 '18 10:08 AqlaSolutions

@AqlaSolutions Rather than list the things that are removed, which forces us to read the first list you wrote and then compare it to the new list repeatedly, just updating the current list would be a little nicer. A new screenshot would be cool, too.

I went ahead and did it manually. Is this the updated list of features, then?

  1. Only recipes from known ingredients are shown (if ItemCheckList is installed), non-directly available recipes (through intermediate crafting) are also considered in this check, you can mark items as not new with right click on currently selected recipe when in the "Only new items" mode.
  2. Instead of "Available recipes" and "All recipes" buttons there are 3 new buttons: "New recipes" (showing never crafted recipes), "All known recipes" and "Hidden recipes"
  3. You can (un)hide recipes with ctrl+left click,
  4. Added sorting by DPS,
  5. Search text can be canceled with right click on text field,
  6. Arrow keys, home, end, delete work in search field,
  7. Added more filters for weapons and equipment,
  8. Added "Recent items" filter that shows last 100 deposited items which were not deposited in this heart before, they are shown with "shinyAndNew" decoration too,
  9. Recipe item icon is displayed on top along with "Selected recipe", (did I understand this right? -G)
  10. You can open ingredient crafting by right clicking it into recipe,
  11. Right click on Deposit button (now named "Transfer All") to Restock,
  12. And Left Click to Quick Stack (with Alt to Deposit All),
  13. Favorites support in storage and recipes (alt+click),
  14. A toggle to show only favorited items,

GregariousJB avatar Aug 26 '18 18:08 GregariousJB

@GregariousJB, @blushiemagic , I'll add the screenshot as soon as we agree with the list of changes. Yes, the list above is correct.

Also after checking the diffs there are some not previously mentioned small changes:

  1. Disabled shift+click to put crafting stations. It's easy to put it by one if there are only 10 of them. I often mistakenly shift+click inventory items trying to put them into a storage while the crafting window is opened.
  2. Changed default sort order to avoid using CompareRarity. Because Rarity is dependent on an item prefix and in a storage similar items become separate which looks not sorted. So for weapons the default is DPS sorting, for summons and ammo it's price.
  3. Closing storage and crafting window doesn't reset search and filters. When you have the way to quickly reset search text if you don't need it it's much better to by default see the last viewed thing when you open the window instead of searching same again and again.
  4. General code improvements: using LINQ OrderBy instead of including BTree class, some duplicated code is extracted, some switch-case were replaced with enum casting, some strong dependencies were replaced with delegate passing, enabled C# 6 compiler (because I used its features).

AqlaSolutions avatar Aug 26 '18 20:08 AqlaSolutions

Disabled shift+click to put crafting stations.

I'm not sure how this works so I might be wrong, but does doing this mean Shift+clicking crafting stations with Magic Storage open will now trash the crafting station? Shift+Click is the "send to trash" hotkey. Blushie might have added this intentionally to prevent players from accidentally deleting their crafting stations and other items.

GregariousJB avatar Aug 26 '18 21:08 GregariousJB

@GregariousJB , no, shift+clicking inventory items with opened crafting ui now does nothing. Trash hot key is overriden but no action is performed.

AqlaSolutions avatar Aug 26 '18 22:08 AqlaSolutions