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

Added CRS indexing based on an sqlite database #108

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

gktval
Copy link

@gktval gktval commented Nov 16, 2022

Implemented a database service to obtain fast indexing of coordinates systems by codes from an sqlite database. Also added a file service to obtain indexing coordinates systems from a csv file.

Fixed parsing errors of wkts.
Added and EXTENSION parse.

There are now three CoordinateSystemServices:

  1. Default
  2. File (this is based on a csv. I initially used this method to test everything) You can obtain this file by exporting the table in the db. Nevertheless, if a user wants to provide a comma delimited file with the same headers as the table, they can.
  3. Database

I am not too sure about how to instantiate the database...
Previously, when CoordinateSystemServices is called without any parameters, it would initialize the Default CRS dictionary of WGS84 and TransMercator. I didn't think this was extremely viable (since it was only two coordinate systems), so I made the default constructor to call the database service. If you want to call the Default service, you should provide any enumeration (or null).

The database was stripped down a bit. For example, about 30 codes overlap with ESRI and EPSG. I removed the ESRI duplicates from the database so that I could make the PrimaryKey the code. I also remove the IGNF authority because these had string values as the code (instead of integer).

The only known problem that persists is parsing horizontal CS with more than two axis. I had commented this exception out while testing. Otherwise, all of the wkts in the database should parse.

Implemented a database service to obtain fast indexing of coordinates systems by codes from an sqlite database. Also added a file service to obtain indexing coordinates systems from a csv file.
Fixed parsing errors of wkts and added and EXTENSION parse.
@CLAassistant
Copy link

CLAassistant commented Nov 16, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ gktval
❌ Travis Yeik


Travis Yeik seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Member

@FObermaier FObermaier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your input @gktval, really appreciated.

It'd b great if you could create a separate PR for the ReadExtension so we can map it to #106


namespace ProjNet.Services
{
public interface ICoordinateSystemService
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A public interface requires proper xml-code documentation

src/ProjNet/Services/DatabaseCoordinateService.cs Outdated Show resolved Hide resolved
src/ProjNet/ProjNET.csproj Outdated Show resolved Hide resolved
src/ProjNet/ProjNET.csproj Outdated Show resolved Hide resolved
src/ProjNet/IO/DatabaseProvider.cs Outdated Show resolved Hide resolved

#endregion

/// <summary>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change, uncomment and mark [Obsolete]

@@ -169,5 +169,18 @@ public void ReadAuthority(out string authority, out long authorityCode)
long.TryParse(ReadDoubleQuotedWord(), NumberStyles.Any, _nfi, out authorityCode);
ReadCloser(bracket);
}

public void ReadExtension(out string extension, out string extensionMethod)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate PR would be great

@gktval
Copy link
Author

gktval commented Nov 17, 2022

I will separate out the EXTENSION. I am still not sure about the the sqlite database being part of the project. So I think I will redo it as well. I think that it is a great addition and rounds the project off, but I don't know how I feel about forcing people to download 6Mb of data if they don't want to use it. I think there are two options: 1) we leave it as a separate download on this github repository and people download it separately, then pass the connection string as a parameter or 2) the sqlite database, the DatabaseService, and the sqlite-net-pcl DLL is put a separate project that people can attach to ProjNet through the ICoordinateSystemService. Thoughts?

Oh nevermind, I see your comments above. We are on the same page.

@gktval
Copy link
Author

gktval commented Nov 17, 2022

I separated out the sql stuff to ProjNet.Sqlite. I also added unit test for each of the services. It looks a lot cleaner now.

There are two EXTENSIONS, one to the Crs and one to the projection. Both of those are added. I really have no idea how to create a separate PR for only the extension stuff; is it possible to close #106 with this PR?

@FObermaier
Copy link
Member

@gktval, I managed to seperate out EXTENSION handling bits or this PR. I see some issues with the code that need to be resolved. Please see #111

Removed the AddParameter method.
Added async search methods to the database.
Travis Yeik and others added 6 commits December 20, 2022 09:52
Update for compatibility with Maui in iOs and Android (ie, embedded resources). Create the Database file if it cannot be found so that things don't crash.
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