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

Simplify grdimage part 1 #4163

Merged
merged 13 commits into from
Sep 12, 2020
Merged

Simplify grdimage part 1 #4163

merged 13 commits into from
Sep 12, 2020

Conversation

PaulWessel
Copy link
Member

Description of proposed changes

Eliminate the coding for three grids (r,g,b) by having that as a deprecated case that we call grdmix to assemble into a proper image, then proceed with the image. The slightly simpler grdimage works well and all test passes except the one I just added for r,g,b grids. It should look like this:

rgb_grids

but because we get

grdimage [WARNING]: The image memory layout (TRP ) is of a wrong type. It should be BRPa.

I end up with this image:

rgb_grids

Looks like we dont have a convert case for TRP to BRP in gmt_api.c anyway. Bedtime here, maybe @joa-quim has ideas. I note the shading is in the right spot so it is just the interleaving that is clearly different. Is it line vs pixel interleaving you think?

@PaulWessel PaulWessel added this to the 6.2.0 milestone Sep 4, 2020
@joa-quim
Copy link
Member

joa-quim commented Sep 4, 2020

THat error message is very annoying because all the times I saw it, it was never true. There must be some incomprehension on what is bottom and top. Also, it can't explain the patch pattern in second image because going from TRP tp BRP is the same as an UD flip.

@PaulWessel
Copy link
Member Author

I will pick a much smaller test, say 4x4 pixels and map out what is happening, in a bit.

@PaulWessel
Copy link
Member Author

The above mess was just me failing to pass -N to grdmix, since grdimage expects r,g,b in 0-255 range and grdimix has an option to acceot normalized values [default] or 0-255 and then divide by 255. Passing -N and I get the expected result. I still get the annoying message though, @joa-quim.

@PaulWessel
Copy link
Member Author

PaulWessel commented Sep 5, 2020

One issue that is coming up is ex11: It actually uses r,g,b grids for each of the 6 sides. So rather silly to have an example highlighting a deprecated option. The various options are

  1. Rewrite ex11 to use (a) pixel-registered grids and (b) convert three into an image for grdimage to plot
  2. Relegate ex11 to the cookbook discussion on color and put something more appropriate into the gallery
  3. Leave input of r,g,b grids as a documented option in grdimage
  4. ???

@joa-quim
Copy link
Member

joa-quim commented Sep 5, 2020

The best is probably shut up this particular message. But on the other hand it reveals that we don't know what is bottom and what is top.

@PaulWessel
Copy link
Member Author

So I have merged in the bug-fixes from master, relegated that annoying message to information and softened the language (since plots seems OK). Even ex11 passes (unresolved is use or rgb grids as input though). However, I have two failed scripts, both GDAL and image related. Neither involves grids (so the grdmix step - which tests show works) is not engaged. The failures are

433 - test/grdimage/grdimage_img_tif.sh (Failed)
441 - test/grdimage/readwrite_withgdal.sh (Failed)

The nature of the failures is that they exceed the threshold but if you look at the PDF or PS it is not clear why. If you could run

ctest -R 'grdimage_|readwrite'

after building this branch and let me know if you noticed anything funny then that would be great. When I started this branch I hoped to do a small update (here, eliminate the 3-grid loop stuff) and then merge in and start a new branch to tackle another aspect. So I need to resolve these failures one way or the other. Note these examples do not exercise the linear negative negative scaling bug fixed yesterday, so not sure why they fail. The one sub plot taht uses a projection is identical, so it is just the linear cases that is slightly different, which corresponds to the section of code I made changes to for fixing those bugs. I am posting the two PNG diff images here. Meanwhile, I will stare at the linear code to see if I made an obvious error.

grdimage_img_tif

readwrite_withgdal

@joa-quim
Copy link
Member

joa-quim commented Sep 5, 2020

relegated that annoying message to information and softened

That was not such a good idea. That warning message was very helpful in Julia development. It happens that this particular one TRP vs BRP has shown to be wrong.

Regarding the tests, they also fail for me but can't see why. Note that the two that seem to be right in readwrite_withgdal also fail around the edges.

@PaulWessel
Copy link
Member Author

OK, you want me to let through the warning except for this case with TRP vs BRP then, yes?

@joa-quim
Copy link
Member

joa-quim commented Sep 5, 2020

Yes, that was my idea.

@PaulWessel PaulWessel changed the title WIP Simplify grdimage part 1 Simplify grdimage part 1 Sep 12, 2020
@PaulWessel
Copy link
Member Author

The new coarse PNG plotting test added to master reveals the same issue, which is a registration failure for images apparently. Compare this failure PNG image to the failure picture below Vader above:

png_image

as shown in #4204, this problem is fixed in this branch, meaning the two failures involving images (readwrite_withgdal and grdimage_img_tif) are due to bad PostScript originals. I have added updated PS files for those two. All tests for grdimage now passes.

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.

2 participants