Check that modules fit on imported fits #512 #522

[andrew-nowak]

On import, modules will respect module quantity limits and hardpoints as well as activation limits.
Also minor bugfix to allow importing of XML loadouts via clipboard.

I only tested these with importing via clipboard, but they should work similarly for other styles. My test loadouts are here: https://gist.github.com/aacn500/e5ec75d4a2048506f4ae

[blitzmann]

Hi,

thanks for the pull request, will look into it tonight probably. But can you elaborate on the XML import bug that you mentioned? I wasn't aware there was an issue with it...

[blitzmann]

Also, not sure if we need this bit for everything:

               # Check that there are no conflicts between active modules
                sFit = service.Fit.getInstance()
                sFit.recalc(f)
                sFit.checkStates(f, None)

If we are making sure the module fits while we add it, then this should always return that the fit was fine, shouldn't it? The problem with this is that if someone imports a lot of fits at one (ie: from a backup), then this is going to recalculate all those fits, causing significant slowdown (untested, I can assume though =P). If there is a reason for it, sure, but I am unsure if this check would ever really do anything (this might be needed for t3 cruisers to check final module count? Unsure, really need tot est this later)

[andrew-nowak]

The XML input bug: there was an uncaught exception at

Pyfa/service/port.py

Line 497 in 4507495

doc = xml.dom.minidom.parseString(text.encode(encoding))

because importXml was being passed encoding=None when trying to import from clipboard. I doubt many people try to import xml fits from clipboard, so I suspect that is why it hasn't come up before.

The code block: I used that block of code to stop multiple prop mods being set active in an import, isValidState doesn't check that by itself.

[blitzmann]

Nice catch with the bug.

I'm still hesitant with calculating the fit with every import however. I just think there is a smarter way to do it, we just gotta figure it out. Although, with my test database of 585 fits, it took 58 seconds to save vs 20 seconds without calculating them. It's not too bad (especially since it's only during large imports), but it's not great.

We have Module.canHaveState() which might be able to help, but two caveats:

• In it's current form, it lets two of the prop mods to pass as active with one disabled, instead of the intended 1 active and 2 disabled. I don't know why this is, but the logic determining this may be funky.
• There's still some edge cases of effects modifying the maxActive attributes (ship hull bonuses to gang links come to mind). I don't believe there's a lot that affect it, but it's still a thing that happens. The only way to know would be to calculate the fit, which is what I'm trying to avoid...

[blitzmann]

Actually, we might be able to look into using checkStates when the fit is loaded to make sure it comes back correctly. In other words, save it to the database with the dirty user-provided states, and check when loading them from the database. Maybe through the actual append code such as I did here (actually, wouldn't work as fit is not calculated at this stage), or when we do a recalc(), or when we load the fit with getFit() (this is probably the preferred method), etc. Something to look into. If you're not interested in looking into it, let me know and I will merge this pull request and leave myself a note to look into it later. =)

I actually need to go through and sort out the recalcs in a bunch of places. There are many times when we load a fit and it calculated it 3 times for some god awful reason. shrug

[andrew-nowak]

Moved the checkStates() calls into getFit() per your suggestion, seems to work nicely. I put the calls inside the if inited is None or inited is False: block, because as far as I can tell that is called only the first time a fit is loaded from the database, and after that we can assume that module states have been correctly set. It might be safer running every time a fit is loaded, but I couldn't think of any cases where it would change any states.

[blitzmann]

The whole recalculation events are funky and there's no good way to do it right with what we have. The engine is a bit clunky right now. It would be good if we can do some introspection into the effect files to find out of a module affects a certain attribute, in which case we could then run that effect alone rather than the entire effect. Would be good for the maxGroupActive attribute such as this, but alas, it's not something that's currently supported.

Regardless, I've merged your changes and tweaked a bit - turns out we don't actually have to call recalc where you did as it's already called in the if not projected bit. Please continue to test and break for bugs - Thanks for the work!