hex-agon / mastering-mixology

BSD 2-Clause "Simplified" License
2 stars 14 forks source link

Add modifier and Digweed to inventory overlay #75

Closed whiitehead closed 1 month ago

whiitehead commented 1 month ago

I know this is a very large PR but it was just easy to do all of these improvements at the same time. However, I am open to breaking this PR up if you only want some of it.

Rationale

This PR makes inventory tags work for modified potions and with Digweed. It was clear that for this feature to work, there would need to be a virtual representation of the potions in the players inventory. Though maintaining this representation throughout the mini game adds some complexity, it was also able to improve functionality and reduce complexity in other parts areas. Fulfilling orders and highlight stations are now based on what is in the players inventory and are able to update on inventory change.

Strategy

The best strategy for points is to fulfill 3 orders at a time but the best strategy for exp for irons and broke mains is to make every type of potion except triples and then hand in multiple at a time while missing some orders. This PR supports that strategy by allowing you to see when you are not longer holding completed orders after handing in potions.

Features

2024-10-14_16-58-42

raiyni commented 1 month ago

I don't think it's worth our time to display information about stations/digweed if we have to track the information by hand. Moving potions will break it.

whiitehead commented 1 month ago

@raiyni test it out and see if moving potions breaks it. The code is very robust and well tested. I apologize that syncInventoryPotions is hard to read, I should probably add more comments.

raiyni commented 1 month ago

@raiyni test it out and see if moving potions breaks it. The code is very robust and well tested. I apologize that syncInventoryPotions is hard to read, I should probably add more comments.

@raiyni test it out and see if moving potions breaks it. The code is very robust and well tested. I apologize that syncInventoryPotions is hard to read, I should probably add more comments.

Drag two potions of top of the same type on each other, what happens?

raiyni commented 1 month ago

I just moved a potion in my inventory and the overlay is flickering back and forth between two potions of the same type.

raiyni commented 1 month ago

I just took a same type potion from the Mixing vessel and it auto assigned a table type to the overlay

whiitehead commented 1 month ago

I just moved a potion in my inventory and the overlay is flickering back and forth between two potions of the same type.

I'm struggling to repro. What two potions did you swap in your inventory? does it keep flickering or just flicker once or twice?

I just took a same type potion from the Mixing vessel and it auto assigned a table type to the overlay

Yeah, those potions should be unmodified. I'm also struggling to repro this. I'll get this fixed.

raiyni commented 1 month ago

I just put 1 potion in a station and it added an overlay to another potion I haven't touched

raiyni commented 1 month ago

https://github.com/user-attachments/assets/4256d244-39fa-4bcc-91a9-09509117f9cb

also lots of this


    at work.fking.masteringmixology.MasteringMixologyPlugin.syncInventoryPotions(MasteringMixologyPlugin.java:334)
    at work.fking.masteringmixology.MasteringMixologyPlugin.onItemContainerChanged(MasteringMixologyPlugin.java:301)
    at net.runelite.client.eventbus.EventBus$Subscriber.invoke(EventBus.java:70)
    at net.runelite.client.eventbus.EventBus.post(EventBus.java:223)
    at net.runelite.client.callback.Hooks.post(Hooks.java:202)
    at client.zl(client.java:52910)
    at client.nm(client.java:3536)
    at client.bj(client.java:1138)
    at ba.aw(ba.java:409)
    at ba.ri(ba.java)
    at ba.run(ba.java:25527)
    at java.base/java.lang.Thread.run(Thread.java:829)```
whiitehead commented 1 month ago

I'm not getting any of that. Our environments are different. Let me nuke my repo

raiyni commented 1 month ago

I just made 2 mixalots and 1 came out with ALA and digweed

image

whiitehead commented 1 month ago

Yeah just hold on. Our environments are different. The line from your call stack cannot produce a null ref on my machine. Just let me sort that out first.

raiyni commented 1 month ago

Somehow the highlight station is broken and not showing quick actions now

whiitehead commented 1 month ago

I know. Just chill for a moment. I'm trying to figure out why the version you have cloned is different than mine.

whiitehead commented 1 month ago

@raiyni It still works for me after re-cloning everything. Not sure if you are on your own fork or what but I cloned hex-agon/mastering-mixology then checked out pr/75 and everything runs fine.

raiyni commented 1 month ago

I have made no modifications to the code. The NPE lines up exactly the same in the PR as it does in intellij. Are you moving items around at all?

whiitehead commented 1 month ago

Yes, I'm doing exactly what you're doing in that video.

K, lets start with the call stack you're getting. Can you send me the entire call stack including the actual line of code it is coming from?

raiyni commented 1 month ago

It's an NPE on this line. https://github.com/hex-agon/mastering-mixology/pull/75/files#diff-cebe29c65df8176a2a8c3367faa504a44ab2aaf1e9c4e30355baa2d3893a8f31R334

whiitehead commented 1 month ago

K, I reproed it. It isn't a null ref, it is an index out of range exception. That line can't produce a null ref. Just send the entire call stack next time. It happens when you do two swaps with three different items in the same tick. I think I have a fix for it but I need to go to bed. I'll submit a revision tomorrow that also includes a few of your other suggestions. I know you don't like the idea of this PR but I do so I'm going to wait for Hex to reject it.

hex-agon commented 1 month ago

Hey, sadly i do not want to add any potion modifier/digweed tracking to the base plugin because, as you've probably noticed, it is not a trivial task to do and there's a lot of edge cases. The only way i'd accept this into the plugin is if jagex ever starts transmitting varobjs for items. See #69 #67 #59

Also this PR changes way too much, consider PRing smaller changes in the future.

whiitehead commented 1 month ago

Fair enough. Thanks for being polite about it. Is there anything from this you might be interested in? like the multicolor inventory tags?

hex-agon commented 1 month ago

I wouldn't be opposed to it as long as there is a off/white/colored option in the config.