Professions

[julienp]

We don't have professions anywhere. They probably should be passed as a list of strings to Stats.

[Aldriana]

Is there a reason we need to pass them in separately instead of adding whatever passive stats they're getting to the stats total passed in and adding anything weird as a gear buff or proc? So far as I know we already everything but lifebloom, the tailoring cloak enchants, and a couple of engineering gismos already, we already have all the framework to support lifebloom and tailoring. I think all that's really missing right now is modeling for activated direct damage abilities (i.e. engineering bombs).

[julienp]

I was thinking of passing in the names of professions and have it set whatever is needed automatically, but that's actually quite annoying as different professions need to touch different parts. Just "manually" adding stats leaves open the possibility to configure a calculator that for example has the alchemy mixology bonus added but doesn't have a flask as buff. I'll admit that it's not a very useful use-case. I guess the UI will have an easier time taking care of those things. Feel free to close.

[Aldriana]

They touch different places, and some of them aren't really definable at all. I mean, what, in fact, is the benefit that Blacksmithing gives? I mean, the optimal thing to do is 2 agi gems, but sooner or later someone will want to assess it with haste gems there instead, so we can't just guess what the benefit will be. Hence, for any profession that modifies the enchants or gemming of your gear (which is most of them), the burden sort of does need to be on the caller.

That said: there are a few professions where that's not the case. In addition to the above things that need to be added, we should probably think a bit about the Mixology case because we are tracking flask stats separate from gear stats. We probably do need to define a gear buff for it and have it insert 80 Agi into the calculations at some point. Similarly, we won't get the crit bonus from skinning just by adding up the gear stats, so we need to figure out how we want to handle that.

So, in short: there are some things that still need to be addressed, so we should probably keep this issue open to track them; but I don't think its a case of just passing in a profession name, since for about half the professions that doesn't really help anything.

[julienp]

__init__ of the base DamageCalculator class could look like this: def init(self, stats, talents, glyphs, buffs, race, professions, settings=None, level=85): self.stats = stats self.talents = talents self.glyphs = glyphs self.buffs = buffs self.race = race self.settings = settings self.level = level self.professions = professions for prof in professions: if prof == "alchemy": if self.buffs.agi_flask: self.stats.agi += 80 elif prof == "mining": self.stats.stam += 120 elif prof == "herbalism": self.stats.gear_buffs.lifeblood = True elif prof == "skinning": self.stats.gear_buffs.master_of_anatomy = True elif prof not in self.allowedprofessions: raise InvalidProfessionException(("Invalid profession {prof}").format(prof=prof))

Other professions can be handled by the UI, allowing you to select 3 JC gems, adding blacksmithing gem sockets or activating profession specific enchants if you got the correct profession.

Alternatively, Lifeblood and Master of Anatomy can be fully handled by passing them as gearbuffs, but Mixology still won't fit in, at least not without special casing it somewhere outside of gear buffs. Edit: actually that's no different from how other gear buffs are handled anyway ...

[Aldriana]

Yeah, I think I'd lean towards just defining Lifeblood, Master of Anatomy, and Mixology as gear buffs and handling them piecemeal. We can later define wrappers that allow you to pass in a profession name if there's a demand for such things later, but for the moment I think its more consistent architecturally to define the buffs rather than the professions.

[Aldriana]

I think this is more or less taken care of for the moment, so closing.