pyfa-org / eos

Eos - library for modeling EVE online ship fits
GNU Lesser General Public License v3.0
60 stars 19 forks source link

Skills Require Sub-requirements Incorrectly #16

Closed DarkFenX closed 7 years ago

DarkFenX commented 8 years ago

Posted by @Ebag333

Simple example. Take a Rig (which has other issues with skills, but that's another issue, literally).

Basic rigs skill requirement looks something like this:

Jury Rigging I
       Mechanics III

In order to train Jury Rigging I, you need to have Mechanics III.

But what if a character did have Jury Rigging I and somehow never trained Mechanics III? Okay, this isn't possible for rigs but it is possible for a number of ships/modules that CCP has reworked, the most prominent being Command Ships. There are many pilots who can fly a command ship, but cannot run links (soon to be bursts), and links are a prerequisite to command ships.

For those pilots, Eos will currently throw an exception, when it's actually a completely legal fit.

Take this code:

from eos import *
from eos.holder_filter import *

data_handler = JsonDataHandler('C:\Temp\PhobosJSONDump\TQ')  # Folder with Phobos data dump
cache_handler = JsonCacheHandler('C:\Temp\PhobosJSONDump\EosCache\eos_tq.json.bz2')
SourceManager.add('tiamat', data_handler, cache_handler, make_default=True)

fit = Fit()

#fit.skills.add(Skill(26252, level=1)) # Jury Rigging I
#fit.skills.add(Skill(3392, level=3)) # Mechanics III

fit.ship = Ship(40340)  # Upwell Palantine Keepstar
fit.rigs.equip(Rig(37275))  # Standup XL-Set Extinction Level Weapons Suite II

fit.validate()
pass

As it stands, this code will throw an exception that you're missing skill 26252, as it should.

Uncomment the line with 26252. Eos will throw an exception that you are missing skill 3392. Which it should not.

Uncomment both lines and Eos is happy.

This should be a fairly straight forward (hopefully) change to make the validation not recursive on ships/modules/rigs/etc.

DarkFenX commented 8 years ago

Uncomment the line with 26252. Eos will throw an exception that you are missing skill 3392. Which it should not.

Consider all the info eos provides to you. Besides telling which skills are missing, it shows you which holder is erroneous.

When module is missing its immediate skill requirement, error for that module will be thrown with missing skill. However, when immediate requirement of module is satisfied but skill is missing another skill requirement, error will be thrown for that skill which has requirement missing. If you wish to support such edge-cases, simply ignore any skill requirement errors for Skill holders.

DarkFenX commented 8 years ago

Posted by @Ebag333

When module is missing its immediate skill requirement, error for that module will be thrown with missing skill. However, when immediate requirement of module is satisfied but skill is missing another skill requirement, error will be thrown for that skill which has requirement missing. If you wish to support such edge-cases, simply ignore any skill requirement errors for Skill holders.

I understand what you're getting at, and you have a valid point.

The problem is that this isn't an edge case. The standard case is to not require the sub-reqs, there are no edge cases where they are required (there's an edge case for rigs where skill reqs aren't required at all).

Okay, so we have two things going on here.

1) Fitting validity - prereqs aren't required, and shouldn't cause fits to fail. 2) Character with missing prereqs - Should be notified prereqs are missing.

Um. 2 seems to be handled more efficiently on the UI side, I'm not sure that a fitting engine cares whether a character has a particular set of skills that don't apply at all to the fit it's calculating.

It'd certainly be nice to pass back a warning, but I don't think that this should be passed back as an exception. Exception to me means something failed, especially since it returns something other than a 0 and bails out early. (I know we can handle the exception so that Python doesn't bail out early, but that's beside the point.)

DarkFenX commented 7 years ago

Skills are part of the 'fitting' just like implants/boosters. Although in this case - I agree - not as important in this conext. Having ability to validate skill tree is nice, from my perspective, in case some functionality might need it - e.g. determine if skill set is completely eligible to use fitting, even if this skill set is used as skill plan for new player.

There're 2 ways to skip it. First way is easy:

fit.validate(skip=(Restriction.skill_requirement,))

But it disables skill requirement altogether. 2nd is more complex, but with it you can achieve what you want:

try:
  fit.validate()
except ValidationError as e:
  data = e.args[0]
  for item in set(data):
    if not isinstance(item, Skill):
      continue
    item_errors = data[item]
    if Restriction.skill_requirement in item_errors:
      del item_errors[Restriction.skill_requirement]
    if len(item_errors) == 0:
      del data[item]
  if len(data) > 0:
    raise ValidationError(data)

This way you remove all skill requirement errors for skills and re-raise validation error if there's anything else remaining. Possibly i will make it built-in later, but do not see need as of today.