CUIItemUpgrade implementation (anvil UI)#417
Conversation
|
Scroll check and other reviews solved. |
This ensures it's correct for the right frame and isn't unintentionally influenced between frames, e.g. when dragging over the top of another icon. Also fix bugs caused by implicit copies; we should just explicitly clone these as needed, rather than this though.
Rather than have iffy logic having to constantly try and figure out if it's a copy (to free) or a reference (to absolutely not free, but restore), we just always allocate a copy regardless. Additionally, we refactor packet handling to modify the inventory and then just reload the inventory state. This is simpler than trying to update 2 states (which it wasn't even doing, so they were easily out of state, especially with stack size changes). Finally, remove MouseProc() - this logic is redundant. It's handled by UIMSG_ICON_DOWN already.
(and consequently when restoring)
At this level, MouseProc() can trigger a little more generously, at least when we indicate work has been done (UI_MOUSEPROC_DONESOMETHING). This allows things to act a little smoother as it still behaves when dragged outside the UI.
It's unclear what exactly we want to copy here; we might want the icon, we might not. Implicit copy behaviour by default would copy the pointer rather than allocate a new instance, and then it wouldn't be clear which parent it should be assigned to. To be very clear about what we want, we explicitly add Clone() to fully clone the entire thing. Anything else can be handled manually, but it should be rare.
Don't want to touch tools. Too much of a hassle with clang-tidy for now. Really need to go over tools with clang-tidy so this is not an issue.
Fix sort order to be consistent with official; this should be descending, not ascending
Officially this will act as a 'move' and only show up in the requirement list. Since we're just cloning, to simulate this, we just selectively hide/show the icon as appropriate.
There was a problem hiding this comment.
Okay, I've gone through and tried to clean it up as best I could without changing too much.
That addresses a lot of the concerning leaks and potential use-after-free issues, as well as issues referencing the wrong item because it was shared by tooltip code.
Additionally, we add the requirement item sorting to enforce consistency, and generally try to do away with awkward code trying to be too abstract, causing confusion and bugs in the process (e.g. icon 'drop' handling), though I didn't go as hard on that as I'd have liked.
I made the cloning explicit, so it was very clear what was happening and that we could rely on it for icons (without having to do more work).
I refactored the icon management when handling a packet indicating a burned or upgraded item to effectively - for the most part - update the base inventory, and then copy it. The old implementation was excessively confusing and very much looked like it was entirely desyncing, especially when dealing with stacks. It seemed to be trying to make the changes to both the real inventory and the copied inventory, but also.... not actually doing so, really only modifying one side of it in odd conditions.
It's a lot more straightforward now.
Some other minor changes I made but really the brunt of it involved the icon management. That was the biggest issue here.
I think it looks good now, so I'm happy to merge this once @kenner2 also approves it.
twostars
changed the title
twostars
changed the title
On successful upgrades, on the next assignment, it should reset. When it burns, we shouldn't bother preserving the slot position as this item is deleted already, and it just blocks us from adding another. Also make this consistent across right-clicks and left-clicks.
3 participants
Please check the type of change your PR introduces:
What is the current behaviour?
-Item Upgrade not implemented
What is the new behaviour?
-The CUIItemUpgrade class has been implemented, and the UI behaviors work the same as in the original 1298 Client. Items can be upgraded correctly.
Why and how did I change this?
-The items in the ItemUpgrade window are copied from the inventory and backed up. When the window is closed or the Cancel button is pressed, the items are restored from the backup. A backup is used to prevent errors when switching between Inventory and ItemUpgrade. If the upgrade process succeeds or fails, the backup is updated accordingly.
Checklist