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

Fix block to use new phases #120

Closed
JonaLam opened this issue Aug 6, 2024 · 3 comments · Fixed by #125
Closed

Fix block to use new phases #120

JonaLam opened this issue Aug 6, 2024 · 3 comments · Fixed by #125
Assignees
Labels
4-Priority: critical bug Something isn't working confirmed Has been approved to make code based on this

Comments

@JonaLam
Copy link
Collaborator

JonaLam commented Aug 6, 2024

Is your amelioration related to a problem? Please describe.

The way block is removed at the start of the turn is with a temporary signal in the phase manager.
That signal should be removed and the block should be removed in a newly implemented remove block phase

Additional context

#114 needs to be done
#118 needs to be done

@JonaLam JonaLam added the enhancement Ask for changes to an existing feature label Aug 6, 2024
@Turtyo Turtyo added bug Something isn't working and removed enhancement Ask for changes to an existing feature labels Aug 6, 2024
@Turtyo
Copy link
Collaborator

Turtyo commented Aug 6, 2024

I would consider that a bug since it doesn't behave as we would like it

This was referenced Aug 6, 2024
@Turtyo
Copy link
Collaborator

Turtyo commented Aug 12, 2024

Once #124 is merged, I think the plan would be to add two block remove phases (one for player, one for enemies). When on this phase, we call the block_reset of health_component

Or we can do it via signal, but it's harder to debug than direct function calls I think (because with direct calls you have a clear stack trace)

@Turtyo
Copy link
Collaborator

Turtyo commented Aug 17, 2024

Okay, now that #124 is merged, I'm thinking of this solution:

  • Add two new phases: reset_block_ally and reset_block_enemy
  • In the battler:
    • reset_block_ally: reset the block of the player by calling block_reset for the health_component of the player
    • reset_block_enemy: reset the block of all the enemies in the same way

After each phase we call the advance_to_next_combat_phase to go to next phase
I'm thinking of putting the reset for ally just before the player turn (so effects like poison still damage the player). And in a similar fashion for the enemies

I'm also thinking that we might want to separate the effects of start and end of turn into their own phase. This way if later we want to move stuff around, it's easier. That way, the player phase is reserved for player interaction with cards, and everything that is automatic (effects triggering) is in its own phase

Later if we decide we want the block to last until after the turn start effects (thus meaning we can block poison for example), we just need to exchange the position of the remove block and the effect of the turn start (which I don't think we can do currently, as everything is in the player turn for that matter)

I'll take the issue because I'm free for the next days so I can do it quickly, I'll wait for approval on the solution first ofc

@Turtyo Turtyo self-assigned this Aug 17, 2024
@JonaLam JonaLam added the confirmed Has been approved to make code based on this label Aug 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4-Priority: critical bug Something isn't working confirmed Has been approved to make code based on this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants