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

Framebased Delay #284

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Framebased Delay #284

wants to merge 2 commits into from

Conversation

timothymclure
Copy link

Framebased Delay added for Addonpack

Framebased Delay added for Addonpack
Copy link
Member

@joreg joreg left a comment

Choose a reason for hiding this comment

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

some cosmetics

namespace VVVV.Nodes
{
#region PluginInfo
[PluginInfo(Name = "Delay", Category = "Animation", Version ="Framebased", Help = "Framebased delay with adjustable frame count.", Tags = "Whiplash/joshuavh")]
Copy link
Member

Choose a reason for hiding this comment

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

"Whiplash/joshuavh" doesn't sound like a proper tag..please elaborate

public ISpread<double> FInput;

[Input("Skip", IsBang = false)]
public ISpread<bool> skip;
Copy link
Member

Choose a reason for hiding this comment

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

all fields should start with a capital F, like FSkip, FReset, FDelayFrames, FListOfQueues

[Input("Input", DefaultValue = 1.0)]
public ISpread<double> FInput;

[Input("Skip", IsBang = false)]
Copy link
Member

Choose a reason for hiding this comment

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

IsBang=false is not needed. bools are toggles by default.


public void Evaluate(int SpreadMax)
{

Copy link
Member

Choose a reason for hiding this comment

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

please remove superfluous linebreaks.

////////////////////////////////////////////////////////////////////////////


public void setDelayQueue(int index, int SMax, int diff){
Copy link
Member

Choose a reason for hiding this comment

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

Operations start with a capital letter

minor changes
@timothymclure
Copy link
Author

hey joreg, we updated the forked repo with the requested cosmetics

Copy link
Member

@joreg joreg left a comment

Choose a reason for hiding this comment

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

sorry for my late answer and thanks for your changes so far. i hope you don't understand this to be to annoy you. i'm just hoping that those changes would contribute to a more concise code-base in the future.

public class DelayFramebased : IPluginEvaluate
{
#region fields & pins
[Input("Input", DefaultValue = 1.0)]
Copy link
Member

Choose a reason for hiding this comment

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

any reason for the default being 1.0 here instead of 0? if not, then simply remove the default value

[Input("Reset")]
public ISpread<bool> FReset;

[Input("Delay", DefaultValue = 1.0, MinValue = 0)]
Copy link
Member

Choose a reason for hiding this comment

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

in the Delay (Animation) node this parameter is called "Time". for consistency reasons i'd suggest to name this pin "Frames"

[Input("Skip")]
public ISpread<bool> FSkip;

[Input("Reset")]
Copy link
Member

Choose a reason for hiding this comment

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

a "Reset" pin would typically be the last pin on a node. just checked with Delay (Animation) where it is not, but that should not stop you from doing it right here. also the original node has a "Reset Value" input, wouldn't that also be useful here?

[Input("Input", DefaultValue = 1.0)]
public ISpread<double> FInput;

[Input("Skip")]
Copy link
Member

Choose a reason for hiding this comment

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

in Delay (Animation) the input is called "Insert". is that the same only in reverse? so not skip = insert? if so i'd suggest to name it "Insert" here also


[Input("Delay", DefaultValue = 1.0, MinValue = 0)]
public ISpread<int> FDelayFrames;

Copy link
Member

Choose a reason for hiding this comment

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

also i suggest to adapt the order of pins to the original node. that would be: Input, Frames, Insert, Reset (Reset Value)

public ILogger FLogger;
#endregion fields & pins

List<Queue<double>> listOfQueue = new List<Queue<double>>();
Copy link
Member

Choose a reason for hiding this comment

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

listOfQueue is also a field just like the inputs. therefore it would also be called: FListOfQueue

FOutput.SliceCount = SpreadMax;


if(SpreadMax != listOfQueue.Count){
Copy link
Member

Choose a reason for hiding this comment

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

in most c# coding guidlines you'll learn to place opening brackets on a new line for good style and better readability.


int difference = SpreadMax - listOfQueue.Count;

if(difference < 0){
Copy link
Member

Choose a reason for hiding this comment

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

while i'm at it: it is good practice to put a space between if and the opening bracket. same with "for ("

@joreg
Copy link
Member

joreg commented Feb 13, 2017

@timothymclure are you still interested in getting this into the addonpack? if so please answer my review..

@joreg joreg closed this Feb 13, 2017
@joreg joreg reopened this Feb 13, 2017
@timothymclure
Copy link
Author

@joreg -- of course. I don´t mean to be disrespectful, but the Plugin works and I think creative decisions and how to format the code should be up to the coder. Made this small plugin for ourselves and wanted to share it in case someone else might find it helpful.

@joreg
Copy link
Member

joreg commented Feb 15, 2017

hei @timothymclure thanks for getting back on this.
my review indeed comprises of different things:

  • node signature, ie naming, order and defaults of pins
  • coding style

i want to explain why each is important and not a creative choice:

  • node signature: in vvvv if you doubleclick a Delay (Animation) node to replace it with your Delay (Animation Framebased) all the pins that have the same name will keep their connections. this is a very convenient feature which you break with your creative decision. this is only regarding the pin-names. pin-order and defaults make sense to keep the same as well because if you already learned how to use the timebased delay-node you should not have to learn something new about a node that pretends (through its naming) to be quite the same but only a framebased version of it.
  • coding style: if you're coding for yourself you can of course choose any style you prefer. the addonpack is a huge codebase that many people contribute to over a long period of time. the more creative individuals get with their contributions the less maintainable the code will get over time.

having said that, i hope you understand why both reasons are important. we haven't been very strict about coding-style in the past so please consider my remarks regarding coding-style more as a recommendation. regarding the node-signature though i'll have to enforce my remarks for the sanity of all vvvv users.

so please either fix those or of course feel free to close this pullrequest if you prefer not to. you'll then still have the option to share your plugin as a contribution: https://vvvv.org/contributions

@timothymclure
Copy link
Author

Hey @joreg, point one accepted, but think it should be communicated way better beforehand. This still is not a plugin especially made for the addonpack, so there was no need to follow guidelines in the first place.
As far as coding for yourself or for this Pack, I really think you should be able to code the way you are comfortable with and still have the opportunity to share it in the Pack, as long as the code is quite readable. But this of course is up to you to decide and I accept that. So I will look back into it, but it may take a while.

@joreg
Copy link
Member

joreg commented Feb 22, 2017

thanks for your understanding. looking fwd to your update anytime...

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.

2 participants