-
Notifications
You must be signed in to change notification settings - Fork 401
Add option to not create a temporary EPS file but instead edit the original PS. #8417
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
Conversation
|
May I ask why things are done this way? (Doing the EPS-stuff, instead of using the PS, as is the goal here) |
|
There are things I don't know. Namely, why GMT PS have a |
|
Thanks @joa-quim. |
|
Not sure if I'm using it correctly, but it crashes for the following command: |
|
Oh ghrrr. That works fine for me on Windows. Can you try it with a debug build. Maybe the stack trace will be more informative. |
Just tried the debug build. The stack trace is the same. One question: even if it doesn't crash, does it mean that the original PS file on the disk will be broken since it's edited when calling psconvert? Or the editing is done to the PS file in memory? |
|
The -! should not break the original PS file but changes it to become equal to what we get with -Te. No in-memory processing though that is already possible when called from Matlab & Julia but I have not tried this in long time. One idea is that would be useful to the wrappers that don't really care about the original PS. |
|
Can you check the beginning of the map.ps file and see if any of the BoungingBoxes have been changed? Original After change |
|
There are no changes in the PS file. |
|
So it must have crashed on lines 2366 or 2368 I'm opening the PS file with |
I tried to add a print statement in the |
|
Please try again. It was a dumb error in the |
Yes, now it works on Linux, but as I asked before, now I can confirm that the original PS file is actually a EPS file after editing. I'm wondering can we just read the PS file and send it to Ghostscript via stdin, like https://stackoverflow.com/questions/70706765/passing-stdin-to-ghostscript-in-php-proc-open. |
|
How to test this? Just running a bunch of scripts? |
Well, yes but what I did was to hardwire the -! option by adding a line (~line 1735 of psconvert.c) and run all the tests. But even all the tests do not probably test everything. |
The The still-in-the-air idea is to one day be able to call the Ghostscript API directly to do these operations. This would free us from the system calls and popen/popen2 troubles. |
And not using popen/popen2 have what benefits? |
|
The |
In that case, do not try to touch any further BoundingBox found.
This seems to one of the last bit to make all the tests pass even with /DPS_NO_DUP
Tried to extend this to modern mode but it breaks all modern mode tests.
|
This can now also crop a pure PS file, but only in classic mode, and needs that the original PS has the wall paper size (32767x32767). Because we are creating another PS from an input PS, the output, when file name was not set via -F, will be named by appending a Tried to extend this to modern mode but I lack a syntax for that. Tried to introduce a small breakage of creating a wall sized PS by default, instead of A4 bit it breaks all modern mode tests. |
|
Ping. I would like to merge this branch. All tests are not affected and recall that without the -! explicitly set this PR should behave as before. |
|
Ping |
|
How can I test this, if necessary? |
|
If built with |
|
Ok, so should I just approve? 😄 |
|
O would like also a confirmation that the pygmt tests are not affected, but if that doesn’t come … |
|
Alright, I can't help with pythonstuff. Let's wait some more.. |
|
Generally speaking, this PR is good to me. When I run the following test, the jpeg version is good, but for the PDF output, it's not perfectly cropped (there are some extra spaces at the top). |
|
Thanks, I will want to fix this somehow. The issue here is that the second time one runs the |
…no subsequent. Problem was that dimension were modified to Portrait but Orientation in header remained unchanged saying it was Landscape.
|
OK, fixed that one too. It was an issue with Landscape files that were left in a contradictory state after first run. |
It has always annoyed me (e., g. #1435) that for running
psconvert(and before,ps2raster) we must create a temporary EPS file that is the one we sent through the Ghostscript guts to get our raster file (I hate solutions based on temporary disk files). No big deal if the PS file is small, but far from ideal if it's a hundreds MB file.This PR adds an option to
psconvertthat avoids that by editing the original PS turning it into what before we obtained with EPS temporary file. Due to shortage of letters, excluding the global options we have no more alphabet letters available, that option was made -!, which inspired in the Julia convention that bang (!) symbol is used for data modifying functions.I have also added run time reports via the -Vt option and, a bit disappointingly, it takes big PS files to show small run times differences between master and this PR (I do have a fast SSD disk, is it it?).
Run the tests with an hard-coded -! so that also the modern mode scripts use it and saw no obvious failures, but certainly not all
psconvertpaths were tested. So I would like to ask for more testing on this. If it proves reliable, the idea is to make this PR the default behavior and add some flag as a modifier of an existing option to keep the ancient behavior.PLEASE TEST