Aldriana / ShadowCraft-Engine

Calculations backend for ShadowCraft, a WoW theorycraft project.
GNU Lesser General Public License v3.0
37 stars 22 forks source link

Detailed EP #73

Closed dazer closed 13 years ago

dazer commented 13 years ago

I'm requesting pull so you see how this is going. If you want to pull the package changes before this I can easily issue new commits/request.

Each section manages its calculations differently; I'm going to explain them a bit so you can review them comfortably:

I tried to build them avoiding nasty code at all costs and I think it is very compact but readable now; I don't know if the methods I set in place to create/delete objects or nested getattr are what they should look like though.

Only assassination.py computes those in my commit. It'd be great if you told me what should go in and what not; glyphs, for instance, is not very usefull for us: I put it there so you can see it in action, but it probably will not make it to the final cut (I mean the usage, the method looks fine to me).

dazer commented 13 years ago

I let go of del/set glyph as well as del_proc. However, the other set methods (set_enchant and set_proc) would require to build a proc/enchant object calcs side, which I find odd. I could easily deprecate del_enchant but I think it helps readability getting rid of a couple 'for' loops.

dazer commented 13 years ago

Naming the procs list to be analyzed as 'procs' was a bad idea; it kept me from fully understanding what julienp was trying to adress.

On the set/del methods, I feel content with the three remaining, but if someone thinks they should go I'll happily refactor them.

edited the 'op' to better reflect the changes made.

Aldriana commented 13 years ago

Is this ready to be pulled, or does it still need work?

dazer commented 13 years ago

The methods are ready, but the combat/subtlety scripts are not calling them yet; assassination.py calls them all. You can either tell me what do you find usefull to be computed there or simply pull and tweak them as you see fit.

Aldriana commented 13 years ago

Personally I think the assassination/combat/subtlety scripts are getting a bit bloated - I'm not entirely sure to do with it, but when I'm doing heavy-duty debugging I frequently find myself commenting out significant sections of them as it is. I suppose a partial solution would be to throw some command line parsing on to the scripts and add some options for which numbers you want displayed, but realistically I think the primary use of this sort of function will be via the UI - either raconzor's test UI, or the one being built over at shadowcraft-ui.

In any event, so long as the feature is working I'm inclined to pull it - we can worry about what to do with it where later.