Proposal: restructure to sequencer.steps instead of sequencer.images.image1.steps

[jywarren]

Breaking out from #250:

Right now, each image in the sequencer has its own steps, but are these all identical? I feel they ought to be -- according to our model, it should be a consistent sequence that you then pass images through, not a collection of images which you assign steps to.

Does this make sense?

[jywarren]

@publiclab/is-reviewers what do you think about this? It's a bit laborious to always be going into:

sequencer.images.image1.steps and also even sequencer.images.image1.steps[1].options.name just to get the name of a step.

Two ways forward might be:

1. (simpler) just create a sequencer.getSteps() method to shortcut
2. store steps in sequencer not in the image, so sequencer.steps, because if we want different sequencers for different images, we should just create different instantiations
3. not allow multiple images in the sequencer anyways - we could always make a new sequencer and pass in the steps from the first. One image per sequencer. (what do you think?)

[jywarren]

This would likely be a breaking change. I want to discuss now because it seems important, we have a better idea of our use cases now, and we'd want to make this change earlier rather than later before more end-uses develop as in #707

[Divy123]

I was wondering about it and this could be a great thing as the things go too complicated sometimes due to the lack of getSteps() function.
Also I would like to suggest that the documentation should be restructured to provide a deep insight not only on the usage part but also on the development thing so that the new fellows can find things much easier.
I would like to suggest Read the Docs for implementing a nice documentation.
Both usage and development docs can be maintained separately that can make contributions much easier.
What do you thihnk @jywarren ?

[jywarren]

Great idea on better docs and in general we should open a new issue on this and link into the tests more and more as an example of usage. Let's start with `getSteps()` which is a reasonable place to start that does not break any API. We can test it too. I'd also like to hear from others - thanks!

…

On Fri, Feb 8, 2019 at 1:49 PM Slytherin ***@***.***> wrote: I was wondering about it and this could be a great thing as the things go too complicated sometimes due to the lack of getSteps() function. Also I would like to suggest that the documentation should be restructured to provide a deep insight not only on the usage part but also on the development thing so that the new fellows can find things much easier. I would like to suggest Read the Docs for implementing a nice documentation. Both usage and development docs can be maintained separately that can make contributions much easier. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#251 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ669znDcKT8wzbcLfbI6ObvXIM4-ks5vLcaXgaJpZM4T1PXI> .

[Divy123]

Sure @jywarren . I am opening a new issue and I would love to restructure the code to implement getSteps() .

[tech4GT]

Go ahead Divy, you can expect any help you need from me :)

[Divy123]

Thanks @tech4GT .

[jywarren]

@Divy123 great!

This may also enable us to simplify the following methods:

function removeStep(image, index) {
function removeSteps(image, index) {

because we'd no longer need to specify an image. We could do that as our next step after getSteps().

[Divy123]

@jywarren I think the major work on the issue is still pending so please reopen it .

[jywarren]

Shall we try to complete this before releasing 3.0.0 what do you think @tech4GT, then we can release it as a breaking change together instead of going to 4.0.0?

[Divy123]

I shall try to complete it as soon as possible.
Already working on it.

[tech4GT]

I don't think this is going to be a breaking change though...

[jywarren]

I think it is, though - for example, InsertStep(ref, image, index, name, o) will change, and many others, no? I think it's OK to wait a bit on the new release and roll these both into one. This will be great!!!!

[Divy123]

@jywarren @tech4GT I was wondering about two possible solutions for handling multiple images now.
Planning to create a new InitSequencer.js that may:

1. return a global sequencer object to the user with the following structure:

   {
    "image1":sequencer1,
    "image2":sequencer2,
   } 

The usage can be like this:

  var global_sequencer = InitSequencer(2); //supposing we want to have 2 images
        global_sequencer.image1.loadImage("src",optional_callback);
        global_sequencer.image1.addSteps('blur');

or we can have like this:

  var global_sequencer = InitSequencer(['src1', 'src2'], optional_steps_array, optional_options);

and we can perform addSteps, removeSteps and so on.

This will create multiple sequencer objects inside InitSequnecer.js which can have the steps and perform loadImage, addSteps accordingly.

2. return an array of this form:

     [sequencer1, sequencer2]

The usage can be like this:

  var global_sequencer = InitSequencer(2); //supposing we want to have 2 images
        global_sequencer[0].loadImage("src",optional_callback);
        global_sequencer[0].addSteps('blur');

or we can have like this:

  var global_sequencer = InitSequencer(['src1', 'src2'], optional_steps_array, optional_options);

and we can perform addSteps, removeSteps and so on.

This will create multiple sequencer objects inside InitSequnecer.js which can have the steps and perform loadImage, addSteps accordingly.

Your thoughts on this please.

PS: I am working on this for a while and am almost done with restructuring loadImages and addSteps function. Will open a PR as soon as all the sequnecer functions are resturctured.
Thanks

[Divy123]

@jywarren @tech4GT

[jywarren]

Hi! Well, what do you think of not trying to store more than one image in the sequencer -- make it so that if you want, you can:

1. pass a second image in, and it comes out the other end, but nothing is preserved of the first image (kind of stateless)
2. make a second sequencer and feed it the same sequence as the first one, so you can use 2 at once

These seem to cover most cases I can think of, and both still allow us to remove the storage of multiple images from Image Sequencer at all - which really simplifies the codebase for the better. What do you think?

[Divy123]

Hello! So what do you suggest should I design a function in ImageSequencer.js file which can reproduce the steps which were defined on the first one and run the steps on second or the loadSteps() can be modified to perform this task?

Also in this

2 make a second sequencer and feed it the same sequence as the first one, so you can use 2 at once

do you mean to creating a replica of the first sequencer that has same sequence of steps that can be used on another image? Can you please elaborate a bit more on this?
Thanks!

[jywarren]

So, I mean not dealing with more than one image at a time in a Sequencer -- we'd create one:

s1 = new ImageSequencer(...);
s1.addImage(...);
s1.addSteps(steps);
s1.run();

s2 = new ImageSequencer(...);
s2.addImage(...);
s2.addSteps(steps);
s2.run();

Or if we wanted to,

s1 = new ImageSequencer(...);
s1.addImage(url1);
s1.addSteps(steps);
s1.run();
s1.addImage(url2);
s1.run(); // it wouldn't save url1, it'd just flush it out as it ran url2 through

Make sense? So then we can mostly just eliminate code that deals with storing separate images inside, as we would not assume Sequencer would "know" or "care" about more than one image at a time. I think this'd help simplify the codebase a lot!

[Divy123]

For sure!
Thanks!

[jywarren]

thank you! This is gonna be awesome :-)

…

On Fri, Mar 1, 2019 at 5:07 PM Slytherin ***@***.***> wrote: For sure! Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#251 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJyFB9ZKERx_SQcw4oJn8oVUFbwucks5vSaR_gaJpZM4T1PXI> .

[Divy123]

@jywarren please review the PR .

[harshkhandeparkar]

@Divy123 since you are already working on this, can I unpin it as I want to pin #842?

[Divy123]

Sure @harshkhandeparkar.

[harshkhandeparkar]

Thanks! 😊