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

Ensure remote files have correct _g|p string in the name #3436

Merged
merged 1 commit into from
Jun 8, 2020

Conversation

PaulWessel
Copy link
Member

This is the first thing we do in gmt_init_module as the file may be used for many things, such as finding -R. We are then guaranteed to have the correct name for a remote grid or image.

gmt grdimage -JG0/0/3i @earth_day_01d -B -pdf map

thus becomes internally

gmt grdimage -JG0/0/3i @earth_day_01d_p -B -pdf map

PS. This gets the image and takes me where I want but I think I have found a bug in grdimage so I will sort that out in a separate branch.

This is the first thing we do in gmt_init_module as the file may be used for many things, such as finding -R.
@PaulWessel PaulWessel requested a review from seisman June 7, 2020 05:24
@seisman
Copy link
Member

seisman commented Jun 7, 2020

I got a crash:

--------------------------------------------------------
ERROR: Caught signal number 11 (Segmentation fault) at
0   libgmt.6.dylib                      0x000000010d84122a GMT_grdimage + 17786
1   ???                                 0x0000000000000008 0x0 + 8
Stack backtrace:
0   libgmt.6.dylib                      0x000000010d4af5a2 sig_handler + 498
1   libsystem_platform.dylib            0x00007fff6cd375fd _sigtramp + 29
2   ???                                 0xd739888e0f2600ba 0x0 + 15508576935547895994
3   libgmt.6.dylib                      0x000000010d4ddd71 GMT_Call_Module + 1073
4   gmt                                 0x000000010d4a1eac main + 1228
5   libdyld.dylib                       0x00007fff6cb3ecc9 start + 1
6   ???                                 0x0000000000000008 0x0 + 8

@PaulWessel
Copy link
Member Author

Yep, in grdimage. But the problem is in master too so I am working on a fix there. OK, perhaps wait to approve this branch until I can merge in from master later.

@PaulWessel
Copy link
Member Author

Sorry, this is convoluted. Going to indexed images for the marbles was a good idea, but now we are taking into parts of @joa-quim code that has some issues, so it will be good to fix these. Here are some of the things I am fixing:

1. grdimage projects the image I to Img_proj and deletes the input I, but then proceeds to use I->colormap and crashes.
2. The duplicate_image function did not deal with the colormap at all.  There is also the color_interp variable that is never set either.  We probably should do strdup on this instead of pointing to some mysterios GDAL memory, no?  Trying to free it gave alloc/free errors.

OK, fixing those and now it runs but the image is pure black. Strangely, I then found that the earth_day_01d_p.tif file was indeed black (!) while the others were fine (not sure why but will fix). So I then tried 30m which is a good image, but it also came out black from grdimage, so presumably the colormap is not set right or not used right. Hopefully have time after dinner...

@PaulWessel
Copy link
Member Author

PaulWessel commented Jun 7, 2020

gdalinfo earth_day_30m.tif reports the color table, looks like this:

Color Table (RGB with 256 entries)
   0: 104,88,56,255
   1: 240,216,140,255
   2: 140,140,164,255
   3: 140,144,152,255

When I go through in debug in populate_metadata it find the same colors. Yet, the image that is produced looks like this:

map

What is going on here, @joa-quim ? The command I run is

gmt grdimage -JG0/0/3i earth_day_30m_p.tif -B -png map

You can get the file via

curl -k -O https://oceania.generic-mapping-tools.org/server/earth/earth_day/earth_day_30m_p.tif

PS: The 01d images that were black were produced by gdal_translate. Since they were small 360x180 I turned tiling off for those and then it worked. So a bug in GDAL somewhere.

@PaulWessel
Copy link
Member Author

Note: To run this command without crashing you need to use the branch in #3438.

@PaulWessel
Copy link
Member Author

Some progress. if I do not specify a projection then the image is correct. So the projection of the image is failing.

@PaulWessel
Copy link
Member Author

OK, so I understand the problem now. GMT_Read_Image (via GDAL) finds an index image, and it just returns the index array and the colormap. It does not build an image. THen, when we need to project the image we are hopelessly interpolating a grid of indices...

@joa-quim, does it ever make sense for us to return a non-image, i.e., a list of indices into a colormap? WOuld it not make more sense to expand that into a rgb image instead. At the very least we will need a index2rgb function that converts an index image to a rgb image and remotes the colormap stuff. I am guessing we do not have that? If we did this then all the stuff in grdimage that tries to do this on the fly when building the PS image would be simpler, plus it would actuallly work for this case.

I am glad we decided to make the earth day/night images index so we actually found all this shit. Since you will be up hours before me, let me know what you think we should do. I think a gmt_index2rgb_image (GMT, struct GMT_IMAGE *I) would be helpful and it should be called when grdimage returns an indexed image. OR, we always do this. What good is it in grdmix and other modules to repeat this again and again. I do not think indices are helpful in our modules.

@joa-quim
Copy link
Member

joa-quim commented Jun 7, 2020

I'll be out for the afternoon.
Yes it makes sense to return indexed images. Think on the externals wanting to manipulate them.
And yes, it also makes sense having an index2rgb. The postcript requires always rgb right? (dumb thing)

@PaulWessel
Copy link
Member Author

I will make something similar to gmt_set_outgrid for images since if the input image is read only we cannot reallocate I->data.

@joa-quim
Copy link
Member

joa-quim commented Jun 7, 2020

It does not make sense to convert an rgb image to indexed and project it. The result from the interpolation is garbage. But it does make sense to project a gray scale image. Think on the old satellite images that came in bands of 8 bits images. People reproject them. So at the end what make and does not make sense in on the users hand.

@PaulWessel
Copy link
Member Author

Projecting color or gray is fine. projecting indices are not. That would only work if there is a one-to-one correspondence between the index 0-255 and the corresponding grayshade entry in the colormap (0-255).

bool gmt_prepare_image (struct GMT_CTRL *GMT, struct GMT_IMAGE *I_in, struct GMT_IMAGE **I_out);

written, compiles and is about to be debugged to see if I got it right. Returns true if we had to create a new image because input is read-only AND indexed. Otherwise we just reallocate internally and return I_in and false.

@PaulWessel
Copy link
Member Author

Hi @seisman, despite all the discussion in here, the PR only does one thing we do want, and the rest is handled by the separate PR #3438. Please have a look at this and hopefully approve.

Copy link
Member

@seisman seisman left a comment

Choose a reason for hiding this comment

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

OK. Let's merge it in master and then test PR #3438.

@PaulWessel PaulWessel merged commit 07beeff into master Jun 8, 2020
@PaulWessel PaulWessel deleted the pre-set-names branch June 8, 2020 00:27
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.

None yet

3 participants