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

Added option to export text as object or path #1992

Merged
merged 9 commits into from Jun 8, 2016

Conversation

Projects
None yet
5 participants
@aashish24
Contributor

aashish24 commented May 26, 2016

No description provided.

@aashish24 aashish24 force-pushed the vector_graphics_text_as_object branch from ce2e03a to eb98211 May 26, 2016

@aashish24 aashish24 force-pushed the vector_graphics_text_as_object branch from 2a3d71d to 0b2065a May 26, 2016

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 26, 2016

@durack1 these are actually ps files but since github does not allow ps to be uploaded, I changed them to pdf extension. Could you please look into these two? Make sure to change the extension to ps after you download them.

foo_as_object.pdf
foo_as_paths.pdf

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 26, 2016

Ref #1537

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 26, 2016

@durack1 it would be great if you can look at this soon for 2.6. I will add a test and if you are happy with the API and results, I will merge it.

@durack1

This comment has been minimized.

Member

durack1 commented May 26, 2016

@aashish24 thanks.. I personally like having the text objects, which then import very neatly into graphics packages.. I also note that the file sizes are markedly different - with the _text object file 5KB, whereas the _text paths file is 103KB this would be a major issue in much more complex figures.

The text objects (left panel) also display more cleanly on my mac, at least for your example files:
160526_vectortextoutput

I do wonder whether some of this needs tweaking though, it seems that you're plotting the text objects/paths outside of a bounding box (so compare the left side of the fig above to the file opened using a vector graphics package below):
foo_as_object_il

@durack1

This comment has been minimized.

Member

durack1 commented May 26, 2016

@aashish24 I also wonder whether you've managed to grab all the formats - so pdf,ps,eps and svg should all have an option to export text as object - you might be missing the eps

Also what is the changed syntax for calling this new functionality?

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 26, 2016

I do wonder whether some of this needs tweaking though, it seems that you're plotting the text objects/paths outside of a bounding box (so compare the left side of the fig above to the file opened using a vector graphics package below):

@durack1 that's just my sample script and the code that decides whats in / out is not controlled by my changes.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 26, 2016

pdf,ps,eps and svg

pdf, ps, and svg is all covered. I didn't see eps (is that ghostscript?).

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 26, 2016

@aashish24 thanks.. I personally like having the text objects, which then import very neatly into graphics packages

Cool! Yes text as object is the default like you wanted 😄

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 26, 2016

@durack1 I have to add a test but other than that if you like everything else then I am going to go ahead with adding some test for it.

@durack1

This comment has been minimized.

Member

durack1 commented May 26, 2016

@dlonie can you remind @aashish24 and myself why the export text as paths was the default? And while you're at it, can you suggest any fringe test cases that should be included to make sure that the proposed change (converting all vector graphic output formats to write text as objects) doesn't trip things over

@allisonvacanti

This comment has been minimized.

Contributor

allisonvacanti commented May 27, 2016

This is mainly a rehash of what I said in #1527 and #1537, but maybe it'll clear things up?

The output formats supported by gl2ps (which VTK uses for postscript/pdf/svg/etc vector exports) handle text objects inconsistently. For example, postscript mangles newlines, pdf doesn't fully support rotation and alignment, stuff like that. These are limitations in the actual format specifications themselves.

On top of that, embedding text objects then relies on the viewer to locate a similar font and render the text, and odds are good that the fonts used by the viewer will have different characteristics than the ones used in the original rendering. So, for instance, you have some right-justified lines of text, like the data at the top of the VCS plots. If the font used by the viewer uses different widths for any of glyphs composing the text, the text will be unaligned along the right-hand side, since the text is always anchored on it's left side due to how these formats represent text objects. This just looks bad.

Exporting text as paths eliminates all of these problems with portability across viewers and inconsistent text object handling between output formats.

For testing, it's somewhat pointless to do in this case, IMO. The differences between backends (e.g. no newlines in postscript) will always be there when text is exported as objects -- text objects cannot correctly represent the rendering in that case. Same with aligned rotations in pdf. As for portability differences, you can't fully test for that, as different platforms/viewers will use different fonts, and they'll always look bad for certain cases (e.g. right-justified text). Trying to regression test using system-dependent fonts is a sure-fire way to have unstable and unreliable tests.

My recommendation is the same as in #1527 -- leave paths as the default, and allow objects as an option. This way things will work for the 99% of users who just want an accurate reproduction of their rendering, and the 1% that wants to tweak the text after exporting will still be able to easily.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented May 27, 2016

I agree with @dlonie , Also @chaosphere2112 can we copy paste this excellent explanation on our wiki. @aashish24 this explanation should also go in the vector format output doc strings.

@durack1

This comment has been minimized.

Member

durack1 commented May 27, 2016

@doutriaux1 ditto from me for every statement you made above. I think the description from @dlonie should be everywhere.. And his defaults suggestion too..

@durack1

This comment has been minimized.

Member

durack1 commented May 27, 2016

@aashish24 @doutriaux1 I also think a documentation of file size implications is also useful info for docstrings and web docs.. My 5KB vs 103KB is a starter very basic example above

@durack1

This comment has been minimized.

Member

durack1 commented May 27, 2016

