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

Class structure for the cards #27

Merged
merged 19 commits into from
Dec 23, 2023

Conversation

Turtyo
Copy link
Collaborator

@Turtyo Turtyo commented Dec 18, 2023

Description

This PR rewrites the system that was in CardBase to have a more flexible system of effects that can be added to a card.

Related issue(s)

Closes #14

List of changes

  • Creates EffectBase and EffectData: EffectBase is parent of all effects. EffectData is the actual thing that is useful for the card, as it contains the effect, the caster, the target and the value useful to the effect (damage to deal, card to draw, number of poison status to apply...)
  • Moves functions that were in CardBase to their respective effect
  • Rewrites the way damage is dealt to multiple targets by providing a list of targets (eventually reduced to only one element) instead of relying on boolean checks inside functions. This list of targets should be created at card creation (ie at the start of combat). It should be updated once an enemy dies during the combat. Creation will be done by the card parser (TODO), and the update is to discuss in further issues.

Tests

No new tests were provided, but #26 should be addressed once this PR closes.

Additional notes

In Draft status so the code can be reviewed. I need to test in the actual player that everything is working correctly, but I need to edit the cards that are given to the player in that scene to be sure (which I don't have time to do for a bit of time).

I am also not sure of what to do with effects that deal damage indirectly like poison. Currently those status don't seem to directly deal damage, as the only thing we do is add or remove them from the list of current status. Changes will probably need to be made to those status to inflict damage (either by calling the EffectDamage or directly calling the deal_damage function, which seems more logical as effects are the player interface and it's a bit too much to call them for internal logic).

@Turtyo Turtyo added the new feature Ask for a new feature label Dec 18, 2023
@Turtyo
Copy link
Collaborator Author

Turtyo commented Dec 19, 2023

Will make the cards that are played using the new structure and test that it works then it'll be good for merge

@Turtyo
Copy link
Collaborator Author

Turtyo commented Dec 19, 2023

Alright here is the problem:
If I want to be able to modify the list of target entity from the editor, I need to export them. But since entities are Node, EffectData should extend from a Node and not a Resource.
So either I don't export and we don't modify from the inspector, or i export but I need to make EffectData a Node a and not a Resource.

A solution would be to export the unique identifier of each entity. So we would have an array of strings for CardBase, with each string being the entity identifier. So we would have a get_instance_id in the Entity.

@cheesefrycook
Copy link
Collaborator

We probably don’t want to assign the targets in the inspector, because that list can change at runtime. Can we somehow pass the list of enemies to the effect data at runtime when they’re needed? Maybe from Battler when the card is played?

@Turtyo
Copy link
Collaborator Author

Turtyo commented Dec 19, 2023

Or I just export everything except the entity. That works too, since there are static data (like the amount of damage for the base card). The thing is that we have damage modifier. So the base damage won't be modified but we will need a way to take into account the modified damage. That will probably come along with the Dictionary of modifiers though.

@Turtyo
Copy link
Collaborator Author

Turtyo commented Dec 21, 2023

Note that damage all doesn't work anymore due to the targeting system. We will need to treat that at the same time as instant cast cards (by implementing the "all / target" system that was discussed in #21 )

@Turtyo
Copy link
Collaborator Author

Turtyo commented Dec 21, 2023

Also I noticed a bug on the draw, which can't be fixed in the scope of the current PR (I think) so i'll open an issue for that

It worked nicely. This card probably won't stay as it is, that would be OP
But it's nice to test that the system is working
@Turtyo Turtyo marked this pull request as ready for review December 21, 2023 01:17
@Turtyo Turtyo self-assigned this Dec 21, 2023
Copy link
Collaborator

@Tysterman74 Tysterman74 left a comment

Choose a reason for hiding this comment

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

Overall changes look good. I have a question though: what's the difference between a Status and an Effect?

Cards/CardBase.gd Outdated Show resolved Hide resolved
Cards/CardBase.gd Outdated Show resolved Hide resolved
Cards/Effects/EffectDamageHealth.gd Show resolved Hide resolved
Tests/test_cards.gd Outdated Show resolved Hide resolved
Cards/Effects/EffectDamage.gd Show resolved Hide resolved
@Turtyo
Copy link
Collaborator Author

Turtyo commented Dec 21, 2023

The difference between a status and an effect is that the effect is the action (like dealing damage, applying poison)
The status is a condition on the character that provides either buff or debuff (weakness, strength, poison).
If we talk a bit more about poison, the poison effect is the thing that applies the poison, the status poison is the condition that gets inflicted to the character and that ends up dealing damage each turn.

@Turtyo
Copy link
Collaborator Author

Turtyo commented Dec 21, 2023

I will let you resolve the conversations if you think the changes are correct @Tysterman74

@Turtyo Turtyo mentioned this pull request Dec 21, 2023
@Turtyo Turtyo merged commit 2e35e02 into Saplings-Projects:main Dec 23, 2023
1 check passed
@Turtyo Turtyo deleted the 14-card_structure branch December 23, 2023 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature Ask for a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Class structure for the cards
4 participants