pyfa-org / Pyfa

Python fitting assistant, cross-platform fitting tool for EVE Online
GNU General Public License v3.0
1.61k stars 407 forks source link

Projected modules on A, which is projected on B, affect B #84

Closed blitzmann closed 10 years ago

blitzmann commented 10 years ago

I hate writing descriptive titles to these things

https://forums.eveonline.com/default.aspx?g=posts&m=4511137#post4511137

Modules that are projected on a ship (A) also affect ships that A is projected onto.

For example, I project Remote Shield Booster to fit A, then project A -> B. B will have Remote Shield Booster effects applied.

Without looking into, seems that this may be somewhat the same as #72 in theory and fix.

This issue also occurs with #72's fix (#78, which is awaiting @DarkFenX's input), so that does not directly solve the problem.

blitzmann commented 10 years ago

tl;dr: bad conditional, fixed locally, will test and submit pull request.

I have fixed this on my own branch: https://github.com/blitzmann/Pyfa/commit/d32407b9f846be0c3199c733fb972c7d2a3dd086 As the fit calculation algorithm doesn't have much documentation, I was able to rig up a series of debug statements that helped to identify the problem. This prints out various variables and indents to follow recursion, making it easier to find out what is affecting what. I commit this to a separate branch as it may come in handy again: https://github.com/blitzmann/Pyfa/commit/8974ab42e401471a9efe4a5d0cb5294a21175b02

Now, to document the problem and the fix: When fits are calculated, they take the values of various sources (modules, drones, projections, etc). These are chained together, and depending on if a fit is projected or not, this chain may differ. This is noted in this (original) block of code:

if targetFit == self and forceProjected is True:
    c = chain((self.character, self.ship), self.drones, self.boosters, self.appliedImplants, self.modules)     
else:
    c = chain((self.character, self.ship), self.drones, self.boosters, self.appliedImplants, self.modules,
              self.projectedDrones, self.projectedModules)

As you can see, there are two paths that the calculations can take: what I like to call the projected path or the standalone path.

The standalone path is for a regular fit and includes it's projected drones and modules as well in the calculations.

The projected path should happen if the fit being calculated has a targetFit - in other words, if its stats should be applied to another fit. This tells us that it is a projected fit, and not to include projected modules or drones.

It seems that the original code was going in the right direction, but a problematic conditional (targetFit == self) was preventing it from going down this path. I think this might be a typo, originally meant to be a != comparison. The thing is that if forceProjected is True, then targetFit should not be self (instead, it should point to the, well, target fit), so instead of fixing the typo I simply remove the whole conditional.

I still would like to run a few tests (in case I'm wrong in that assumption), and as always for calculation issues I'll submit a pulll request for @DarkFenX's review before committing to master. :smile:

StinGer-ShoGuN commented 10 years ago

OK so I'll do the same comment here as in the chained fit problem: I reported a bug on the previous Pyfa website regarding self projected fits that causes Pyfa to crash. (If you project fit A on itself, boom, everything goes down.) The solution applied was to forbid self projection so I think the targetFit == self is here on purpose. It's to prevent appliance of projection onto self (I guess). I would recommend to test self projection with your commit and check whether it crashes or not.

blitzmann commented 10 years ago

Confirmed that self projection still isn't allowed (I believe through the GUI rather than EOS). If you muck around with the database self projection may still cause issues (this might actually be fun to try later :smiley:) Regardless, if you follow the logic in the code, targetFit should never be self while forceProjected is also true, making the conditional meaningless. Unless I'm missing something, which is totally possible.

This may be a case where the Pyfa development team back then wanted to do self projection and tried to include support in the code for it, and various parts of the code still have artifacts of this. I'm not sure. It would be nice if we had access to the old issue tracker, but I believe Kadesh says that it's gone for good.

I will try to make a build and release on my own repository of the changes to calculations so that people can test these things. =)

EDIT: here's the test release on my repo: https://github.com/blitzmann/Pyfa/releases/tag/v1.1.23-projectionTest

EDIT2: Come to think of it, I could have made this test release on this repo. Oh well. Please try to break things.

StinGer-ShoGuN commented 10 years ago

Well, there is not much to say about the previous issue tracker. They just forbid self projection. Point. And I think it's a good idea anyway. Otherwise, you're going into programming madness. But I'd say Pyfa somewhere in its "middle ware" (i.e. between EOS and the GUI) should check/delete any self projection.

Maybe, another option to allow self projection would be to create "hidden fits". For instance, let's say I want to project A onto A. Then Pyfa silently creates fit .A (notice the . in front :smiley: ) and then projects .A on A but displays A as being projected. Then, it keeps tracks somewhere to update .A when A is updated and to delete .A when projection is removed or A is removed.

But, I'm getting away from the original issue depicted at first here. :P

DarkFenX commented 10 years ago

For the sake of documentation:

[11:16:00 PM] DarkPhoenix: i think i had to do this on engine level [11:16:08 PM] DarkPhoenix: because people already had self-projected fits [11:16:15 PM] DarkPhoenix: and when they opened it, it choked [11:16:26 PM] DarkPhoenix: so blocking only on service/ui level was not enough