@dlonie just a quick thanks for taking the time to write that description above.. It's certainly very useful to all of us to have your input!

# #
# #
# Screen Encapsulated PostScript wrapper for VCS. #
# #
##########################################################################
def eps(self, file, mode='r', orientation=None, width=None, height=None, units='inches',

This comment has been minimized.

@durack1

durack1 May 27, 2016

Member

@aashish24 this also needs a , textAsObject=True appended OR conversely, these all need to be changed back so that textAsObject=False for all defaults, however the functionality is clearly documented in docstrings and the web documentation

@allisonvacanti

This comment has been minimized.

Contributor

allisonvacanti commented May 27, 2016

Happy to help.

@aashish24 aashish24 force-pushed the vector_graphics_text_as_object branch from f83bbc1 to 0b2065a May 28, 2016

@aashish24 aashish24 force-pushed the vector_graphics_text_as_object branch from 14fe569 to 92d5053 May 28, 2016

@aashish24 aashish24 changed the title from [WIP] Added option to export text as object or path to Added option to export text as object or path May 28, 2016

aashish24 added some commits May 28, 2016

@aashish24 aashish24 force-pushed the vector_graphics_text_as_object branch from 869ff2e to a90c075 May 28, 2016

@aashish24 aashish24 force-pushed the vector_graphics_text_as_object branch from 8cd0d70 to fdf6beb May 28, 2016

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 28, 2016

This discussion is very useful. I made the path as default with the option textAsPaths set to True. User can override this option when they call the method. Also I documented the reasoning behind this option in vectorGraphics method. I felt that's the best place since the reasoning seems to be implementation dependent. I will add explanation for it my rst documentation as well.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 28, 2016

One another thing, I found the eps method was broken, I fixed it too. @durack1 it would be nice if you can verify the results for all formats.

@aashish24 aashish24 force-pushed the vector_graphics_text_as_object branch from fdf6beb to f53c74a May 28, 2016

@durack1

This comment has been minimized.

Member

durack1 commented May 31, 2016

@doutriaux1 is there a way to get a branch installed using conda - so theoretically we have nightly, but how do I best test branches that are not in master?

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Jun 6, 2016

@aashish24 while it's nice to put the explanation in vectorgraphics function, we need to add it in the doc string of pdf/postscript/eps/svg, etc... These are the functions that the end user will access.

@aashish24 aashish24 merged commit c28d151 into master Jun 8, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@aashish24 aashish24 deleted the vector_graphics_text_as_object branch Jun 8, 2016

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Jun 8, 2016

I will update the rst_docs and the method post 2.6

@durack1

This comment has been minimized.

Member

durack1 commented Jun 8, 2016

@aashish24 thanks for this.. As a user I would really like inline docs to include documentation on this change, so I would advocate for a doc update prior to tagging 2.6 rather than after

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Jun 9, 2016

I totally understand @durack1. The document I am referring to is much detailed documentation which is not only for this option for other options as well. I just pushed and merged a basic vcs user-guide but I am afraid I won't have deep dive user-guide for 2.6. What I can do in the mean-time is document in "changes in 2.6" for the end-users.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Jun 9, 2016

@aashish24 See the following flake8 warnings. Is this you? 😄
[~/projects/uvcdat/build]$ /home/danlipsa/projects/uvcdat/build/install/bin/flake8 "/home/danlipsa/projects/uvcdat/src/Packages/vcs/vcs/" "--show-source" "--statistics" "--max-line-length=120"
/home/danlipsa/projects/uvcdat/src/Packages/vcs/vcs/dv3d.py:162:5: E303 too many blank lines (2)
def setProvenanceHandler(self, provenanceHandler):
^
/home/danlipsa/projects/uvcdat/src/Packages/vcs/vcs/Canvas.py:4917:5: E303 too many blank lines (2)
def postscript(self, file, mode='r', orientation=None, width=None, height=None,
^
2 E303 too many blank lines (2)

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Jun 9, 2016

def setProvenanceHandler(self, provenanceHandler):

mostly likely not me

def postscript(self, file, mode='r', orientation=None, width=None, height=None,

Might be me. Do you want me to push a fix?

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Jun 9, 2016

@aashish24 That would be great. Thanks.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Jun 9, 2016

@aashish24 @danlipsa I fixed this in my conda_build branch

@durack1

This comment has been minimized.

Member

durack1 commented Jun 9, 2016

@aashish24 just so we're all on the same page there are VCS docs available from http://uvcdat.llnl.gov/documentation/vcs/vcs.html - it'd be great to refresh and update these docs as they are the most comprehensive currently available.

I also have a preference for inline docs with functions - so using the dir(function) or print function.doc approach.. This would make it really easy for a command line VCS user to be exposed to all the new functionality without having to figure out where to point a web browser

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Jun 10, 2016

@doutriaux1 The problem is that we have a failing flake8 until your conda branch is merged. As a process, I would rather have someone who breaks master fix it, rather than many people fixing that in their branches, or waiting for a particular unrelated branch until master is fixed. What do you think?

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Jun 10, 2016

@aashish24 or anyone else could push a fix in 5 min, instead of waiting a day for your conda branch or any other branch.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Jun 10, 2016

@danlipsa I don't really care, but I really care about the conda branch being merged in sooner rather than later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment