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

meta-modules in node #203

Closed
wants to merge 2 commits into from
Closed

Conversation

tech4GT
Copy link
Member

@tech4GT tech4GT commented Mar 14, 2018

@jywarren The approach i have taken for this one is having a meta-modules.json file and when user gives the input the meta-modules are expanded in program.step without destroying any ordering then normal operation occurs
I have defined one named example which equals green-channel invert
Let's say program.step initially has value [brightness, example, blur] then expandMetaModules is called, then program.step will become [brightness,green-channel,invert,blur]
Is this a good enough approach for this?, please review
Signed-off-by: tech4GT [email protected]

Signed-off-by: tech4GT <[email protected]>
@tech4GT
Copy link
Member Author

tech4GT commented Mar 14, 2018

#203

@tech4GT
Copy link
Member Author

tech4GT commented Mar 14, 2018

@jywarren also a perk to having a file to store these modules is that if one user wishes to save or his/her configuration( like we do with IDEs like webstorm and VSCode ) they can simply share this file. For eg VScode handles configuration using a config.json file as well

Copy link
Member

@jywarren jywarren left a comment

Choose a reason for hiding this comment

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

Ok! Left a comment, also how do we specify preset settings for each module?

Thanks!!! Exciting!

{
"name": "example",
"description": "An example meta module",
"steps":[
Copy link
Member

Choose a reason for hiding this comment

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

So while this is cool, I wonder if we can specify meta modules in a serialized string that is readable as part of a url request. Maybe we need both options so like this OR ?steps=invert[settings...],green-channel, threshold [50] just for example.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jywarren you are talking about the browser, right? Actually we have a separate issue to do this for the browser and it's a part of summer proposal as well, if you say i can start working on this right now?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jywarren this also looks doable, as part of summer project we will be having support for saving a sequence with settings in the url itself and as far as connecting it with this we can have a method that outputs the readable part of the url from the meta-module

function genarateURL(module){
/* generate a string based on default values and sequence of steps8?
return the string
}

Copy link
Member

Choose a reason for hiding this comment

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

Actually, 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.

Great, though -- so for this PR, so as not to create an incompatibility, we should either think about a subset of the full syntax that will remain compatible after the settings-in-a-string feature, so while here we might just enable settings steps in JSON, we should expect to be able to detect a string submission and switch into a different parsing mode which can read in the settings. I think that means we are OK here - but maybe we should leave an inline comment saying we'll soon accept compact string format like ?steps=invert(settings...),green-channel,threshold(50)

Perhaps using parentheses will make it more like a function call... but we don't have to make a final decision just yet.

Copy link
Member

Choose a reason for hiding this comment

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

See #127 for current planning on compact settings options in URLs

@tech4GT
Copy link
Member Author

tech4GT commented Mar 24, 2018

@jywarren presets are given just as we give for a sequence of steps since all this is doing is making an alias for a sequence.

@tech4GT
Copy link
Member Author

tech4GT commented Mar 24, 2018

@jywarren I was thinking 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?

@jywarren
Copy link
Member

^ great idea -- adding to #127!!

@jywarren
Copy link
Member

Looking great; let's add an inline comment referencing string-based steps and how we'll parse them in #127, and move forward!

@jywarren
Copy link
Member

Also lets' implement a meta-module in a follow-up PR so that we can link to that commit as an example in the README, showing how simple it is!

@tech4GT
Copy link
Member Author

tech4GT commented Mar 31, 2018

@jywarren this sounds great, I'll add the comment, I had some questions though

  • Saving to local storage is the browser counterpart of saving to the json file right?
  • do we want string based steps in node environment as well, what I was thinking was that node and browser meta-modules implementation can be separate, on the node side we store them in the json file as json and on the browser side we store them as parable strings(that will be converted to json when processing) in the local storage, is this right, sorry I wasn't able to understand completely?
  • Also should I add for options in the meta-module.json if a particular meta-module has different default options than it's constituent steps?
  • Also where should i add the inline comment?
    Thanks😁

@jywarren
Copy link
Member

  1. Saving to local storage is the browser counterpart of saving to the json file right?

It's one possible place where sequences could be stored as a string. I'm trying to think about how we make different solutions easy once we standardize a way to store sequences as a string.

  1. do we want string based steps in node environment as well, what I was thinking was that node and browser meta-modules implementation can be separate, on the node side we store them in the json file as json and on the browser side we store them as parable strings(that will be converted to json when processing) in the local storage, is this right, sorry I wasn't able to understand completely?

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.

  1. Also should I add for options in the meta-module.json if a particular meta-module has different default options than it's constituent steps?

that is interesting... i think meta-module should probably have some way that options for the meta-module are passed into the sub-modules. I don't know how that should work, but it should be possible to create a metamodule with preset default options, too. Does that make sense? Say, a metamodule with 3 steps, and the second step's options should be fixed, but we should still accept options for the first and third?

  1. Also where should i add the inline comment?

How about at line 5 of src/config/MetaModules.json ?

@jywarren
Copy link
Member

Sorry this took me a while!!!

@tech4GT
Copy link
Member Author

tech4GT commented Apr 16, 2018

@jywarren no prob, working on this now I'll try to come up with a meta module checklist in this issue itself so we can build up from here✌️
Thanks

@tech4GT
Copy link
Member Author

tech4GT commented Apr 16, 2018

@jywarren should I put these in an improvements for summer issue ? since this overlaps with some stuff there, I was planning on making issue as well as you suggested

@jywarren
Copy link
Member

Yes, i think most of this could be done in follow-up, and we could start with the most basic meta-module implementation and create a checklist for what to do after merging this. How does that sound?

@tech4GT
Copy link
Member Author

tech4GT commented Apr 16, 2018

That sounds great 😁

@jywarren
Copy link
Member

jywarren commented Apr 16, 2018 via email

@tech4GT
Copy link
Member Author

tech4GT commented Apr 20, 2018

@jywarren actually i cannot comment on the .json file😅

@jywarren
Copy link
Member

Let's just open up a new issue exactly describing this needed upgrade, or clean up the one where we spec out the string-based system. OK?

@tech4GT
Copy link
Member Author

tech4GT commented Apr 20, 2018

@jywarren i have kind of made a checklist related to this here #200 once you approve we'll edit the issue and add the ckecklist there, i think it pretty much consolidates all of our ideas😁

@jywarren
Copy link
Member

Awesome, I replied over there. Do you want to merge this in, or tackle more of the system first?

@tech4GT
Copy link
Member Author

tech4GT commented Apr 23, 2018

@jywarren We should wait on this one I guess, I want to work on this a little more before merging this is. Thanks :)

@tech4GT tech4GT added almost-complete PRs that are almost done but little more work. and removed review-needed labels Apr 24, 2018
@tech4GT tech4GT mentioned this pull request Apr 29, 2018
24 tasks
@tech4GT
Copy link
Member Author

tech4GT commented Jul 25, 2018

closing this in favor of #308

@tech4GT tech4GT closed this Jul 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
almost-complete PRs that are almost done but little more work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants