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

Un-bloat and speed-up (FYI, not for merge) #145

Open
wants to merge 34 commits into
base: master
Choose a base branch
from
Open

Conversation

jaseg
Copy link

@jaseg jaseg commented Mar 31, 2013

Hi,
I really like your idea (thanks for your awesome work!), but I think that 2458 lines of code is a tad on the enterprise side of things and I could not stand the slowness of everything. I rewrote the whole thing, re-implementing everything (or so I hope) except for the tty pony stuff and cleaned up the repo a bit for my personal use.
I changed the command line options towards a more standard usage and added a "--center" option horizontally centering the pony on the terminal.
The new script is 117 lines long and about 50-85% faster (now limited by the startup time of the python interpreter). Especially -q (random pony with quote) is now usable for me for login shells etc.

Missing stuff:

  • doc
  • tty ponies

You may want to consider using my code, though you would have to fix these things (and probably some more which I forgot).

@maandree
Copy link
Collaborator

maandree commented Apr 1, 2013

Which branch did you start your work from?

@JotaRandom
Copy link
Collaborator

@maandree based on the number of quotes and the lack of 3.0 on CHANGELOG was master branch
@jaseg correct me if I was wrong
adittionally for @jaseg actually a HUGE change is happening on develop branch, check it

@jaseg
Copy link
Author

jaseg commented Apr 1, 2013

This is based on master.

@jaseg
Copy link
Author

jaseg commented Apr 1, 2013

@jristz I had a look on the changes in develop. As far as I can tell ponysay.py grew to 2852 lines of code and a 1211-line ponysay-tool.py appeared that appears to manage this diy metadata format (and includes an emacs-clone instead of using $EDITOR). Unfortunately I think that actually makes things worse and I think you should still consider using my code. It should be compatible with (read: able to ignore) the metadata format in develop.

I really do not think something as simple as a slightly more advanced cowsay clone (sry ;) needs to take up 4000 lines of code1. Short, concise code is more maintainable, hackable and reliable than what you currently have.

@JotaRandom
Copy link
Collaborator

and includes an emacs-clone instead of using $EDITOR
Because Editor cannot deal whit the coloring pony... and was idea of maadree

the metadata is fo give proper credits, searching and onther stuff
and yes actually we have problems with 350 quotes and increasing

@jaseg
Copy link
Author

jaseg commented Apr 2, 2013

I added a Makefile for installation/uninstallation and bash/zsh-completion. The zsh completion is even tested! ;)

@jaseg jaseg closed this Apr 2, 2013
@jaseg jaseg reopened this Apr 2, 2013
@maandree
Copy link
Collaborator

maandree commented Apr 3, 2013

Complete for bash, zsh and fish is generated, using the content in the completion directory.

@jaseg
Copy link
Author

jaseg commented Apr 6, 2013

I just wrote a script converting .pony files back to pngs that includes the metadata in the png metadata (132 lines), and I fixed my png->terminal renderer to print the png metadata in the comment field (now 98 lines). I also converted my repo so it only includes the source pngs so they can be rendered by the makefile. That way conversion is possible in both ways, all the while preserving metadata. If you wanted to use my code (which I very much advise you to do) you could probably emulate that whole group stuff with a bunch of symlinks and like three lines of code.

This fork now has everything I expect of it (which excludes the ttyponies, fancy metadata handling and 1337-page pdf) in less than 5% of the code.

btw, the zsh/bash completion is just like 30 lines each, so I do not think that warrants using an automated tool.

@maandree
Copy link
Collaborator

maandree commented Apr 6, 2013

I will look in to what you have done as soon as I can.

There is a problem with using PNG in the source, the .pony-files include more features than you might except,
for example, to my knowledge jaseg/pixelterm only supports $balloon$ with minimum width, not everything that
ponysay itself supports. util-say however does support everything, but the .pony files are more source than PNG files, because we do modification to them directly, or at least I do, without using an image editor.
Further I do not thing PNG should be used just because that is the source, an image is as much source in my opinion even if you convert it to another format.

Auto-complete automation is nice, because we something is added it can applied to all shells. And if a shell is not
supported support for it can be added. I my optinion, it is perferable to only use one file for all shells, it makes it simpler for developers to support all shells in there programs, as long as nothing superfancy is needed. So I think it should be used for the sake of having tools that lets developers save time.

@jaseg
Copy link
Author

jaseg commented Apr 6, 2013

pixelterm supports arbitrary-minimum-width balloons, you just draw a 50% transparent red, two pixel high bar as wide as the minimum width of the balloon. I just had a look at the balloon rendering in the new backend.py, indeed I did not implement minimum height or these justification options. I pretty much reverse-engineered the .pony format from looking at .pony files. As far as I egrep'ed, justification or minimum width from .pony files is not currently used. In my fork I implemented justification of text within balloons, but I made a command-line switch for it since it is situation-dependant whether you want your text to be centered (like lines of a poem) or left-aligned (like almost everything else). In case there is only a single line in the balloon, my code already centers that line because I think that looks nice.

