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

Planetary Approach Suite and Docking Computer export problem (easy fix) #632

Closed
mattbledsoe opened this issue Jan 13, 2021 · 11 comments
Closed
Labels
bug Where the data or feature exists, but doesn't work properly cat:import Import related problems, where an import fails, or flags the issue

Comments

@mattbledsoe
Copy link

mattbledsoe commented Jan 13, 2021

[Update added at bottom only 15 min later]

When clicking on the "Stations That Sell This Build" button, the "1I Planetary Approach Suite" is included in the "Stations sells Modules" on eddb. With that included, nothing can match as they are no longer supported as "for sale" by eddb. I spent almost 15 minutes trying to figure out how Jameson didn't sell my build. After I realized the issue was so simple and removed that single Module, I immediately got multiple hits. With so many new players and new coriolis and eddb users (and some of us having played for a while LOL), it may appear the export is broken while it actually isn't. Hopefully simply removing the singular object from your api will be super-simple.

Thanks!

I also read the only previous ticket for "Planetary..." and see it was left due to legacy issues from the previous ED versions. Now though, there is no need for this to be included as neither eddb nor the recent ED base package integration with Horizons. (Sure, there's someone out there still running the original, but if they are, I'm sure they won't have any trouble finding this part!)

[15 min update]
Maybe there was a brief eddb issue? After entering this ticket, Jameson DOES now show up as having the Planetary Approach Suite (PLAS). However, when removing only that, one more station shows as having the build. I then did a search for only the PLAS and see there are a total, including planetary and not carriers, of 3075 stations that carry it. When entering very common things such as a 1E Supercruise assist or 1E Advanced Docking, 23,000 to 24,000 locations appear. This tells me the eddb database isn't consistent with regards to the PLAS, so my initial suggestion to remove it still appears to be a good idea.

Additionally, while testing this, I noticed the "1E Advanced Docking Computer" is exported incorrectly as the "1E Standard Docking Computer".

@mattbledsoe mattbledsoe changed the title Planetary Approach Suite Export problem (easy fix) Planetary Approach Suite export problem (easy fix) Jan 13, 2021
@mattbledsoe mattbledsoe changed the title Planetary Approach Suite export problem (easy fix) Planetary Approach Suite and Docking Computer export problem (easy fix) Jan 13, 2021
@felixlinker
Copy link
Member

I can't reproduce this behaviour. For me, the planetary approach suite is not included. Could you give me more details to reproduce this?

@felixlinker felixlinker added the reviewing This issue or pr is currently under review label Jan 27, 2021
@mattbledsoe
Copy link
Author

mattbledsoe commented Jan 29, 2021 via email

@felixlinker
Copy link
Member

Thanks for giving it another go.

I prefer issues. It's better organized here and more public. But I'm happy to take discussions to another platform if it suits you. The one who shares the issue can choose :)

I will close this issue, for now, feel free to reopen it, once you find a way to reproduce the behaviour!

@bakanisan
Copy link

The problem with the Planetary Approach Suite is if you import from EDMC and then export the build to EDDB it will be included if it's on your ship. Create a new build and export it to EDDB afterwards should no longer have the PAS included.

Also I'm not sure if this qualifies as a new issue but since it is mentioned here I'll post here instead.
When exporting the build to EDDB, no matter which Docking Computer is chosen it is always exported as Standard Docking Computer.

@felixlinker felixlinker reopened this Feb 3, 2021
@felixlinker felixlinker added bug Where the data or feature exists, but doesn't work properly cat:import Import related problems, where an import fails, or flags the issue reviewing This issue or pr is currently under review and removed reviewing This issue or pr is currently under review bug Where the data or feature exists, but doesn't work properly cat:import Import related problems, where an import fails, or flags the issue labels Feb 3, 2021
@felixlinker
Copy link
Member

felixlinker commented Feb 3, 2021

Still can't reproduce. I imported one of my builds using EDMC and didn't have the PAS on EDDB afterwards.

Could you send me an import link I can reproduce this problem with? It is critical that the link is an import link, i.e. has coriolis.io/import?data= in the beginning.

@bakanisan
Copy link

bakanisan commented Feb 3, 2021

Here you go.
I've also tested creating a new identical build in incognito mode and the result is the same: no PAS included just like creating a new one in "normal mode". Copying the import link to incognito mode brings back the PAS so I think the problem is with EDMC itself.
This is not the EDMC github so maybe I should bring this issue over to them?
The Docking Computer issue is an EDDB one though.

@felixlinker
Copy link
Member

Thanks for taking the whole picture into consideration. I think, though, that the issue is on our side. To my knowledge, EDMC simply compresses the loadout from Elite and hands it to coriolis. Coriolis should handle the approach suite correctly. I will investigate this myself first!

@felixlinker felixlinker added bug Where the data or feature exists, but doesn't work properly cat:import Import related problems, where an import fails, or flags the issue and removed reviewing This issue or pr is currently under review labels Feb 3, 2021
@bakanisan
Copy link

Thank you! After looking up PAS on EEDB with Planetary Bases and no Carrier I've found just shy of 3000 Stations having this module. This is simply incorrect as I myself was in one with PAS and don't see it in the list.

I suggest maybe remove the module entirely from Coriolis when exporting to other websites or when importing from EDMC (or EDDI but I don't use it so I can't be certain).

@mattbledsoe
Copy link
Author

mattbledsoe commented Feb 5, 2021 via email

@mattbledsoe
Copy link
Author

mattbledsoe commented Feb 5, 2021 via email

@alex-williams
Copy link
Collaborator

This was resolved in this commit EDCD/coriolis-data@3654123 It has been merged to Dev and is live at https://beta.coriolis.io. The data was wrong, with the Supercruise Assist Module having the EDDB Id of the PLAS against it inside coriolis-data and the Advanced Docking Computer having the same EDDB Id as the standard one. This caused any export with an SCA in it, to look for the PLAS in the export link and it caused any export with an Advanced Docking Computer, to simply look for a standard one. Closing this issue, as it will be deployed to the live site soon. However, I have created #756 which seeks to improve the export link generation, so that it does not include default core modules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Where the data or feature exists, but doesn't work properly cat:import Import related problems, where an import fails, or flags the issue
Projects
None yet
Development

No branches or pull requests

4 participants