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 new grid for Bavaria; Fixed "grid not found error" on Mac OS #46

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

vmarquar
Copy link

@vmarquar vmarquar commented Apr 11, 2020

Hey Guys,
please see my attached pull request to integrate into your plugin.

Short Description:

  1. Fix for Issue: nadgrids not found under Mac OS X
  • 1.1. Problem:
    The default plugin directory under Mac OS X contains a whitespace character > "Application Support", e.g. /Users/<Username>/Library/Application Support/QGIS/QGIS3/profiles/default/python/plugins/ntv2_transformations/grids
  • The best way would be to escape the whitespace character with \ or wrap it into "...". But that's not possible inside the proj-string definition (see Proj#1152, Proj#218).
  • 1.2. Current implemented workaround:
  • Inside transformations.py I added some code to create a hidden symbolic link which is placed inside the user's home directory and references the plugin directory. Not the best solution but I think it's acceptable.
  1. Added new transformation for the grid BY_KANU
  • create two new files for the two transformations VectorDE_BY... and RasterDE_BY...
  • created a download error message which instructs the user to download the big grid file (1.03 GB download size) and move it to the grid directory.

If anything is unclear don't hesitate to contact me.

Kind regards
Valentin
Fixes #45

@gioman
Copy link
Contributor

gioman commented Apr 11, 2020

@vmarquar many thanks for your PR! have you tested the changes on all major OSes (Linux, Windows and macOS)?

@gioman gioman requested a review from alexbruy April 11, 2020 14:58
RasterDE_BY_GK4_UTM32DirInv.py Outdated Show resolved Hide resolved
RasterDE_BY_GK4_UTM32DirInv.py Outdated Show resolved Hide resolved
RasterDE_BY_GK4_UTM32DirInv.py Outdated Show resolved Hide resolved
VectorDE_BY_GK4_UTM32DirInv.py Outdated Show resolved Hide resolved
@vmarquar
Copy link
Author

I added your suggestions. So far I have only tested the plugin under MacOS, but I can double check the plugin under Windows in the upcoming days. I will let you know if there are any Windows specific issues.

@gioman
Copy link
Contributor

gioman commented Apr 12, 2020

I added your suggestions.

Thanks!

So far I have only tested the plugin under MacOS, but I can double check the plugin under Windows in the upcoming days. I will let you know if there are any Windows specific issues.

We would appreciate it. We can test on Linux. Cheers!

@gioman
Copy link
Contributor

gioman commented Apr 12, 2020

@alexbruy thanks for the review!

@vmarquar
Copy link
Author

vmarquar commented Apr 15, 2020

So I checked the Plugin under Windows 10 (QGIS 3.6.2) and my adaptions seem to work fine. There is only one problem where QGIS is not guessing the EPSG Code correctly (it guesses EPSG:3397 PD/83 instead of EPSG:5678/EPSG:31468). Should I try the -a tag in the gdal command to assign a Code explicitly?

However, I found another bigger bug which is affecting all urlretrieve statements. When running behind a Proxy Server (or you don't have an Internet connection at all) the Plugin chrashes with a timeoutError. We should at least rise an exception with a correct error message, which informs the user to download the files manually. Another Option would be to implement a download solution which is getting the proxy configuration from the QGIS Settings (like e.g. using the QgsNetworkAccessManager class) or just bundle the small .gsb files directly with the plugin.

@gioman
Copy link
Contributor

gioman commented Apr 15, 2020

So I checked the Plugin under Windows 10 (QGIS 3.6.2) and my adaptions seem to work fine.

@vmarquar Nice, many thanks for your effort. That QGIS version is anyway too old especially because it uses ancient GDAL and Proj versions compared to the latest QGIS ltr. Any test at the moment must be done against QGIS installations using GDAL 3 and Proj 6/7.

There is only one problem where QGIS is not guessing the EPSG Code correctly (it guesses EPSG:3397 PD/83 instead of EPSG:5678/EPSG:31468). Should I try the -a tag in the gdal command to assign a Code explicitly?

What GDAL and Proj versions are used by the installations you have tested with? In the QGIS bug tracker a few similar issues have been reported and in most cases they were fixed upstream in GDAL or Proj. It is possible that the fixes have not yet landed in QGIS packages. You should test this hypotheses by doing a transformation from the command line using ogr2ogr and then loading the results in QGIS. Of course we can also test if -a_srs helps anyway.

However, I found another bigger bug which is affecting all urlretrieve statements. When running behind a Proxy Server (or you don't have an Internet connection at all) the Plugin chrashes with a timeoutError. We should at least rise an exception with a correct error message, which informs the user to download the files manually. Another Option would be to implement a download solution which is getting the proxy configuration from the QGIS Settings (like e.g. using the QgsNetworkAccessManager class)

I see, but this scenarios were never really in the scope of the plugin. Of course supporting this cases would be a nice improvement, but unfortunately right now we can't put any effort in it.

just bundle the small .gsb files directly with the plugin.

There is a "no binaries in plugins code" policy.

@vmarquar
Copy link
Author

@gioman I've conducted the test with following QGIS installation parameters:

  • QGIS-Version | 3.6.2-Noosa | QGIS-Codeversion | 656500e0c4
  • GDAL/OGR | 2.4.1
  • PROJ | 5.2.0 | Rel. 5.2.0, September 15th, 2018

You are right. My installation is indeed very old and I will try to update QGIS and will let you know if the problem still exists after the upgrade.
Btw, a short test using gdalwarp on the command line didn't work as the -a_srs parameter doesn't get accepted by gdalwarp.

@gioman
Copy link
Contributor

gioman commented Apr 21, 2020

  • GDAL/OGR | 2.4.1

  • PROJ | 5.2.0 | Rel. 5.2.0, September 15th, 2018

@vmarquar hi, yes as said there is not point in testing any QGIS installation that does not use GDAL 3 and Proj 6/7. CRSs handling has changed a lot in this GDAL/Proj versions so we must test on them to see if any change is needed the way plugin uses ogr2ogr or gdalwarp.

Btw, a short test using gdalwarp on the command line didn't work as the -a_srs parameter doesn't get accepted by gdalwarp.

no, but gdal_translate has it. So if needed we may think use translate instead of warp (or we can pipe the result of warp in a translate command).

Copy link
Author

@vmarquar vmarquar left a comment

Choose a reason for hiding this comment

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

added small improvements.

@gioman
Copy link
Contributor

gioman commented May 31, 2022

added small improvements.

@vmarquar thanks for the follow up. I will review it ASAP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Grid for Germany, Bavaria
3 participants