Improve MagicStack stack removal logic and stack notifications via SwingWorkers#9850
Improve MagicStack stack removal logic and stack notifications via SwingWorkers#9850adrianrlaw wants to merge 4 commits intoCard-Forge:masterfrom
Conversation
adrianrlaw
force-pushed
the
magicstack-perf-enhancements
branch
from
9ca51ae to
d2cb1d0
Compare
| Runnable tryAgainThread = () -> notifyStackAddition(event); | ||
| GuiBase.getInterface().invokeInEdtLater(tryAgainThread); | ||
| // TODO: signalAll() may be unnecessary since we only increment in this worker. | ||
| updatedNotifiableStackIndex.signalAll(); // Notify threads waiting on updatedNotifiableStackIndex |
There was a problem hiding this comment.
this seems like so much work for such a basic thing:
what stops us from just storing each GameEvent in a List and then displaying the modals until it's empty again?
There was a problem hiding this comment.
I'm not sure why nextNotifiableStackIndex is being tracked but I'm assuming it's to help with ordering the modals (please let me know and correct me if I'm wrong). With this assumption in mind, sure we can probably keep each GameEvent in a List and iterate through it to find which GameEvent has a stack index that matches, but that sounds expensive and we'd have to wait on a GameEvent to be inserted into the list again anyways when it ends up empty (which also requires condition checks and locking to be thread safe if we want to avoid locking up the EDT thread). Also, with the previous implementation, we'd just re-queue our notifyStackAddition() to the back of the EDT queue recursively, which with my quick adventures with Google is supposedly discouraged. SwingWorker is supposed to already kinda do this on its own without hogging up EDT time. The ultimate goal here, I guess, is reducing the cpu time spent on checking nextNotifiableStackIndex. (Note: before coming up with these changes, I generated a flame graph to see where the slowdown occured. notifyStackAddition() was by far the biggest hog for some reason.)
That said, the most important change here is fixing the double-removal issue in MagicStack.
There was a problem hiding this comment.
On a side note, a function recursing over a condition without modifying it on its own (in the branch where it recurses) is very scary to me.
There was a problem hiding this comment.
if the goal is to always display the topmost stack entry you could just grab them from the end of the list 🤔
and when you receive a GameEvent while the list isn't empty the method should just return early (the previous call can just loop on those additional ones) 🤷♂️
There was a problem hiding this comment.
I'm not very experienced with Java, so forgive me for having difficulty envisioning the implementation. What happens when the stack grows to 100, 200, or maybe even 1000 (Storm is probably Wizard's funniest mistake)? Do we want to loop over a large list of game events looking for the one we want to show on the EDT thread or let each event handle itself (running and cleanup)? Also, what if Wizards cooks up something that changes the order of things resolving on the stack (not that they will but I can imagine them doing this, or maybe they already have)? How do we handle cancellation?
There was a problem hiding this comment.
hmn, I admit to never really having used this feature, so maybe I misunderstood how it works 😅
when the stack gets filled with multiple entries is it supposed to only show you a modal for the top - and only when that leaves the stack you'd get one for the next below etc.?
There was a problem hiding this comment.
I think I'm starting to understand what this section of code is trying to do but I may be reading this wrong. So ya know when there's like a trigger that's supposed to reveal some sort of information when it goes on the stack or when the AI is supposed to reveal or make public some sort of game object as part of a choice or additional cost? This part of the code is supposed to catch and handle that and notify the player that some sort of information was revealed. Think AI sacrifices a facedown card as part of an additional cost sort of thing.
There was a problem hiding this comment.
not really, we have other ways to reveal stuff?
Nobody has worked on this feature in a long time, it doesn't even exist on mobile and tbh personally I don't see the point of showing a huge window with redundant information but I thought you were finding it useful :P
If you don't even have its preference enabled but it was still causing significant CPU time something is seriously wrong...
There was a problem hiding this comment.
the double removal in MagicStack was probably the biggest problem tbh...
notifyStackAddition only runs when nextNotifiableStackIndex is equal to the event's stackIndex. Instead of recursively putting notifyStackAddition back into the edt queue, why not have it wait for nextNotifiableStackIndex to match in a SwingWorker.
adrianrlaw
force-pushed
the
magicstack-perf-enhancements
branch
from
5aba8d6 to
4903ee0
Compare
2 participants
As a combo player, spamming spells/abilities and abusing the stack is my MO. Taking this playstyle to Forge however has caused some noticeable performance problems that I suspected to be programmatic.
This PR aims to provide two sets of improvements:
1) Centralize stack instance removal in
MagicStack.2) Rework stack notification UI behavior to be more performant.
notifyStackAddition()intoSwingUtilities.invokeLater().