Minimum balloon height support would be easy to add (especially in the png format), but currently I do not see the merit of that.

util-say however does support everything

Perhaps everything is a bit too much ;)

The problem of .pony files vs. PNGs is imo that .pngs support the full color space while .pony files of course are limited to xTerm's 256-color subset. Since the .pony->png conversion is lossless you can of course work in either format, only I think PNG is somehow more user-friendly (almost everybody has a drawing program and I personally do not like manually messing around with color escape sequences except when I'm trying to make ponies blink).

I agree that auto-auto-completion is nice, but it's really just 30 lines (less than this comment) anyway and there are not that many shells on this planet ;)

@maandree
Copy link
Collaborator

maandree commented Apr 6, 2013

The justification is how the balloon is justificed in the space it can be draw in,
and I think it makes the ponies look rather nice, but currently no pony uses it.

"Perhaps everything is a bit too much ;)", naturally, but why not, you can make really nice ponies.
Ponysay had everything it needed a while ago, now it getting more stuff that could be nice to have.
The balloon stuff is just some of it, but other backend features that are not used right now will be
used in version 4.0 or something like that, or if somepony just chooses to, for example colouring
the pony could be used on ASCII ponies.

Actually, the TTY pony files supports full colour, except translucent colours, and you can make
Xterm ponies that does, but Xterm would not be able to print that in full colour and will print them
very slow. Full colour space is also unnecceary as it is not used (but it can be for TTY, and some
future terminal.)

We could store new PNG files, but not just convert the .pony files to .png, to store the ponies
in full colour, however, the colour in the PNG files are adjusted to make the colours look could
when approximated into xterm-256color colours so I did see a point in keeping those colours.
But we should keep the .pony files, if you want to do a major it, it is not too hard to convert to
PNG first and convert bak, we could even make an small sript or ponysay-tool command to
automate everything (stash metadata in a seperate file, convert to PNG, fire up GIMP, convert
back to .pony, and reapply metadata). But you will probably not that often do that.
However we could remove the ttyponies and build the on compilation (when uname -s = Linux).

@maandree
Copy link
Collaborator

maandree commented Apr 6, 2013

By the way, you can find a full description of the .pony format, on page 34(29 on the paper) (§§12.1, 12.2).
And if you want the balloon encoding in PNG files for pixelterm to be interoperable with util-say,
you can read maandree/util-say#14, well as the lower part of this comment.
The alpha colour is choosen so a it is not read as encoded by mistake when an image has dithered
transparance (as the hair for Luna in Browser Ponies has.)
Personally I think that is it peferable with interoperability, but I we no when to 50% to have special
meaning, but I can see the charm in it for user friendlyness.

argb(100, 255, 0, 0) = $$
argb(100, 0, 0, 255) = $/$
argb(100, 255, 0, 255) = $X$ (not yet support in ponysay)
A balloon can also be encoded in one pixel pair using alpha=99 (read the Image.java code for detail on that.)

@jaseg
Copy link
Author

jaseg commented Apr 6, 2013

On 04/06/2013 10:21 PM, Mattias Andrée wrote:

By the way, you can find a full description of the .pony format, on page 34 (§§12.1, 12.2).
And if you want the balloon encoding in PNG files for pixelterm to be interoperable with util-say,
you can read maandree/util-say#14, well as the lower part of this comment.
The alpha colour is choosen so a it is not read as encoded by mistake when an image has dithered
transparance (as the hair for Luna in Browser Ponies has.)
I do not think I care too much for the case when someone tries to import pngs containing semi-transparent pixels. I think whoever uses that script can manage to fire up gimp before using an image with pixelterm since one would probably do that anyway.

Also, I am not a big fan of using stego on pngs as a user interface. I think it is a feature that these pngs are editable with any drawing program except for mspaint (that does not handle transparency).

Personally I think that is it peferable with interoperability, but I we no when to 50% to have special
meaning, but I can see the charm in it for user friendlyness.
Sorry, could you perhaps rephrase this? I do not understand what you mean.

argb(100, 255, 0, 0) = $$
argb(100, 0, 0, 255) = $/$
argb(100, 255, 0, 255) = $X$ (not yet support in ponysay)
A balloon can also be encoded in one pixel pair using alpha=99 (read the Image.pony for detail on that.)
To be honest, I did not even that part of your code.

@maandree
Copy link
Collaborator

maandree commented Apr 7, 2013

Hopefully nopony will go through the trouble of installing mspaint to draw images with transparency for a non-Windows program (but it does run on cygwin, as virtually everything except valgrind does).

Whoops, something happend in the middle of that sentence.
I meant that I do not want to use alpha=50% im my images because of the problem with dithered transparency,
but I find it to be better for user friendlyness, so if you perfer that over supporting rare cases when importing
ponies from other sources, you should not use the same encoding as I do.

I understand that you did not use my code, but the alpha=99 parts was to long to write and is not too important as it is inferior in all but really special cases.

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.

None yet

4 participants