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

Reduced randomness #20

Merged
merged 4 commits into from
Feb 12, 2018
Merged

Conversation

parkercoates
Copy link
Contributor

A small set of changes, including two new flags to control how pipes are displayed. Could be broken up into smaller pull requests if desired.

@Foggalong
Copy link
Contributor

This pull request is confusing me a bit and yeah should probably be split up into smaller pulls. It also seems to introduce a few bugs which is less than ideal.

  • I really like the idea of being able to specify a colour with the -c flag and I can't believe it's not something that's been thought of before. It's implementation seems cause some problems though:
    • Breaks the -C flag which used to give all white pipes and now just uses the last colour used in -c
    • Does weird things if you use numbers ≥8
    • Does weirder things if you put something that isn't a valid number
    • If multiple -c are used but one is invalid, all pipes come out green
    • It isn't documented in the README
  • I'm not entirely sure what the -E tag does.
    • I get your commit description but it doesn't really apply to pipes.sh since it only draws one pipe at a time. Because of that for me running with -E just does the same as -c 2.
    • It isn't documented in the README or under --help
  • If I'm understanding 2ffa69a correctly then the idea is that if I use -t 2 -t 3 then the pipes will alternate between 2 & 3 every time a new one is drawn (and the same theory for -c 2 -c 3). If that's the case then this isn't working, sometimes a style 2 will be followed by another style 2 and not by a style 3.

Anyways, thanks for the pull request! I definitely like some of the ideas in here :)

@parkercoates
Copy link
Contributor Author

Thanks for the feedback. I didn't modify the README at all, because I didn't know if there was interest in the changes, but I should have mentioned that.

  • The -C option seem broken even without this change set, for me. With it set, pipes.sh doesn't output any colour escape codes, but it doesn't actually reset anything either. So the pipes are drawn with the last drawn colour, until the -r limit is hit at which point they are drawn colourless. Adding a call to tput reset into the script startup code seems to fix this. See -C uses last pipe colour from previous invocation #22.

  • I don't see how -c could be causing issues with values above 7, since the argument handling code does a range check and ignores any values outside that range. But good catch on passing in random text to it. The average string evaluates to 0 in a numeric context, which the range check was letting through. I will fix that.

  • -E and the changes to -t only really make sense with -p set to something greater than 1. You are correct, with -p1 it doesn't really do anything. Some examples would probably help:

    • pipes.sh -E -p5: 5 pipes, each a different, permanent colour
    • pipes.sh -E -p2 -c2 -c3: one permanently green pipe and one permanently yellow pipe
    • pipes.sh -E -p2 -c2 -c3 -t2: one permanently green, curvy pipe and one permanently yellow, curvy pipe
    • pipes.sh -E -p2 -c2 -t2 -c3 -t6: one permanently green, curvy pipe and one permanently yellow, dotted pipe
    • pipes.sh -E -p4 -c2 -t2 -c3 -t6: two permanently green, curvy pipes and two permanently yellow, dotted pipes
    • pipes.sh -E -p6 -c2 -c3 -t1 -t6 -t8: 1 green/curvy, 1 yellow/dotted, 1 green/railroad, 1 yellow/curvy, 1 green/dotted, 1 yellow/railroad

    Maybe this is a sign that the -h text for this feature could be clearer. Suggestions are welcome.

  • No, the intent of 2ffa69a isn't for a single pipe to cycle through the types, but instead for the initial assignment of pipe types to be deterministic, as is used in the examples about.

@parkercoates parkercoates force-pushed the reduced-randomness branch 2 times, most recently from 7542784 to cae7e2e Compare September 16, 2015 01:33
@parkercoates
Copy link
Contributor Author

  • A few small fixes made.
  • README documentation added.

@livibetter
Copy link
Contributor

LGTM (didn't test) for almost all except the cfa757e (the second commit), I think it should be an option like -E in 1cf6c7d, I consider the random starting color is a feature and I do not want deterministic.


I think pipes.sh has gone a long way, although I am not entirely sure if this:

pipes.sh -E -p6 -c2 -c3 -t1 -t6 -t8

is what I want to see, but it's become more complex for sure. (and overly complicated for me)

Copy link
Contributor

@livibetter livibetter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a nitpick on the randomness which isn't uniform.

pipes.sh Outdated
@@ -117,7 +122,7 @@ while REPLY=; read -t 0.0$((1000/f)) -n 1 2>/dev/null; [[ -z $REPLY ]] ; do
((${l[i]}%2)) && ((x[i]+=-${l[i]}+2,1)) || ((y[i]+=${l[i]}-1))

# Loop on edges (change color on loop):
((!NOEDGECHANGE&&(${x[i]}>=w||${x[i]}<0||${y[i]}>=h||${y[i]}<0))) && ((c[i]=RANDOM%8, v[i]=V[${#V[@]}*RANDOM/M]))
((!NOEDGECHANGE&&(${x[i]}>=w||${x[i]}<0||${y[i]}>=h||${y[i]}<0))) && ((c[i]=${C[$((RANDOM%${#C[@]}))]}, v[i]=V[${#V[@]}*RANDOM/M]))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The outcome isn't uniform, see my post.

It should be like # * RANDOM / M, but if you don't care, well, it's just a color.

@parkercoates
Copy link
Contributor Author

I consider the random starting color is a feature and I do not want deterministic.

The starting colour isn't actually random at the moment. It's always red. I agree that having the starting colour be random would be nicer.

I'm now rethinking this patch set, and I think I can do all this in a simpler more understandable way. Hopefully I can have an update up soon.

@livibetter
Copy link
Contributor

@parkercoates My comment was from the future! Just kidding, my memory was messed up with pipesX.sh.

I love simplicity, by the way, I think setting CN=${#C[@]} would increase readability, shorten the lines, might even improve performance. (leave ${#V[@]} for another commit, I found the script has a few mismatch coding styles, my bad, actually)

This behaves pretty much identically to -t. It sets up a pool of
available colours, which are assigned to pipes in order and which is
chosen from when randomising.
Previously, initial pipe colours were handed out in a predetermined
order. Conversely, initial pipe types were assigned completely at
random making no attempt to avoid duplicates.

The new behaviour is consistent for both. The colour and type of the
first pipe is completely random, but the colour and type values are
then cycled through in an orderly fashion. This ensures a random, but
evenly spread start, which I think looks pretty nice.
Turning off randomisation allows pipes to retain their "personality"
after crossing the edge. Personally, I like this because I can watch,
say the different colour pipes fight it out for control of the screen.

This also disables the randomisation of the initial pipe colour and
type.
@parkercoates
Copy link
Contributor Author

Okay, so the changes aren't as substantial as I thought they'd be. The order of the commits makes more sense now, though. I tried to order them from least to most controversial. ;)

If there's a feeling that the -K option introduced in 40033f4 is just too complicated or confusing, I completely understand.

pipes.sh Outdated
@@ -114,12 +114,12 @@ while REPLY=; read -t 0.0$((1000/f)) -n 1 2>/dev/null; [[ -z $REPLY ]] ; do
((${l[i]}%2)) && ((x[i]+=-${l[i]}+2,1)) || ((y[i]+=${l[i]}-1))

# Loop on edges (change color on loop):
((${x[i]}>=w||${x[i]}<0||${y[i]}>=h||${y[i]}<0)) && ((c[i]=RANDOM%8, v[i]=V[${#V[@]}*RANDOM/M]))
((${x[i]}>=w||${x[i]}<0||${y[i]}>=h||${y[i]}<0)) && ((c[i]=8*RANDOM/M, v[i]=V[${#V[@]}*RANDOM/M]))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is unnecessary, for divisor n == 2^i, that is 32768 % n == 0, the outcome is evenly distributed.

(I was hoping this commit would come separate (from this PR), after coding style fix, and or within refactoring, but it's fine)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am making this as a change request.

Copy link
Contributor

@livibetter livibetter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert that line.

pipes.sh Outdated
@@ -114,12 +114,12 @@ while REPLY=; read -t 0.0$((1000/f)) -n 1 2>/dev/null; [[ -z $REPLY ]] ; do
((${l[i]}%2)) && ((x[i]+=-${l[i]}+2,1)) || ((y[i]+=${l[i]}-1))

# Loop on edges (change color on loop):
((${x[i]}>=w||${x[i]}<0||${y[i]}>=h||${y[i]}<0)) && ((c[i]=RANDOM%8, v[i]=V[${#V[@]}*RANDOM/M]))
((${x[i]}>=w||${x[i]}<0||${y[i]}>=h||${y[i]}<0)) && ((c[i]=8*RANDOM/M, v[i]=V[${#V[@]}*RANDOM/M]))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am making this as a change request.

@livibetter
Copy link
Contributor

@parkercoates I will merge once you make the change.

Off-topic: If you have time, I think you should apply for pipes.sh new maintainer position at pipeseroni/pipeseroni.github.io#3, currently, we have no one to actively maintain pipes.sh.

@livibetter livibetter dismissed their stale review February 12, 2018 07:09

Confused by the outdated.

@livibetter livibetter merged commit 1cef29e into pipeseroni:master Feb 12, 2018
@livibetter
Copy link
Contributor

@parkercoates I've merged this, if you have more to contribute in the future, please keep PR/commit atomic.

@parkercoates
Copy link
Contributor Author

parkercoates commented Feb 12, 2018

@livibetter, Thanks for the merge! I'm normally a stickler for atomic review requests, so I doubt this will be an issue in the future.

And speaking of the future, thank you for the invitation to become a maintainer, but I think I lack both the qualifications and the motivation. I know very little about all the terminal control stuff and really only wanted to make a handful of changes so that I could watch two differently coloured pipes battling it out to dominate the screen.

image

@parkercoates parkercoates deleted the reduced-randomness branch February 12, 2018 17:26
@livibetter
Copy link
Contributor

If you are really lack of them, I'd not have asked. Anyway, they are cool pipes!

@livibetter
Copy link
Contributor

@parkercoates I have stolen your idea in that screenshot to make a video.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants