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

Update Card.IsRelateToEffect #450

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

Conversation

Yuzurisa
Copy link

The effect relation check should work in current chain resolving only, rather than any possible chains.
This solves Fluorohydride/ygopro#2389 , without changing any scripts.

The effect relation check should work in current chain resolving only, rather than any possible chains.
This solves Fluorohydride/ygopro#2389 , without changing any scripts.
@mercury233
Copy link
Collaborator

it will break this kind of check in condition and target

@Yuzurisa
Copy link
Author

In fact we have to tell what exactly a relation to an effect is.
Relations are defined to establish between a card and a chain, rather than an effect. Mostly we used Card.IsRelateToEffect in effect operations to check if a card is related to a specific effect activated in any current chain, rather than any chains to be triggered. In fact the reason why Fluorohydride/ygopro#2389 occurred was, we in the past got the definition of Card.IsRelateToEffect mixed together.

it will break this kind of check in condition and target

Those checks are actually another meaning of Card.IsRelateToEffect that we don't often use, which is to check whether a relation exists between a specific effect and any chains either resolving or to be triggered. In such cases, we may either add another param to Card.IsRelateToEffect, or create another function like Card.IsRelateToAnyEffect, to separate the definitions of those two different use.

@Yuzurisa
Copy link
Author

What's more, I have checked every use of Card.IsRelateToEffect in ygopro-scripts inside condition or targets, while all of them are in EVENT_CHAINING, EVENT_CHAIN_SOLVING, or EVENT_CHAIN_SOLVED checks, in which relations are formed already. So it wouldn't break when checking conditions or targets.
I also tested cards such as Confronting the "C", in which Card.IsRelateToEffect is used in continuous checks. They also worked well, without using the 3rd param.

@edo9300
Copy link
Contributor

edo9300 commented Jun 15, 2022

The issue here lies in the fact that the Effect object (which is unique and shared, like a Singleton) is being used all over the place to store individual properties tied to other factors, from the card relation which should be tied to the chain instead, the labels that could easily overlap if the same effect is used multiple times in the same chain, and other things

@Yuzurisa
Copy link
Author

Yuzurisa commented Jun 15, 2022

@edo9300 in such cases checking relations to chain, use Card.IsRelatedToChain instead.
In addition, I believe over 80% of the scripts are written in wrong way. e:GetHandler():IsRelateToEffect(e) should actually be e:GetHandler():IsRelateToChain(0) instead. For some EVENT_CHAINING effects, re:GetHandler():IsRelateToEffect(re) should be re:GetHandler():IsRelateToChain(ev). Such mistake should be spotted out in advance. Just those maintainers copy out from other similar script, without thinking how it worked in detail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants