Final grdimage upgrade related to transparency #8241

PaulWessel · 2023-12-28T11:43:19Z

This branch replaces the deprecated branch #8191 which added some complications. This branch still passes 100% of the tests. I will now work on adding tests that highlight new capabilities (like variable transparency via square polygons) while making sure all tests still pass.

Moving parts over from old branch

github-actions · 2023-12-28T11:57:58Z

Summary of changed images

This is an auto-generated report of images that have changed on the DVC remote

Status	Path
added	test/baseline/grdimage/grdimage_Q_effects.ps
added	test/baseline/grdimage/grid_transp.ps
added	test/baseline/grdimage/image_vartrans.ps
added	test/baseline/grdimage/transp_mix.ps
modified	test/baseline/grdimage/

Image diff(s)

Added images

test/baseline/grdimage/grdimage_Q_effects.png

test/baseline/grdimage/grid_transp.png

test/baseline/grdimage/image_vartrans.png

test/baseline/grdimage/transp_mix.png

Modified images

Path	Old	New

Report last updated at commit 41df65d

seisman · 2023-12-28T12:45:19Z

Building fails on Linux:

/home/runner/work/gmt/gmt/src/grdimage.c: In function ‘grdimage_img_variable_transparency’:
/home/runner/work/gmt/gmt/src/grdimage.c:1313:46: error: ‘x’ undeclared (first use in this function)
 1313 | #pragma omp parallel for private(srow,kk_s,k,x,scol,node_s,node_i,fill,y,dim) shared(GMT,Conf,Ctrl,H_s,H_i,image)
      |                                              ^
/home/runner/work/gmt/gmt/src/grdimage.c:1313:46: note: each undeclared identifier is reported only once for each function it appears in
/home/runner/work/gmt/gmt/src/grdimage.c:1313:72: error: ‘y’ undeclared (first use in this function)
 1313 | #pragma omp parallel for private(srow,kk_s,k,x,scol,node_s,node_i,fill,y,dim) shared(GMT,Conf,Ctrl,H_s,H_i,image)
      |                                                                        ^
/home/runner/work/gmt/gmt/src/grdimage.c:1313:74: error: ‘dim’ undeclared (first use in this function); did you mean ‘idim’?
 1313 | #pragma omp parallel for private(srow,kk_s,k,x,scol,node_s,node_i,fill,y,dim) shared(GMT,Conf,Ctrl,H_s,H_i,image)
      |                                                                          ^~~

Did not test OpenMP so missed some new variables.

PaulWessel · 2023-12-28T13:02:01Z

Oops, was not using OpenMP here so that was never exercised. Fixed now I hope - let me know. Removed x, added ix,iy as shared, and idim as private.

seisman · 2023-12-28T13:16:02Z

Still fails:

/home/runner/work/gmt/gmt/src/grdimage.c: In function ‘grdimage_img_variable_transparency’:
/home/runner/work/gmt/gmt/src/grdimage.c:1313:75: error: ‘y’ undeclared (first use in this function)
 1313 | #pragma omp parallel for private(srow,kk_s,k,idim,scol,node_s,node_i,fill,y,dim) shared(GMT,Conf,iy,ix,Ctrl,H_s,H_i,image)
      |                                                                           ^
/home/runner/work/gmt/gmt/src/grdimage.c:1313:75: note: each undeclared identifier is reported only once for each function it appears in
/home/runner/work/gmt/gmt/src/grdimage.c:1313:77: error: ‘dim’ undeclared (first use in this function); did you mean ‘idim’?
 1313 | #pragma omp parallel for private(srow,kk_s,k,idim,scol,node_s,node_i,fill,y,dim) shared(GMT,Conf,iy,ix,Ctrl,H_s,H_i,image)
      |                                                                             ^~~
      |                                                                             idim

PaulWessel · 2023-12-28T13:18:29Z

Sorry, blindness. Now y is gone and dim is idim

