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.

Sign up for GitHub

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

Jump to bottom

GMT 6.x.x and MB-System issues #1055

Closed
PaulWessel opened this issue Apr 11, 2020 · 22 comments
Closed

GMT 6.x.x and MB-System issues #1055

PaulWessel opened this issue Apr 11, 2020 · 22 comments

Comments

@PaulWessel
Copy link

Hi MB-System developers. The GMT developers have decided to be more proactive in making sure MB-system works well with GMT 6.x and future releases. I have therefore forked your repo and trying to build and fix anything so I can submit a pull request. This is on macOS Catalina with macports. Since I rarely build MB it is likely I am running into trivial issues but they are preventing me from doing my testing. Here is my status:

  1. Your list of prerequisites seems incomplete: proj is not mentioned, and GDAL is missing from your README.md (but mentioned on the MBARI list).
  2. I have a configure script that has worked in the past, and works (up to a point) with 5.7.5. The problem is that despite configuring with --with-proj-lib=/opt/local/lib/proj6/lib --with-proj-include=/opt/local/lib/proj6/include, the master build fails with
g++ -std=gnu++11 -DHAVE_CONFIG_H -I. -I../../src/mbio  -I../../src/mbio -I../../src/mbaux -I../../src/gsf  -I/Users/pwessel/GMTdev/gmt-dev/dbuild/gmt6/include/gmt -I/opt/local/include -I/opt/local/include   -g -O2 -MT mbsvpselect.o -MD -MP -MF $depbase.Tpo -c -o mbsvpselect.o mbsvpselect.cc &&\
	mv -f $depbase.Tpo $depbase.Po
mbsvpselect.cc:193:10: fatal error: 'geodesic.h' file not found
#include <geodesic.h>

That file exists, so I am not sure why my includes are not honored for this makefile. I see there is now lots of C++ files that have been added so perhaps the build for these are not robust yet. So I try the 5.7.5 tarball instead. This works until we hit src/gmt. This step fail because the precompiler directives to check for GMT 5 versions fail on 6.0.0 and think it is < 5.2.1, and chaos ensues. Specifically,

