Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add deterministic ability ID generation to PermanentImpl.addAbility #13465

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jimga150
Copy link
Contributor

Fixes #11560
Does NOT fix #10372 - Most likely due to the Effect ID changing each call (CopyEffect.java:copyToPermanent)

image

Potentially an alternate solution to #10802

I'm open to suggestions about the methodology for generating the new UUID. I wanted to adhere to the following requirements:

  • must be able to handle using a single seed ID (in case the ability ID and sourceId are null) and output a different UUID from the input in that case
  • must not cycle in case of multiple chained addAbility calls (if you XOR UUIDs together, you'll wind up with A ^ B ^ A = B)
  • must be deterministic

I considered using RandomUtil, but converting multiple UUIDs into a single long for setSeed would reduce the seed space by a big factor and increase the probability of UUID collisions. I don't know if that's significant or not, but i didn't want to chance it with this change that has such widespread implications for the engine.

@jimga150 jimga150 marked this pull request as draft March 22, 2025 02:17
@jimga150
Copy link
Contributor Author

It's been a while since i've done a PR, but it looks like the failures in the build are just the oracle checks and rarity/ability hint verification, etc. Is there a true failure in the build? I thought the oracle checks were turned off for GitHub PRs?

@JayDi85
Copy link
Member

JayDi85 commented Mar 22, 2025

Verify test failed. It’s not related to PR. Mtgjson got update with fixed rarity in one of the new cards, so master start to fail.

IMG_1253
IMG_1254
IMG_1255

@jimga150 jimga150 marked this pull request as ready for review March 22, 2025 02:51
@jimga150 jimga150 changed the title Add deterministic ability gain to PermanentImpl.addAbility Add deterministic ability ID generation to PermanentImpl.addAbility Mar 22, 2025
@JayDi85 JayDi85 self-assigned this Mar 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tyvar the Bellicose ability not resolving for all Elves BUG // Propaganda asks to pay again infinitly
2 participants