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

Realigning order of basemap frame, gridlines, and annotations/ticks for all modules #4274

Merged
merged 86 commits into from
Oct 13, 2020

Conversation

PaulWessel
Copy link
Member

Description of proposed changes

See #4265 for contest. We want to add consistency to how GMT places the "basemap" on top of or below a plot within the same module (obviously, stacking modules can override any default arrangements). In general, we now place the optional gridlines before plotting data, but include images and polygons (e.g., via pscoast) in which case gridlines goes on top. We generally lay down the frame on top to ensure any lines or symbols at or near the boundary do not "eat" into the frame width, but if the module has a no-clip option like -N then we draw it first so symbols can straddle that line. Annotations and tick-marks are usually laid down before the plot but in the case of MAP_FRAME_TYPE=inside then we must do it at the end instead. There are also some exceptions for true 3-D plots.

Implementation is done via a new function gmt_set_basemap_orders which sets, for each of the discussed items, what the order of plotting should be. For instance, in pscoast it may say

gmt_set_basemap_orders (GMT, clipping ? GMT_BASEMAP_FRAME_BEFORE : GMT_BASEMAP_FRAME_AFTER, GMT_BASEMAP_GRID_AFTER, GMT_BASEMAP_ANNOT_AFTER);

where clipping options may require the frame to be drawn first instead of last, but otherwise we draw items later. Each plot module has such a call and the arguments vary. The function itself can override the settings based on some of the conditions discussed above.

After implementation, we now have ~180+ "failures" in the tests. Most of them are of these types:

Some plots that had gridlines on top now has gridlines on the bottom, avoiding cutting through symbols and bars
Some plots with polygons or bars near a border (e.g., histograms) now have a clean frame line without notches
Many base frames have subtle changes because tick marks are not plotted and are visible whereas in some cases they were covered up.

I have labeled this PR WIP because I think we need to actually look at all these to see if anything got lost in the translation that we want to preserve.

@PaulWessel PaulWessel self-assigned this Sep 30, 2020
@PaulWessel
Copy link
Member Author

I know this is a big tasks to look at those 180 PNG and PDFs but I think we need all of us to do this in case there are some subtle regressions. I've stared at these for a while and the results look good to me. Let me know of you see anything that should not change, @joa-quim and @seisman.

@joa-quim
Copy link
Member

I got only 133 failures in total.

@PaulWessel
Copy link
Member Author

That is a new one - less failures on Windows than macOS (186 right now).

@joa-quim
Copy link
Member

Ex16 is partially eating the frame

@PaulWessel
Copy link
Member Author

Yes, I need to fix the setting for grdview. It draws before but shoud be after (unless 3-D etc).

@seisman
Copy link
Member

seisman commented Sep 30, 2020

  • doc/script/GMT_inset.sh: Tiny changes in the coastline
  • ex04, ex05: Tiny changes in the title string
  • ex08: The z-axis at the back should be invisible
  • ex11: this example is messy now.
  • ex15: in panel (d), some pixels overlap with the frame. The old PS looks better to me.
  • ex16: @joa-quim already mentioned above

@joa-quim
Copy link
Member

joa-quim commented Sep 30, 2020

grdcut/origin. The inner pajama frame is overlapped
image

@PaulWessel
Copy link
Member Author

  • doc/script/GMT_inset.sh: Tiny changes in the coastline
    No idea why coastline is affected, but perhaps I removed a pen reset - will look
  • ex04, ex05: Tiny changes in the title string
    Unsure about what that is
  • ex08: The z-axis at the back should be invisible
    working on that
  • ex11: this example is messy now.
    Yes must be a major error I missed
  • ex15: in panel (d), some pixels overlap with the frame. The old PS looks better to me.
    Fixing that in mask
  • ex16: @joa-quim already mentioned above
    Same, fixing in grdview

@PaulWessel
Copy link
Member Author

grdcut/origin. The inner pajama frame is overlapped

But that was always the case. We do -B in the first command and then pile on. This needs moving the -B to the last call.

@PaulWessel
Copy link
Member Author

I think we have many tests where we plot numerous overlays and often start with -Baf. To follow our plan I think these need to move to the last command. Finally, we do not yet have a solution for subplots since it will add -B settings to the first command in the panel, not last. Changing this is harder and will be pursued after this PR.

@joa-quim
Copy link
Member

grdimage/grdclip

image

@joa-quim
Copy link
Member

grdview/categorical

image

@PaulWessel
Copy link
Member Author

Apologies for fixing #4181 in this PR but I wanted to be safe it did not break other things. The double frame is now gone.

@seisman
Copy link
Member

seisman commented Oct 11, 2020

: The new PS file looks great in ghostview (gv) and test passes, but if I tried open anim04.ps then Preview would show the black checkerboard at the wrong angle.

Yes, we saw it before and we thought it may be a Preview bug.

@seisman
Copy link
Member

seisman commented Oct 11, 2020

I have two more failing tests than you, but there are just tiny differences:

	596 - test/psbasemap/asymm_grid.sh (Failed)
	640 - test/pscoast/placement.sh (Failed)

@PaulWessel
Copy link
Member Author

So OK with me updating the orig PS in this PR then?

@seisman
Copy link
Member

seisman commented Oct 12, 2020

Yes to me.

@PaulWessel
Copy link
Member Author

Hi @seisman ane @joa-quim, I think we have reached the end of the line on this PR. This clearly improved the quality of the plots. The one thing remaining is related to subplot panels where the -B happens on the first panel but perhaps should always be last. That will require more work since we cannot know we are last until we either move to a new frame or we are done. That is a separate PR.

@seisman
Copy link
Member

seisman commented Oct 12, 2020

../../src/psrose.c:799:10: warning: unused variable 'dim' [-Wunused-variable]
                double dim[PSL_MAX_DIMS];

@PaulWessel
Copy link
Member Author

Hi @seisman, seems a CI failure but nothing to be concerned about? @joa-quim, may be helpful if you could run all the test one more time on this branch. I think all is well now with expected result apart form the notes on subplot panel orders.

@seisman
Copy link
Member

seisman commented Oct 12, 2020

Hi @seisman, seems a CI failure but nothing to be concerned about?

Yes, just an occasional network connection issue.

@joa-quim
Copy link
Member

Same number of failures (16). Some, doc/scripts/GMT_tut_11.sh, 18, etc fail because the grid is slightly different. Don't get it.

@seisman
Copy link
Member

seisman commented Oct 12, 2020

I just made a commit 784347f to trigger full tests in CI. Will know if all tests pass in CI in 2 hours.

@seisman
Copy link
Member

seisman commented Oct 13, 2020

This PR is good to me, but please check this file test/psxy/clipping5.ps.

image

@PaulWessel
Copy link
Member Author

OK, soon after finishing another PR.

@PaulWessel
Copy link
Member Author

So clipping5.sh is flagged as GMT_KNOWN_FAILURE. I think it is not improved or degraded by this PR though; its solution lies elsewhere.

@seisman
Copy link
Member

seisman commented Oct 13, 2020

So clipping5.sh is flagged as GMT_KNOWN_FAILURE. I think it is not improved or degraded by this PR though; its solution lies elsewhere.

OK, I didn't notice that. The CI looks good. One failure on Linux and 7 failures on Windows. They are all known failures.

@seisman
Copy link
Member

seisman commented Oct 13, 2020

I just disabled the full tests in e529b14. Now this PR looks good to me.

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.

3 participants