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

Shared cooldown support #137

Merged
merged 3 commits into from
Jun 18, 2024
Merged

Shared cooldown support #137

merged 3 commits into from
Jun 18, 2024

Conversation

xpdota
Copy link
Collaborator

@xpdota xpdota commented Jun 16, 2024

Adds sharesCooldownWith field to Cooldown. If you want A, B, C, and D to have shared cooldowns, then B/C/D should have sharesCooldownWith: A.

The source and target cooldowns can have different CDs. If one has a 60s CD, and one has a 120s CD, then the cooldown time will be derived from whichever you actually use. That is, using the first skill would block both skills for 60s, and using the second would block both skills for 120s.

Behavior is undefined for abilities with more than one charge.

@xiashtra
Copy link
Contributor

xiashtra commented Jun 16, 2024

Adds sharesCooldownWith field to Cooldown. If you want A, B, C, and D to have shared cooldowns, then B/C/D should have sharesCooldownWith: A.

Just to clarify, the intention is that if A and B share a cooldown, B should reference A via sharesCooldownWith: A, but A should not reference B via sharesCooldownWith: B, because it would cause a circular-reference? This seems like an easy trap to fall into for sim implementers. Perhaps there is a better way to implement this that would prevent creating a footgun, possibly by using a hash of the shared cooldown abilities for the key instead.

@xpdota xpdota merged commit 5bdfa6f into master Jun 18, 2024
7 checks passed
@xpdota xpdota deleted the shared-cd-support branch July 3, 2024 15:34
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