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

Set affecting skill from context menu #101

Closed
blitzmann opened this issue May 21, 2014 · 9 comments
Closed

Set affecting skill from context menu #101

blitzmann opened this issue May 21, 2014 · 9 comments
Labels
enhancement This is a feature request, or an idea to enhancement a current feature

Comments

@blitzmann
Copy link
Collaborator

Affecting skills are already collected per module, figure out a way to tap into this and access via module / ship context menu.

@DarkFenX
Copy link
Member

We have bigger problem here:

  1. Skills which do not affect at the moment should be in list (e.g. when skill is absent it won't be shown in list of affectors)
  2. List should have all skills which affect given module, even when indirectly through other modules

This is pretty advanced functionality, so i am unsure if it can be implemented in current eos. I have most of functionality to support it in new (fast affector/affectee tree traversal & fit ghosting).

@blitzmann
Copy link
Collaborator Author

Granted, this kind of feature might not be able to accommodate cases like those without significant reworking (in which case, it's probably not worth it with the current codebase). However, IIRC, EFT also does not take those cases into account. Not saying Pyfa should be like EFT, however users that come from EFT to Pyfa will be able to experience at least the same functionality, so I believe even having the limited functionality of currently affecting skills will be a benefit.

Fake edit: I just checked, and it does seem that EFT includes skills which are not trained yet. Hmm... I'll have to look at the character classes and see how these things are applied.

I haven't looked at it yet, but when a skill is set to lvl0 or even "Not Learned", are they removed from the skill list of the character entirely? Would it be as simple as including those in the skill list of the character? That way the show up in the affector list. And then to reduce problems and help debug, we can possible remove any lvl0 skills from the items stats > Affected by window so that we show what is actually affecting it, rather than skills that are lvl0 and could affect it.

@blitzmann
Copy link
Collaborator Author

I've started work on this feature: blitzmann@45fca2a

It goes a bit deeper than I thought, but is a relatively easy thing.

Each module in a fit goes through and finds any other item/skill/etc that is affecting it and adds it to a map. However, before it adds it to the map, it checks to see if it even changed the value at all. If not, then it doesn't get added. This is how we only show currently affecting skills / items in the "Affected By" tab - if we have a skill that is not trained, then it's not affecting the item and isn't added.

I changed this by moving that conditional to a parameter in ModifiedAttributeDict.__afflict(). This way, the modifier is added to the map, and this new flag, used, can be used to determine if it is currently affecting the item.

This makes it possible to have a list of skills for an item that can affect it and display them, but also allow us through the use of the used flag to not display them (eg: to keep the "Affected By" tab as it current is now)

As far as I'm aware this doesn't have any penalties in regards to performance/bugs, and I've confirmed that I am showing the same stats between this change and Stable, so I consider the backend support for this feature is more or less completed. I now just need to make the context menu. This is also a change that I will submit pull request for as it deals with a few EOS changes.

EDIT: I lied about backend being completed. There still seems be issues with pulling all the skills affecting it.

@blitzmann blitzmann reopened this May 25, 2014
@blitzmann
Copy link
Collaborator Author

Just pushed recent changes to my repo.

As stated at the bottom of my last comment, my solution of including all affecting skills, whether or not they are currently being applied, doesn't seem as robust as I believed. My recent changes simply remove the check to see if the skill does anything to the attribute in question and includes it in the modifiers list regardless. However, this seems like the initial list of modifiers is not including everything a level 5 character might. I am still unsure how Pyfa gathers it's initial modifier list.

Pictures for demonstrating between level 0 character and level 5 character: (not that before my changes, level 0 wouldn't even show that as no skill modifiers would have been added to the module)

All 0 Copy character:
all0

All 5 character (showing more skills)(also, skill level icons are placeholders):
all5

Otherwise, the actual implementation of skill changing works great and much the same way as EFT does it. Select the skill, and it updates the character and the fit.

@blitzmann
Copy link
Collaborator Author

I made some progress as to why some skills aren't showing up in lvl0 characters vs lvl5.

I should clarify: lvl0 character is actually a character with all skills set to "Not Learned", not level 0. Any skill that has a level (including 0) shows up properly in the list as expected.

The problem seems to stem from the way skills are saved in the database for each character. It seems that a new character starts with a blank slate, and it seems that skills are added "as needed" ie when you change a skill's level. This feature iterates through the skills that are pulled from the database - if the character doesn't have a skill (was not previously set), the logic doesn't know about it, and thus it doesn't show up.

Likewise, the "All 0" character that I was using only has something like 279 skills in the database (hence why some of them are showing up, and some of them aren't). As a comparison, "All 5" has 399 skills. It seems that new characters and the default "All 0" character save skills as needed (the functionality of which I haven't figured out yet)

So the fix is simple - make sure the characters have all skills attached to them. I'll make a pull request for this feature tomorrow if testing passes.

@blitzmann
Copy link
Collaborator Author

I'm going to reopen this issue so that I don't forget about it again. The code to support this feature is in the pyfa codebase, however there is an issue with it. Skills that affect ship bonuses are not included in the "Affectors" list. It was decided to disable this feature until it's sorted out (or determined to not bother fixing)

Take the wolf for example:
Assault Frigate Bonus per skill level

  • 10% bous to Small Projectile Turret falloff
  • 5% bonus to Small Projectile Turret damage

Minmatar Frigate bonus per skill level

  • 5% bonus to Small Projectile Turret damage
  • 7.5 bonus to Small Projectile Turret tracking speed

(also, wtf, why can't I copy this from pyfa? Furthermore, why can't I find traits online to c&p?)

One would expect these to show up on the affecting skills list of small projectile turrets, but they do not:
untitled

Nor does it show up in the affector list of the shup hull.

it's because pyfa handles skill effects weird. From what I understand (which isn't much), the hull takes the skill level and simply adjusts itself - the hull never sees the skill. This modified ship bonus is then applied to the modules in question.

It might not be possible to show these skills for the module, but at least try to get them to show for the ship hull.

@blitzmann blitzmann reopened this Sep 4, 2014
@DarkFenX
Copy link
Member

DarkFenX commented Sep 9, 2014

Yes, EVE handles these effects in a more straight-forward manner. I think we can't do much about it atm. In best case - write skill name as a module-level attribute (like context and state, for example), and process it.

Or just wait for new eos arrival at soem point.

@blitzmann
Copy link
Collaborator Author

With my work on affected by tab, decided to look at this again. Immediately realized that we could do something that doesn't overhaul the whole system.

The issue is basically that, during calculation, the ship hull itself is registered as the affector, and then in the effect file the skill is used to modify the bonus. Continuing with the flow, the registered affector (ship) is then read from the fit object and inserted into the afffectors list.

The nice thing here is the way it's set up. After we register the affector, we can override it at any time before it's added to the affector list. We simply do fit.register(skillItem) and that's it.

I developed a new parameter for effect code, skill. It takes the skill name used to modify the ship bonus, registers it, and then does the modifications in modifiedAttributeDict rather than the effect file. So, instead of

def handler(fit, ship, context):
    level = fit.character.getSkill("Caldari Cruiser").level
    fit.modules.filteredItemBoost(lambda mod: mod.item.group.name == "ECM",
                              "falloff", ship.getModifiedItemAttr("shipBonusCC2") * level)

We have this:

def handler(fit, ship, context):
    fit.modules.filteredItemBoost(lambda mod: mod.item.group.name == "ECM",
                              "falloff", ship.getModifiedItemAttr("shipBonusCC2"), skill="Caldari Cruiser")

The skill modifies the modifier with *= as that's the only operation currently in use when applying skills with ship bonuses. If for some reason an expansion makes a ship bonus that does not follow this rule, we can revert to the previous format as needed with a minor tweak:

def handler(fit, ship, context):
    skill = fit.character.getSkill("Caldari Cruiser")
    fit.register(skill)
    fit.modules.filteredItemBoost(lambda mod: mod.item.group.name == "ECM",
                              "falloff", ship.getModifiedItemAttr("shipBonusCC2") OPERATION skill.level)

I'll be making a new prerelease to see if anyone in the community can find something I've broken with this before merging it into the main branch.

untitled

@blitzmann
Copy link
Collaborator Author

Finally get to close this. 01db1ef

w9jds pushed a commit to w9jds/Pyfa that referenced this issue Aug 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is a feature request, or an idea to enhancement a current feature
Projects
None yet
Development

No branches or pull requests

2 participants