#if GMT_MINOR_VERSION == 1 && GMT_RELEASE_VERSION < 2
			if (gmt_check_filearg(GMT, '<', opt->arg, GMT_IN)) {
#else
			if (gmt_check_filearg(GMT, '<', opt->arg, GMT_IN, GMT_IS_DATASET)) {
#endif

needs to be

#if GMT_MAJOR_VERSION == 5 && GMT_MINOR_VERSION == 1 && GMT_RELEASE_VERSION < 2
			if (gmt_check_filearg(GMT, '<', opt->arg, GMT_IN)) {
#else
			if (gmt_check_filearg(GMT, '<', opt->arg, GMT_IN, GMT_IS_DATASET)) {
#endif

Same for mbcontour.c and mbgrdtiff.c. Similarly, the two latter tools also need

#if GMT_MAJOR_VERSION == 6 || (GMT_MAJOR_VERSION == 5 && GMT_MINOR_VERSION > 3)
	n_errors += gmt_M_check_condition(GMT, !GMT->common.R.active[RSET], "Syntax error: Must specify -R option\n");
#else
	n_errors += gmt_M_check_condition(GMT, !GMT->common.R.active, "Syntax error: Must specify -R option\n");
#endif

to compile. With these edits, mb-system builds with GMT 6.x. I note your instructions state you require GMT 5.4 or later. If so, you could have configure check for that (via gmt --version) and stop if that is not true - you could then remove all those #ifdefs that deal with things older than 5.4. In fact, you have lots of defines at the top of each of these three C codes that deal with 5.2 and older. You could yank all that if you stick to your 5.4 cutoff. Even Ubuntu seems to have managed to move up to 5.4.5.

However, if you can address the build issues with g++ above (or tell me how to circumvent it) then I am happy to submit a pull request.
@PaulWessel PaulWessel mentioned this issue Apr 11, 2020
Closed
@dwcaress
Copy link
Owner

dwcaress commented Apr 12, 2020 via email

@joa-quim
Copy link
Contributor

Dave, just let me add that I'm sad that you decided to go into the C++ way. C++ is simply no good neighbor. Now the possibility of having the MB programs adapted to be GMT modules. like mbcontour for example, and hence be easily accessible from other languages like Matlab or Julia is definitively gone.

@dwcaress
Copy link
Owner

dwcaress commented Apr 12, 2020 via email

@joa-quim
Copy link
Contributor

Dave,

All GMT modules end up as shared libraries. Shared libraries in C++ suffer from the name mangling issue and are not callable by other languages (not even C). That is why C++ is no good neighbor.

Regarding the GeoTiffs, sorry I don't get. grdimage does that since the option to write GeoTiffs was added many years ago.

@PaulWessel
Copy link
Author

I think Dave is particular about what is written to the geotiff. THe easy solution would be to rip out most of mbgrdtiff and simply do a module call to GMT_grdimage and return a GMT_IMAGE, which mbgrdtiff can then write out as it sees fit.

@dwcaress
Copy link
Owner

dwcaress commented Apr 12, 2020 via email

@joa-quim
Copy link
Contributor

Ok, this is a simple example with geotiff grids. Tried to project it to make it more clear but some shit in propagating the proj coords metadata is happening

grdimage does not create geotiffs with other than the image. I.e., not images with vector lines. But psconvert does it.

grdmath -R0/20/30/50 -I0.1 X Y MUL = lixo.grd
grdedit lixo.grd -J+proj=longlat
grdconvert lixo.grd lixo.tif=gd:GTiff
gdalinfo lixo.tif

Driver: GTiff/GeoTIFF
Files: lixo.tif
Size is 201, 201
Coordinate System is:
GEOGCRS["WGS 84",
    DATUM["World Geodetic System 1984",
        ELLIPSOID["WGS 84",6378137,298.257223563,
            LENGTHUNIT["metre",1]]],
...

@dwcaress
Copy link
Owner

dwcaress commented Apr 13, 2020 via email

@joa-quim
Copy link
Contributor

joa-quim commented Apr 13, 2020 •
edited
Loading

Dave, you'll excuse me but I don't remember that we have done that failed test. I will be making a PR in GMT to fix some bugs related to the non-propagation in some cases of the CRS info when writing either nc or GTiff files. But that is a separate issue. With those fixes one can do
(But maybe it will work for you because the bugs occurred when one uses the -J+proj=utm+zone=32 and the necessary space before +zone was not introduced in some part of the code)

gmt grdmath -R0/20/30/50 -I0.1 X Y MUL = lixo.grd
gmt grdproject lixo.grd -J"+proj=utm +zone=32" -Glixo_utm.grd
gmt grdconvert lixo_utm.grd lixo_utm.tiff=gd:GTiff

gmt grdimage lixo_utm.grd  -JX8c -I+d -Ba -BWSen -P -K > gimg_tif.ps
echo 10 40 | mapproject -J"+proj=utm +zone=32" | gmt psxy -JX8c -Rlixo_utm.grd -Sc1c -Gwhite -O -K >> gimg_tif.ps
gmt grdimage lixo_utm.tiff -JX8c -I+d -Ba -BwSen -X8.5c -O -K >> gimg_tif.ps
echo 10 40 | mapproject -J"+proj=utm +zone=32" | gmt psxy -JX8 -Rlixo_utm.tiff -Sc1c -Gwhite -O -K >> gimg_tif.ps

gmt grdimage lixo_utm.grd -I+d -Agrdimg.tiff        # This is a shaded GTiff IMAGE, not a GRID

gmt grdimage grdimg.tiff -JX8c -X-8.5c -Y8.5c -Ba -BWseN -O -K >> gimg_tif.ps
echo 10 40 | gmt mapproject -J"+proj=utm +zone=32" | gmt psxy -JX8c -Rgrdimg.tiff -Sc1c -Gwhite -O >> gimg_tif.ps

and get
gimg_tif

@joa-quim
Copy link
Contributor

PR submitted in GMT. But with this we hijacked the original issue. Sorry.

@PaulWessel
Copy link
Author

PaulWessel commented Apr 13, 2020 •
edited
Loading

BTW, I am this close to having redone mbgrdtiff via a call to GMT_grdimage.

@PaulWessel
Copy link
Author

I have hacked up a new version of mbgrdtiff.c that uses a call to GMT_grdimage to do the work. Since I am unable to use your github version directly [there must be something that is brew-specific about the configure process], I did this with 5.7.5. Since the new version adds a few new things and deletes most of the old grdimage-lookalike code I think you can easily do this with what is attached and insert into your master version. Here are the steps:

  1. The main differences is a new static function called mbgrdtiff_get_image that takes your options and builds a command string and then calls grdimage and receives the image. Just insert that code into your mbgrdtiff.c above the main function.
  2. Instead of the old image calculations, just keep the new call to that new function and the next three lines, i.e., everything between

/*---------------------------- This is the mbgrdtiff main code ----------------------------*/
and
/*------------------------- Write out the GeoTiff and world files -------------------------*/

  1. Compile with -Wall and remove all the unused variables. I tested this and both the new and old 5.7.5 mbgrdtiff gives me the same tiff file.

One caveat is that this requires GMT 6.x since I had to fix some issues in grdimage to make this work.

mbgrdtiff_new.c.txt

@PaulWessel
Copy link
Author

Note the old mbgrdtiff lists options in the usage message that are not available in that module (-B, -K, -P, -X, etc). I deleted all of those.
Also, looks like 5.7.5 version crashes since it is accessing Grid_orig[0] in a fprintf (stderr message long after Grid_orig has been destroyed.

@dwcaress
Copy link
Owner

Ok, thanks. I will try it out. Jumping back to the beginning of the thread, I've duplicated the build error you found when building with prerequisites installed through MacPorts.

@dwcaress
Copy link
Owner

And I fixed the build error that died at mbsvpselect. I needed to add libproj_CPPFLAGS to AM_CPPFLAGS in src/utilities/Makefile.am, and then resconstruct the build system. This fix has been merged into master.

@PaulWessel
Copy link
Author

Great, I will give it a try. if that works, would you want me to submit a PR for the tiff stuff instead?

@dwcaress
Copy link
Owner

Does this work with the GMT 6.0.0 release?

@PaulWessel
Copy link
Author

No, since it doing this I found a bug. This was the first time we called GMT_grdimage and wanted a memory reference to an image back, so had not been tested before. Minor bug, but not in 6.0.0.

@dwcaress
Copy link
Owner

So, no, don't put it into a PR. You've given me the code - I need to wrap it in conditionals so we can still build and work with GMT 6.0.0, and for the time being, GMT 5.4.5. I'll try to do it moderately cleanly. I take it the required mods to GMT are merged into your master branch, correct?

Thanks. This is definitely an improvement.

@PaulWessel
Copy link
Author

OK, that is fine. Yes, the fix has ben committed to our master branch. Let me know if anything fails.

@dwcaress
Copy link
Owner

dwcaress commented Apr 14, 2020 via email

@schwehr
Copy link
Collaborator

schwehr commented Apr 23, 2020

Dave, just let me add that I'm sad that you decided to go into the C++ way. C++ is simply no good neighbor. Now the possibility of having the MB programs adapted to be GMT modules. like mbcontour for example, and hence be easily accessible from other languages like Matlab or Julia is definitively gone.

I strongly disagree with your blanket statement about C++.

C++ doesn't impact the ABI if used from a C API. e.g. gdal and lots of other open source projects. IMHO, C++ ( > 11 ) makes for much more robust internals to libraries. I did nothing with C++ in MB-System that impacts the ABI. If it was up to me, all of MB-System would be C++ >= 17 with a C APIs for basic use by others. The reliability and standardization of basic functionality is such a huge win.

@dwcaress dwcaress closed this as completed Mar 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@schwehr @joa-quim @dwcaress @PaulWessel