seisman · 2023-12-28T13:24:21Z

/home/runner/work/gmt/gmt/src/grdimage.c: In function ‘grdimage_img_variable_transparency’:
/home/runner/work/gmt/gmt/src/grdimage.c:1313:33: error: ‘idim’ appears more than once in data clauses
 1313 | #pragma omp parallel for private(srow,kk_s,k,idim,scol,node_s,node_i,fill,idim) shared(GMT,Conf,iy,ix,Ctrl,H_s,H_i,image)

PaulWessel · 2023-12-28T13:31:30Z

Amazingly stupid. Sorry about this @seisman but hopefully this nʻth iteration works.

seisman · 2023-12-28T13:37:54Z

Yes, it works, and please add #!/usr/bin/env bash to ./test/grdimage/image_vartrans.sh

PaulWessel · 2023-12-28T13:47:11Z

Thanks, and done.

joa-quim · 2023-12-28T14:13:34Z

This still crashes

gmt grdimage RandomGeoTIFFWithAlpha.tiff -png lixo

joa-quim · 2023-12-30T00:17:22Z

Tried to fix the indexed images case with a transp value set by the nan_value by adding a I->alpha but didn't work. Debugged but couldn't find why.

joa-quim · 2023-12-30T00:39:29Z

The trans_mix test and image_vartrans fail for me with corrupted PS. Here goes one (not finished PS).
transp_mix.ps.zip

PaulWessel · 2023-12-30T09:01:31Z

Two issues I think.

You have to run dvc pull to get the latest PS originals.
As for the other branch (img-layout) I had forgotten to merge master into it.

With grdimage-upgrade branch selected and git pulled, and dvc pulled everything works just fine. No reason this should not work for you as well. Please test again.

joa-quim · 2023-12-30T11:22:39Z

You haven’t open the PS I attached. The problem is more serious and I don’t know how to debug it. Nor have time these next 3 days.

PaulWessel · 2023-12-30T11:47:55Z

I did look at it, and was pushed, so I ran transp_mix.sh again and it crashed. Reason was I had not updated git etc as I mentioned so it complained that grdimage -Q+i was not available (it wasn't before). After git pull etc and rebuild it ran fine. So please run transp_mix.sh again manually (maybe bash -xv transp_mix.sh) and see if you still get any errors or message, and let me know. Hard to debug what does not crash here but it is clearly at the end of the square-transparency call.

PaulWessel · 2023-12-30T11:57:48Z

Comparing your and my PS. While I have

PSL_font_encode 0 get 0 eq {ISOLatin1+_Encoding /Helvetica /Helvetica PSL_reencode PSL_font_encode 0 1 put} if
0 -114 M 142 F0
(180°) tc Z

...
you have

PSL_font_encode 0 get 0 eq {Standard+_Encoding /Helvetica /Helvetica PSL_reencode PSL_font_encode 0 1 put} if
0 -114 M 142 F0
(180�) tc Z

Not sure why you use Standard+ and how, and the degree symbol is an odd glyph?

You also have a lot of color setting duplicates, e.g.,

11.8 -12.2 1582.7 838.6 SS
{0.924 0.958 0.965 C} FS
{0.924 0.926 0.936 C} FS
{0.924 0.926 0.936 C} FS
{0.924 0.926 0.936 C} FS
{0.924 0.926 0.936 C} FS
11.8 -12.2 968.5 732.3 SS

which I dont understand how can happen. I dont see that since they are the same color as before. But, it is possible this is some roundoff causing trouble when I check if next color is the same or not as the previous. I'll see how that is handled.

PaulWessel · 2023-12-30T12:01:15Z

I use this to determine if the new and old color are the same:

#define PSL_same_rgb(a,b) (PSL_eq(a[0],b[0]) && PSL_eq(a[1],b[1]) && PSL_eq(a[2],b[2]) && PSL_eq(a[3],b[3])) /* If two colors are ~identical */

with

#define PSL_eq(a,b) (fabs((a)-(b)) < PSL_SMALL) /* If two color component are ~identical */

and PSL_SMALL = 1.0e-10.
Maybe that is too small on Windows. We use 10-9 in many places and certainly should be plenty here. Maybe you can try 10-9 and see if you get any repeat color lines.

joa-quim · 2023-12-30T16:37:03Z

I was using latest git (nothing new comes out on a git pull)

It would be a disaster if different compilers gave different results on a comparison to < 1e-10. They all round off to same numbers, that's IEEE thing. A thing that my change it is the use of fast-math that should not be used to build the ref PSs.

Running the transp_mix.sh script directly and with debug mode shows:

j@dell-from-hell MINGW64 /c/v/build/test/grdimage/transp_mix
$ ./transp_mix.sh
PSL:C:\progs_cygw\GMTdev\gmt5\master\src\postscriptlight.c:6188: Error: Could not allocate memory [13881334300.86 Gb, 14904969211763476135 items of 1 bytes]
PSL:C:\progs_cygw\GMTdev\gmt5\master\src\postscriptlight.c:6188: Error: Could not allocate memory [8680354956.13 Gb, 9320460163558510294 items of 1 bytes]
./transp_mix.sh: line 34:  1186 Segmentation fault      gmt grdimage rgba.tif

PaulWessel · 2023-12-30T16:51:02Z

Sorry, but I cannot determine what goes wrong on Windows. You should be able to place a stop point at line 607 and 618 and then go up and see why you get crazy sizes?

joa-quim · 2023-12-30T17:31:57Z

Shit happens in gmt grdimage rgba.tif -Rd -JQ9c

It's the OMP block in grdimage#L1313. I just learned that I can't debug inside it. The fill struct is invisible but when I let it go inside gmt_setfill I can see it has lots of garbage.

Commenting the OMP makes it work

PaulWessel · 2023-12-30T18:51:33Z

Ah OpenMP. That makes sense since Xcode does not have it. K, will do the gcc build we use for the release then and I suspect I will figure that out quickly, Thanks!

joa-quim · 2023-12-30T19:41:04Z

We should have checks to see if the OMP blocks actually speed up or not.

PaulWessel · 2023-12-30T21:48:00Z

Built with OMP and got crash etc so will work on this tomorrow.

PaulWessel · 2023-12-31T12:02:36Z

So learning a few things about OPenMP here. The initial issue was that even though I memset fill to 0 before the loops and use fill as a private variable for each thread, it clearly was not passed in to each thread as empty. So the PS had output in it as if it had pattern fill. Just junk.

I then placed the memset init inside the for loop over columns so it was set to NULL before we start filling in fill.rgb. Now we have no more image, pattern crap. But, still problems. The issue is at least partly the check in PSL if the new rgb differs from the last one sat. Well, what does that mean when there are 10 threads doing this and the PSL is doing this

else if (PSL_same_rgb (rgb, PSL->current.rgb[PSL_IS_FILL]))
	{ /* Skipped, fill already set */ }

This test fails because we implemented this to think that if we had just set the same rgb then skip it, but "just" does not work in parallel mode. So we end up with duplications in the PS:

{0.514 0.514 0.593 C} FS
{0.514 0.514 0.593 C} FS
{0.514 0.514 0.593 C} FS
{0.514 0.514 0.593 C} FS

And once in a while we get a junk character:

{0.53 0.514 0.593 C�} FS

Those are the things that crash gs, not the duplications.

Conclusion seems to be we cannot do OPM if we change color or transparency inside those loops. So probably no point with OPM in those cases. This is very different from the calculation OPM which does not depend on some recently set variable like in PSL.

Agree on reming OpenMP when PSL functions expecting linear memory is engaged, @joa-quim ?

joa-quim · 2023-12-31T12:10:42Z

Sure. If it can’t be used.

PaulWessel · 2023-12-31T12:12:37Z

Gonna try one more thing. We have no gmt_setcolor function (only set fill) which complicates this as I need to pass that fill structure that the f**s us. If that still fails then most of the loops with OPM are ok; it is only the new ones with fill.rgb that fails (hence most tests were not affected - just the new stuff). Will update you soon

PaulWessel · 2023-12-31T12:45:04Z

Removed only the OPENMP loop that surrounds the plotting of squares since that is where the color and previous color enters. See how it works for you now.

PaulWessel · 2023-12-31T13:15:52Z

One issue. Here is revised trains_mix.sh output via PDF (we had two identical plots so I replaced on with a constant 70% transparency):

To make the fake transparency, which goes towards 1 in UR show that behavior I had to add +i to grdmix -Atransparency.nc+i, basically saying I am passing opacity (so it then converts it). But that does not make sense to me. We are passing transparency so perhaps the grdmix internals need to have a 1-x correction if +i is not given so that the image matches what we expect?

PaulWessel · 2023-12-31T14:55:24Z

Updated some PS files (dvc pull) and all pass except two colorbar categorical plots due to the new #8246 PR. FYI, I will remove that as it messed with the placement of the label and causes two failures. I think the best plan is to

Just implement the < and > label stuff for now but leave positioning and guessing of panel dimensions for now.
After 6.5, spend the effort to determine the panel dimension in PostScript based on actually size of the labels. As it is, categorical colorers with AAA-BBB label cannot correctly set the panel dimensions and one must use the +c clearance trial-and-error. I'd rather make that modifier obsolete by correctly computing the PS dimensions, next year.

joa-quim · 2023-12-31T18:49:43Z

See how it works for you now.

$ ./transp_mix.sh
grdimage [ERROR]: Option -Q parsing failure.  Correct syntax:

  -Q<color>
      Specify <color> as one of:
       • <gray> or <red>/<green>/<blue>, all in range 0-255;
       • <cyan>/<magenta>/<yellow>/<black> in range 0-100%;
       • <hue>-<saturation>-<value> in ranges 0-360, 0-1, 0-1;
       • Any valid color name.
     For PDF fill transparency, append @<transparency> in the range 0-100% [0 = opaque].

let's finish this. I cannot be at the computer today and tomorrow.

This was in master. With the branch it still crashes.

PaulWessel · 2023-12-31T20:14:30Z

I know you can't. And I cannot work at the moment but maybe a bit later.

seisman · 2024-01-07T17:37:33Z

As I understand it, the PR is already superseded by PR #8251. So closing.

PaulWessel added 7 commits December 28, 2023 10:39

Update grdimage.c

f731c0f

Moving parts over from old branch

More brought over and still no failures

31b6053

Update grdimage.c

d75477b

Update grdimage.c

f6b34e4

More copying with no failures

d6147bd

More OK updates

8d4f279

Update grdimage.c

c460c10

PaulWessel added the enhancement Improving an existing feature label Dec 28, 2023

PaulWessel added this to the 6.5.0 milestone Dec 28, 2023

PaulWessel self-assigned this Dec 28, 2023

Add test grid_transp.sh

9a48641

Handle -Q+i for image

6da3529

PaulWessel added 2 commits December 28, 2023 13:57

Add transp_mix test and PS

52031da

Update grdimage.c

11c8e93

Did not test OpenMP so missed some new variables.

PaulWessel added 2 commits December 28, 2023 14:07

Update transp_mix.sh

7dc56b3

Update grdimage.dvc

d370d89

Fix OpenMP issue

10b8f33

Add image_vartrans.sh test and PS to DVC

1539c28

Update grdimage.c

ff1a259

Update image_vartrans.sh

a1ee70e

Merge branch 'master' into grdimage-upgrade

6790ee8

Merge branch 'master' into grdimage-upgrade

41df65d

PaulWessel mentioned this pull request Jan 1, 2024

Final version of updates for 6.5: grdimage, colorbar, grdmix #8251

Merged

seisman modified the milestones: 6.5.0, 6.6.0 Jan 7, 2024

seisman closed this Jan 7, 2024

seisman deleted the grdimage-upgrade branch January 7, 2024 17:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Final grdimage upgrade related to transparency #8241

Final grdimage upgrade related to transparency #8241

PaulWessel commented Dec 28, 2023

github-actions bot commented Dec 28, 2023 •

edited

Loading

Added images

Modified images

seisman commented Dec 28, 2023

PaulWessel commented Dec 28, 2023

seisman commented Dec 28, 2023

PaulWessel commented Dec 28, 2023

seisman commented Dec 28, 2023

PaulWessel commented Dec 28, 2023

seisman commented Dec 28, 2023

PaulWessel commented Dec 28, 2023

joa-quim commented Dec 28, 2023

joa-quim commented Dec 30, 2023

joa-quim commented Dec 30, 2023

PaulWessel commented Dec 30, 2023

joa-quim commented Dec 30, 2023 •

edited

Loading

PaulWessel commented Dec 30, 2023

PaulWessel commented Dec 30, 2023

PaulWessel commented Dec 30, 2023

joa-quim commented Dec 30, 2023

PaulWessel commented Dec 30, 2023

joa-quim commented Dec 30, 2023 •

edited

Loading

PaulWessel commented Dec 30, 2023

joa-quim commented Dec 30, 2023

PaulWessel commented Dec 30, 2023

PaulWessel commented Dec 31, 2023

joa-quim commented Dec 31, 2023

PaulWessel commented Dec 31, 2023

PaulWessel commented Dec 31, 2023

PaulWessel commented Dec 31, 2023

PaulWessel commented Dec 31, 2023

joa-quim commented Dec 31, 2023 •

edited

Loading

PaulWessel commented Dec 31, 2023

seisman commented Jan 7, 2024

Final grdimage upgrade related to transparency #8241

Final grdimage upgrade related to transparency #8241

Conversation

PaulWessel commented Dec 28, 2023

github-actions bot commented Dec 28, 2023 • edited Loading

Summary of changed images

Image diff(s)

Added images

Modified images

seisman commented Dec 28, 2023

PaulWessel commented Dec 28, 2023

seisman commented Dec 28, 2023

PaulWessel commented Dec 28, 2023

seisman commented Dec 28, 2023

PaulWessel commented Dec 28, 2023

seisman commented Dec 28, 2023

PaulWessel commented Dec 28, 2023

joa-quim commented Dec 28, 2023

joa-quim commented Dec 30, 2023

joa-quim commented Dec 30, 2023

PaulWessel commented Dec 30, 2023

joa-quim commented Dec 30, 2023 • edited Loading

PaulWessel commented Dec 30, 2023

PaulWessel commented Dec 30, 2023

PaulWessel commented Dec 30, 2023

joa-quim commented Dec 30, 2023

PaulWessel commented Dec 30, 2023

joa-quim commented Dec 30, 2023 • edited Loading

PaulWessel commented Dec 30, 2023

joa-quim commented Dec 30, 2023

PaulWessel commented Dec 30, 2023

PaulWessel commented Dec 31, 2023

joa-quim commented Dec 31, 2023

PaulWessel commented Dec 31, 2023

PaulWessel commented Dec 31, 2023

PaulWessel commented Dec 31, 2023

PaulWessel commented Dec 31, 2023

joa-quim commented Dec 31, 2023 • edited Loading

PaulWessel commented Dec 31, 2023

seisman commented Jan 7, 2024

github-actions bot commented Dec 28, 2023 •

edited

Loading

joa-quim commented Dec 30, 2023 •

edited

Loading

joa-quim commented Dec 30, 2023 •

edited

Loading

joa-quim commented Dec 31, 2023 •

edited

Loading