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

Multibuy format export feature, adresses #726 #733

Merged
merged 7 commits into from
Sep 27, 2016

Conversation

petosorus
Copy link
Contributor

Hey, it's my first time working on Pyfa but I think I got what needed to be done for this.
I compared with the example in #726 and it matches.

Not sure about where to put the Multibuy option in the list so I just put it at the bottom, written MultiBuy (not sure about typography either).

@blitzmann
Copy link
Collaborator

(☞゚∀゚)☞ You're awesome.

I'll take a look later and test, but looking at the code I don't see any issues.

@Ebag333
Copy link
Contributor

Ebag333 commented Sep 15, 2016

Great start.

I tested it a bit, and while it exports the fit just fine, it doesn't export the following:

  • Implants (Take a look at EFT Implants for some code to steal)
  • Boosters (guessing it might be covered by implants?)
  • Fighters (Don't feel bad about this, the EFT export doesn't handle fighters either....)

It seems to export drones and cargo okay.

The code looks good, not spotting any issues there.

@petosorus
Copy link
Contributor Author

I added Implants, Boosters (they are not covered by implants) and Fighters.

I wonder if charges need a quantity. For now we have

Light Electron Blaster II
Void S

but may need

Light Electron Blaster II
Void S x120

I don't have Eve to test right now, but should have it around 12 Eve time.

@petosorus
Copy link
Contributor Author

On a side note, should EFT export add Fighters and Boosters that are not in cargo?

@Ebag333
Copy link
Contributor

Ebag333 commented Sep 15, 2016

I wonder if charges need a quantity.

They should. The charges out of cargo have the amount, but loaded charges just has a single one. It'd be nice if they reflected the amount loaded.

Listing them multiple times is OK too, EVE will combine them automatically.

On a side note, should EFT export add Fighters and Boosters that are not in cargo?

Well, while you are in there, might as well.

I'm absolutely amazed that no one has reported on this yet.

@petosorus
Copy link
Contributor Author

I wonder if charges need a quantity.

They should. The charges out of cargo have the amount, but loaded charges just has a single one. It'd be nice if they reflected the amount loaded.

I just did this now that I am online, seems to work.

On a side note, should EFT export add Fighters and Boosters that are not in cargo?

Well, while you are in there, might as well.

I'm absolutely amazed that no one has reported on this yet.

I'm amazed too about the Fighters. I wasn't sure of the Boosters export legitimacy because it's under the Booster tab, like projected effects, and not in cargo.
Should this be done on a new branch or on the same?

@Ebag333
Copy link
Contributor

Ebag333 commented Sep 15, 2016

I'm amazed too about the Fighters. I wasn't sure of the Boosters export legitimacy because it's under the Booster tab, like projected effects, and not in cargo.
Should this be done on a new branch or on the same?

I did open an issue for fighters that you can reference.

Umm. Boosters (and implants) are complicated and ugly. The current EFT export with implants gets the implant section rejected. If you put them under cargo, they will import but are invisible.

Boosters are technically just implants, so I'm rather tempted just to say export them under the implant section for EFT. Better solution is probably to export them as cargo for the standard export.

It's all exports, so the same branch should be fine.

@blitzmann
Copy link
Collaborator

I should mention that whatever we do here with the boosters, we need to ensure it actually imports correctly into both EVE and EFT. :)

Looking good so far though, thanks for the help @petosorus!

@Ebag333
Copy link
Contributor

Ebag333 commented Sep 15, 2016

@blitzmann

I should mention that whatever we do here with the boosters, we need to ensure it actually imports correctly into both EVE and EFT. :)

So does it count as correct if it rejects the section, or imports it but it's invisible? 😆

Umm. Boosters (and implants) are complicated and ugly. The current EFT export with implants gets the implant section rejected. If you put them under cargo, they will import but are invisible.

On the subject of boosters vs implants, why do we handle them differently when in game boosters simply are implants that expire?

@blitzmann
Copy link
Collaborator

On the subject of boosters vs implants, why do we handle them differently when in game boosters simply are implants that expire?

Because that's how it's always been. I don't know if there is a real reason. There's been no reason to change it (and indeed, if we ever get around to adding booster side effects, they should stay separate)

@Ebag333
Copy link
Contributor

Ebag333 commented Sep 15, 2016

I don't want to hijack this thread further, but as food for thought if we treated them as implants then we could eliminate a good chunk of code that's essentially just duplicated.

(Potentially also eliminate a tab from the gui, since there's no real reason we couldn't do both in a single tab.)

If I look into this further I'll start a new issue or PR (or you can, if this interests you).

@blitzmann
Copy link
Collaborator

I'd rather not divert efforts into merging implants and boosters. The code is already there, and works. If we ever implement booster side effects, then they will have to be separate, both on the back end and the GUI, to support the effects attached to them.

@petosorus
Copy link
Contributor Author

I added fighters to the EFT exports, according to #736.

Concerning boosters in EFT, I decided to not export them with the base EFT export, as we don't export implants.
I included them in the EFT Implants export, but like the implants, they are not recognized when the fit is imported in game. I left them in the export for consistency.

@blitzmann
Copy link
Collaborator

excellent. I'll be merging this with the next major release (have a planned point release coming up if I ever get off my ass and do it). Thanks a bunch!

@blitzmann blitzmann merged commit e243461 into pyfa-org:master Sep 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants