pyfa-org / Pyfa

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

Attempting to use rigs of incorrect size is being handled improperly #2400

Closed fonsuiplaysvideogames closed 2 years ago

fonsuiplaysvideogames commented 2 years ago

The image below shows the "charge stats" window for the currently loaded rockets, and shows a bonus from a medium missile damage rig that I dragged on to the fitting. The program is treating it as if the rig is fitted, but the rig does not appear in the fitting screen. This is only reversible by using the undo function.

image

blitzmann commented 2 years ago

A few things:

fonsuiplaysvideogames commented 2 years ago

Here's a fresh DB with only a Heron in it, with one launcher and one turret. I don't know if the DB will contain any errors, it might be entirely ephemeral and only exist in memory.

On Mon, Jan 31, 2022, 15:44 Ryan Holmes @.***> wrote:

A few things:

  • Can you post the EFT format of the fitting?
  • Do you have overrides enabled? (if you don't know, then probably not)
  • Which version of pyfa?
  • Can you send me your saveddata.db file (location can be found in preferences > database) to @.*** and include in the email the fit that is exhibiting this issue? it could be that, somehow, your fit has a phantom rigs applied to it in the database and is involved with calculations, but isn't showing in the UI. Regardless, if it's reproducible with your database file, I can at least try to pin point where that affected by entry comes from.

— Reply to this email directly, view it on GitHub https://github.com/pyfa-org/Pyfa/issues/2400#issuecomment-1026192486, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOMWKMJMFUVZVP3T45MNVHLUY3YEJANCNFSM5NFIBO3A . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

fonsuiplaysvideogames commented 2 years ago

Further description of observed behavior:

fonsuiplaysvideogames commented 2 years ago

It is also worth noting that only dragging the rig into the fitting results in this issue, double-clicking the rig or using the alt+# hotkey do not result in the rig being applied incorrectly.

ghost commented 2 years ago

This does not appear to be limited to rigs, other modules with fitting restrictions like capital sized plates and shield extenders have the same issue. The phantom module can be removed by forcing an update in the GUI by adding a valid module to a different slot. In this case the GUI is not being refreshed when attempting a replace with an invalid module.

https://github.com/pyfa-org/Pyfa/blob/b62260cd1f308c550ab3ad878964b094859ee9c6/gui/fitCommands/calc/module/localReplace.py#L47-L52

self.savedStateCheckChanges = sFit.checkStates(fit, newMod) is being called before checking if the module is valid and undo-ing the replace operation. This sets self.savedStateCheckChanges to a tuple of empty dicts, which prevents needsGuiRecalc returning true when it's called after the undo replace operation.

https://github.com/pyfa-org/Pyfa/blob/b62260cd1f308c550ab3ad878964b094859ee9c6/gui/fitCommands/calc/module/localReplace.py#L93-L97

I think the easiest solution to this is just to move self.savedStateCheckChanges = sFit.checkStates(fit, newMod) out of the conditional where it is now and move it to the end of the method after all the restrictions checks.

blitzmann commented 2 years ago

Thanks for looking into it @burnsypet! Haven't had a chance to dig, but this seems like the reasoning behind it. I'll look into the proposed solution tonight

blitzmann commented 2 years ago

Pushed up a fix for this

DarkFenX commented 2 years ago

The fix is wrong. The code correctly detected that module does not fit. In case of putting it over previously empty module, it correctly used CalcRemoveLocalModulesCommand. What was missing is recalculation after its execution. I pushed proper fix in 3024ccd176c2a1a1e0f617c9857f03669ea01bf5

DarkFenX commented 2 years ago

FYI checkStates is something keeping track of side-effect changes. E.g. when projected scram is added, MWD is disabled. It should not carry any extra information, and if there were no side-effect changes, it should be kept as None (not like it matters much in this particular case, but if a few principles are broken, code is much harder to debug and trace).