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

Adds Primed Thrusters upgrade #853

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

rub8n
Copy link

@rub8n rub8n commented Apr 24, 2018

No description provided.

@Sandrem
Copy link
Owner

Sandrem commented Apr 24, 2018

Hi. We have Slack workspace for developers.
You are welcome!

Invite link is deactivated. Tell me if you want to get invite again.

@Sandrem
Copy link
Owner

Sandrem commented Apr 24, 2018

  1. Why "Movement.ManeuverColor.Red" is checked? Can this check be removed?
    Please, check situation where ship has 3 stress tokens and them performs green maneuver.
    (You can use console command "tokens assign" for testing)

  2. I don't like that you unsibscribe from Rules.StressRule. You can cause bug in another upgrades like Chopper crew. (I don't remember any rebel ship with tech and crew, but it can be added in future.)
    Maybe it is better to use "action.CanBePerformedWhileStressed" for BR/Boost?

  3. You call "Ship can perform free Boost or Barrel Roll". This is different from card's text and maybe can cause unknown problems.

Overall, I like quality of code, but I think that we can use more elegant solution.
So, I propose to use event "Phases.BeforeActionSubPhaseStart" to check number of stress tokens and set "action.CanBePerformedWhileStressed" to "true". In this case they will be shown in usual "Choose action to perform" window, and you don't need to unsubscribe from rules / call "perfrom free action" method.
If you have questions - contact me on Slack or here.

@rub8n
Copy link
Author

rub8n commented Apr 25, 2018

Sounds good. I'll take a look at your suggestions and send an update in the next few days.

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.

2 participants