Skip to content

Conversation

@Eradev
Copy link
Contributor

@Eradev Eradev commented Oct 13, 2025

I've played against the Nightmare deck, and it was very laggy as it tried to count its Swamps over and over again. This should alleviate the problem by caching some expensive queries, which are cleared every time the zones are changed. Even if it happens often, it will still only calculate it once per reset rather than 3-4 times.

What do you think?

@Hanmac
Copy link
Contributor

Hanmac commented Oct 13, 2025

Calculate Amount needs some refactoring anyway because right now you need to do "put into cache" at multiple locations

Might refactor it into a Supplier with computeIfAbsent

@Hanmac Hanmac closed this Oct 13, 2025
@Hanmac Hanmac reopened this Oct 13, 2025
}

protected void onChanged() {
game.resetCache();
Copy link
Contributor

@tool4ever tool4ever Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this will work
while zone change is one of the more common ways for the board state to change there are just too many others, e.g. using Orcish Farmer (though technically we might see this one because it already uses effect card)
by the time you're done hooking this up to all possible places I'd expect you end up with way more than 100 calls

Copy link
Contributor Author

@Eradev Eradev Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that tapping does trigger a change as well.

EDIT: Now it does.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After testing, Zone already did trigger when tapping. Both the Battlefield and Graveyard apparently.

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmn I see
so reset will happen every mainLoopStep at least

@Eradev
Copy link
Contributor Author

Eradev commented Oct 13, 2025

Calculate Amount needs some refactoring anyway because right now you need to do "put into cache" at multiple locations

Might refactor it into a Supplier with computeIfAbsent

There are some instances you don't want to cache (like the random one) so that's why it is targeted ATM. I could add a utility function to prevent some code repeat.

@Hanmac
Copy link
Contributor

Hanmac commented Oct 13, 2025

While looking at #8842 i noticed, that Player Properties can't be chained endlessly in xCount

it really didn't like this, what i tried:
PlayerCountPropertyYou$DungeonsCompleted$DifferentCardNames/Twice

Also for another issue, we need to think about how stuff like /Twice or /Min are applied.

@Eradev
Copy link
Contributor Author

Eradev commented Oct 13, 2025

As it is right now, it mostly helps with decks with multiple cards that checks for the same values, or have copies of itself (such as Relentless Rats decks). But I do see multiple cache hits when playing a single copy of Wight of Precinct Six.

I thought about invalidating cache values depending on the updated zone(s), but I'm not sure about the added complexity.

@tool4ever
Copy link
Contributor

your assumption of same string = same value is on shaky ground :P
what about something like Acidic Soil with:
SVar:X:Count$Valid Land.RememberedPlayerCtrl

@Eradev
Copy link
Contributor Author

Eradev commented Oct 13, 2025

your assumption of same string = same value is on shaky ground :P what about something like Acidic Soil with: SVar:X:Count$Valid Land.RememberedPlayerCtrl

I don't think it'll matter for spells as they'll trigger cache reset when the stack gets changed.

But I see your point that remembered player can be different. I'll see if I can do anything about it.

@github-actions
Copy link

This PR has not been updated in a while nad has been marked on stale. Stale PRs will be auto closed

@Hanmac Hanmac added keep no stale and removed no-pr-activity labels Nov 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

keep no stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants