Fix for Issue9 : Error while adding a new step from HTML UI, A small fix for #7

[ccpandhare]

I have fixed the Issue for now by declaring a global variable document.sequencer_image.

[ccpandhare]

I have also fixed a small variable name issue for FireFox

[jywarren]

Hmm, though -- this is basically using a global, no? Is there a more local way to do that, and is it specific to the demos?

[ccpandhare]

There is a local way to do it.
By creating options.initial_image instead of document.sequencer_image.
Is that okay?

[jywarren]

Local to the module, right? Yes, that sounds good!

…

On Mar 12, 2017 3:51 PM, "Chinmay Pandhare (CCD)" ***@***.***> wrote: There is a local way to do it. By creating options.initial_image instead of document.sequencer_image. Is that okay? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#12 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ73CQ30HgZBic3IYP1xec9nvb7P-ks5rlEyvgaJpZM4MaeZ3> .

[ccpandhare]

Yep. I've done that now.

[jywarren]

Oh, hmm, but sequencer.run(sequencer.options.initial_image); seems to imply initial_image is local to the sequencer, not the module. What about if each module preserves an internal inputImage and outputImage, and when we do run(), we default to the inputImage of the first module?

[jywarren]

Oh, i see -- this line shows that we store initial_image for both module and sequencer. Do you think that's necessary, as opposed to just for the modules?

https://github.com/publiclab/image-sequencer/pull/12/files#diff-f887b503b80e10727ac6cee436178fc5R56

Thank you so much for all this, it's pretty complex, and I appreciate your careful thought and contribution!

[ccpandhare]

@jywarren I'm afraid you're getting me wrong. Let me try to eplain.

Issue

The issue was that sequencer.run() needed an image parameter, unlike what was the assumption earlier. This is because sequencer.run() takes in an image parameter and runs it through sequencer.steps[]. Once we have an image, We do have draw functions for all steps. But not a single image which we can input as sequencer.steps[i].draw(image)

/src/ImageSequencer.js defines run() as :

  function run(image) {
    if (image) steps[1].draw(image);
    else steps[0].draw();
  }

But the function steps[0].draw needs an image parameter as per its definition in /src/modules/ImageSelect.js (steps[0] is 'image-select' as per a browser implementation is concerned.):

  function draw(image) {
    el.html(image);
    if (options.output) options.output(image);
  }

Solution

So basically, we needed a starting point (image) for drawing.

Hence, I decided to introduce a variable local to the ImageSequencer instance. instance.options.initial_image (in /index.html we have taken the instance name to be 'sequencer', hence sequencer.options.initial_image).
This variable stores the initial image of a Sequence. The one which gets loaded into the first module. Say I have a.jpg. Then I load it into ImageSequencer and apply some modules on it. Throughout this process, instance.options.initial_image is equal to something like <img src="/path/to/a.jpg">.

initial_image is stored for the sequencer. Since we are loading images via a special module 'ImageSelect.js' (Something, I believe, should change.) The variable gets defined there. Hence you might have thought that initial_image is stored per module, which is not the case. It gets defined in 'ImageSelect.js' and is defined for the Sequencer instance.

Some Notes / Concerns

Sorry, I should have mentioned all this in the Pull Request.

And as far as the inputImage/outputImage implementation per module is concerned, I think we should do that too, sooner or later, in the interest of non-repetitive calculations as we were discussing in Issue #14 ('Unnecessarily Slow Algorithm')

What do you think about this? Should we stick to something like instance.options.initial_image for now, or immediately change it to inputImage/outputImage defined per module?

[jywarren]

I think we're struggling with multiple issues here, so perhaps we should just implement a short term fix and treat them separately. But I guess I was thinking that any module should have inputImage and outputImage, and that if they did, a sequencer-level locally stored image could be unnecessary.

There's nothing API dependent about the sequencer also tracking its initial image, so I'm OK with it on those grounds, but I guess I have a mental model of the sequencer that's not state-ful. Now that I think about it, the modules are stateful because they can be configured. They might also be stateful in that they (if we do this) remember the last input and output images that passed through them. I tend to think as little state as possible is ideal. But the sequencer too has state - its list of steps.

What do you think of all this? We also need not make a permanent decision on it. We may find later that we run into stronger reasons to change these decisions. For the time being, I'm fine merging this, though I think we should standardize to camelCase for initialImage just so we don't use two conventions.

[ccpandhare]

Yes, As we were discussing earlier, we should create the inputImage and outputImage for every step.

So, what is the model of Image-Sequencer which you have in mind? How do you wish to implement it?
Since we are early into development phase, I feel we can change stuff at this stage. After a while, that task would become increasingly cumbersome, isn't it?

As far As the CamelCasing is concerned, I'll do that.

[jywarren]

I feel we can change stuff at this stage. After a while, that task would become increasingly cumbersome, isn't it?

I agree completely, and that's why I'm being extra cautious at this stage. I appreciate your patience :-)

Let me look at this when i have a moment today and may just merge it, since I feel my #1 changes are holding things up...

[ccpandhare]

So should we wait until we find a solution for #1 and then merge this?

[jywarren]

Do we need to set it to "" right here? Can't we just start using it, and it will be default undefined (so options.initialImage !== undefined) until we fill it? Not a big deal but want to be pretty tidy in this code as it's core functionality. Thanks!

[ccpandhare]

Okay, I'll do that.
I'll remove that line.

[jywarren]

Do you think you could rebase and recompile? I think we can pull this in for now and resolve #1 anyhow, since this will make the current functionality work better in Firefox, right?

[ccpandhare]

Yes, It will make things work in Firefox, but that's a very small bugfix, anyways.

Okay I'll recompile.

[ccpandhare]

@jywarren There's a rebase issue with this branch. Please merge PR #16 instead.