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

Adding Boomkin Capabilities #103

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

chrislbs
Copy link

I figured I'd go ahead and open this PR to get some feedback if I'm going in the right direction. I'm interested in getting the boomkin simulator going as that's my main. For the most part, I've been trying to just follow the examples in the warlock talents. I was about to start working on updating the Druid class and wanted to get some feedback on how that should be handled.

@marisa-ashkandi

@chrislbs
Copy link
Author

I will continue to work on this. This is from a few hours worth of work. I just wanted to get a PR open so no one was potentially duplicating any of this effort. I'll continue to push to this branch and update the PR when new chunks of work are completed.

override val epBaseStat: SpecEpDelta = spellPowerBase
override val epStatDeltas: List<SpecEpDelta> = defaultCasterDeltas

override fun redSocketEp(deltas: Map<String, Double>): Double {
Copy link
Author

Choose a reason for hiding this comment

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

I wasn't sure what the purpose of these were actually. I just copied it straight from the warlock destruction spec.

Copy link
Owner

Choose a reason for hiding this comment

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

These are for EP calculation - each socket should have an implementation that reasonably estimates which stat would be put in that socket.

I believe boomies would generally use spelldamage in every socket, so the yellow could probably use that as a basis as well.

override val hidden: Boolean = true

override fun modifyStats(sp: SimParticipant): Stats {
// TODO: is it easier to do this than add the 2% hit on each spell call?
Copy link
Author

Choose a reason for hiding this comment

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

I wasn't sure how best to handle these types of things. It felt easier to just update the sims hit rating rather than do additional calculations per spell to add the additional hit percent. I was also trying to prevent confusion between using percents as 0 < pct < 1 and 0 < pct < 100

Copy link
Owner

Choose a reason for hiding this comment

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

It depends on the ability - many talents like this are spell-specific, so they would have to be limited. Since Balance of Power applies to all spells, implementing as generic hit rating is just fine!

Copy link
Owner

@secretbis secretbis left a comment

Choose a reason for hiding this comment

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

This is a really great start! I'll be happy to help finish up any integration with the web stuff or EPs/ranks once you have a good baseline!

@@ -117,6 +118,10 @@ class SimParticipant(val character: Character, val rotation: Rotation, val sim:
return this
}

inline fun <reified T: Talent> getTalent(talentName: String): T? {
Copy link
Owner

Choose a reason for hiding this comment

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

We can't use Kotlin reflection or reified types, since it won't translate to the javascript web build.

const val name = "Wrath of Cenarius"
}

override val name: String = Dreamstate.name
Copy link
Owner

Choose a reason for hiding this comment

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

Should this be Companion.name?


override fun modifyStats(sp: SimParticipant): Stats {
// TODO: is it easier to do this than add the 5% crit on each spell call?
return Stats(spellDamage= (increasedSpellDamagePercentByIntellect() * sp.spellDamage()).toInt())
Copy link
Owner

Choose a reason for hiding this comment

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

Should this be increasedSpellDamagePercentByIntellect() * sp.intellect()?

override val hidden: Boolean = true

override fun modifyStats(sp: SimParticipant): Stats {
// TODO: is it easier to do this than add the 5% crit on each spell call?
Copy link
Owner

Choose a reason for hiding this comment

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

Stale comment?

override val hidden: Boolean = true

override fun modifyStats(sp: SimParticipant): Stats {
// TODO: is it easier to do this than add the 5% crit on each spell call?
Copy link
Owner

Choose a reason for hiding this comment

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

Yep! Moonkin form applies to everything, so modeling as generic crit rating is fine!

const val name = "Moonfire"
}

// TODO: lookup this actual value
Copy link
Owner

Choose a reason for hiding this comment

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

Rank 12 is 26988

}

val baseDamage = Pair(305.0, 357.0)
val baseDot = 600.0
Copy link
Owner

Choose a reason for hiding this comment

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

We probably don't need this value in the main ability - it's covered in the DoT implementation

/**
*
*/
class NaturesReach(currentRank: Int) : Talent(currentRank) {
Copy link
Owner

Choose a reason for hiding this comment

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

The sim does not take any spell range into consideration, so this talent can probably be omitted.

/**
*
*/
class Vengeance(currentRank: Int) : Talent(currentRank) {
Copy link
Owner

Choose a reason for hiding this comment

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

@chrislbs
Copy link
Author

chrislbs commented Oct 5, 2021

Thanks for the feedback. I'll look into getting in some additional changes this weekend.

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