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

ability to set step options via string command syntax: "invert,threshold:{level:50}" #127

Closed
jywarren opened this issue Oct 2, 2017 · 29 comments

Comments

@jywarren
Copy link
Member

jywarren commented Oct 2, 2017

The ability to record both steps and settings in a string could be useful for browser work, for saving sequences in localStorage, in a key/value database on server or client side, and for REST based processing, so I think it's worth considering for all those reasons.

Building on #91, the URL format:

/examples/#steps=green-channel,crop

Could have additional options set with:

/examples/#steps=green-channel,crop[w:50%|h:30%]

Is this an appropriate way to encode steps? crop[w:50%|h:30%] or is there an easier or more straightforward way?

Update

Considering after #203, using parentheses:

?steps=invert,green-channel,threshold(50) or for more complex parameters:

?steps=ndvi('blue'),lens({x:25,y:15})

OR simpler:

?steps=ndvi('blue'),lens(x:25,y:15)

Or even full JSON, although it's less compact:

?steps={ndvi:'blue'},{lens:{x:25,y:15}} --

we could potentially have a "simple" syntax that's optimized for use in URLs, and a "full" syntax that's actually JSON.

@jywarren
Copy link
Member Author

@tech4GT suggests, and I agree!:

maybe we can have an option to store default value of options in the json itself and if the user does not give any values when the module is used it can be used as sort of a fallback case or default values for the meta-module, what say?

@tech4GT
Copy link
Member

tech4GT commented Mar 31, 2018

@jywarren On the browser side once we implement this maybe we can have a save to local storage in the sharing options which will ask the user for a name and the meta-module will be saved with the settings user has currently applied, what say?
PS just thinking how we would make the UI/UX for this so that it feels natural

@jywarren
Copy link
Member Author

jywarren commented Apr 16, 2018

See comment here: #203 (comment)

I think i'm really thinking of a way to do something like sequencer.toString() - a general purpose way to string-encode a sequence, so it could be used in any of these places. We don't even need to DO them all just yet, but if we make good architectural choices at this point we are in a good place for the future.

@jywarren
Copy link
Member Author

So in summary, we should run the URL hash through a new sequencer.import() type command, and implement options-setting as part of the standard "command string" syntax which this import() reads. Changing title to match this!

@jywarren jywarren changed the title Demo feature where step options can be set via url hash ability to set step options via string command syntax: "invert,threshold:{level:50}" May 10, 2018
@jywarren jywarren modified the milestones: Default UI work, Core work May 24, 2018
@jywarren
Copy link
Member Author

Here's the current implementation of importStepsFromUrlHash() which could be generalized into a sequencer.importSteps(string):

// look up needed steps from Url Hash:
function importStepsFromUrlHash() {
var hash = getUrlHashParameter("steps");
if (hash) {
var stepsFromHash = hash.split(",");
stepsFromHash.forEach(function eachStep(step) {
_sequencer.addSteps(step);
});
_sequencer.run();
}
}

Hope that helps!!!

@tech4GT
Copy link
Member

tech4GT commented May 28, 2018

@jywarren thanks😀

@jywarren
Copy link
Member Author

jywarren commented Jun 1, 2018

Once we get to this, we'll want to have a number of tests which show how it can be used. For example, let's have a test which begins with just one module, then use a string like invert,importImage(url:"dir/example.jpg"),ndvi and then confirm that the sequencer has those steps invert, importImage, ndvi - and that importImage has correctly received the URL. We could show tests for each type of parameter too, like string parameters, integers, and even functions.

The tests can act as a kind of guide for how to use the feature -- and when we add documentation (which could also be part of the PR), we can link people to the corresponding tests to demonstrate how they work in real code. It may be worthwhile to break out the import and toString tests into their own test file, maybe called importExport.js or something?

@tech4GT
Copy link
Member

tech4GT commented Jun 1, 2018

@jywarren Got it!! This makes total sense!!

@tech4GT tech4GT mentioned this issue Jun 2, 2018
@jywarren
Copy link
Member Author

jywarren commented Jun 3, 2018

Should we make a pure-json variant or hold off on this for now?

@tech4GT
Copy link
Member

tech4GT commented Jun 3, 2018

@jywarren I think after doing this only one function makes sense sequencer.importStepsfromJSON() since after importing the steps sequencer.toString() can be used to produce the string.

@tech4GT
Copy link
Member

tech4GT commented Jun 3, 2018

this function can basically serve as a wrapper around adding multiple steps defined in an array with their options
This will also complete the architecture we originally decided

@jywarren
Copy link
Member Author

jywarren commented Jun 3, 2018 via email

@tech4GT
Copy link
Member

tech4GT commented Jun 3, 2018

@jywarren I don't understand, do we want to stringify JSON?

@tech4GT
Copy link
Member

tech4GT commented Jun 3, 2018

I mean the current ImportStepsfromString() imports these steps right?

@jywarren
Copy link
Member Author

jywarren commented Jun 3, 2018

oh, wait -- let me see. the API has changed a lot in the past week!

@tech4GT
Copy link
Member

tech4GT commented Jun 3, 2018

@jywarren I'll list it so we have a sequencer.toString() which takes the currents steps and outputs them as a string, then we have a sequencer.importStepsFromStringtoJson(string) which takes a stringified sequence and returns the steps as json.

@jywarren
Copy link
Member Author

jywarren commented Jun 3, 2018

Oh, wait - no that's ok, no need for importStepsFromStringtoJson - i just don't see a sequencer.importStepsfromString() -- going from, say, a URL hash directly to steps. It's exactly the function the excerpted demo code does. The code we have already in the core is only to JSON, not to instantiated steps. I just want one more simple function that gets you all the way in a single function -- how does that sound?

@jywarren
Copy link
Member Author

jywarren commented Jun 3, 2018

Yikes sorry too many function names flying around. Edited the above to mean importStepsFromString()

@tech4GT
Copy link
Member

tech4GT commented Jun 3, 2018

@jywarren that is a great idea!!
what we can do is that we can have a importString function which internally calls the sequencer.toJSON function(basicall we change the current importStepsfromStringtoJson to sequencer.json)

@tech4GT
Copy link
Member

tech4GT commented Jun 3, 2018

@jywarren I got this!! Don't worry I have a pretty good idea of what we need now

@tech4GT
Copy link
Member

tech4GT commented Jun 3, 2018

I 'll do some refactoring and then post all the final function names and what they do here

@jywarren
Copy link
Member Author

jywarren commented Jun 3, 2018

OK! One thing that may help is to move all of the import/export functions into a separate source file called io.js or something? Our main source file ImageSequencer.js is getting a bit long, and we have a whole set of inter-related functions we could put in their own file, right?

@tech4GT
Copy link
Member

tech4GT commented Jun 3, 2018

@jywarren Good idea!! I'll do that too!!

@jywarren
Copy link
Member Author

jywarren commented Jun 3, 2018

But isn't it very late there? No need to do this now, just thinking about next steps!!!

@tech4GT
Copy link
Member

tech4GT commented Jun 3, 2018

working on this now!!

@tech4GT
Copy link
Member

tech4GT commented Jun 3, 2018

@jywarren No problem this will only take a couple of minutes

@tech4GT
Copy link
Member

tech4GT commented Jun 8, 2018

This is done now closing!!

@tech4GT tech4GT closed this as completed Jun 8, 2018
@jywarren
Copy link
Member Author

jywarren commented Jun 8, 2018 via email

@tech4GT
Copy link
Member

tech4GT commented Jun 8, 2018

@jywarren we have it already!!😁

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

No branches or pull requests

2 participants