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

Opaque meta module test - colorbar #762

Closed
wants to merge 3 commits into from
Closed

Conversation

jywarren
Copy link
Member

Re #315 -- an attempt to refactor the colorbar module to be a fully opaque meta module - that is, a module which:

  1. contains other modules
  2. acts like a normal module in every way
  3. contains an independent sequencer in order to pass an input through a sequence and produce an output

If this works (and it's not running yet -- I'm stuck for some reason because draw() is not executing... help!) then we may be able to create a generator for these, which is to say, a way to automate the creation of opaque meta modules given:

  1. a JSON object or string of steps
  2. a function which maps the outer module options to the options objects of the internal steps (or falling back to the defaults if mapping is not provided)
  3. a set of overriding defaults for internal steps (which could be done via the mapping function too)

@publiclab/is-reviewers take a look here, i got inspired and tried it out, but need a bit of help to debug my work!

@jywarren jywarren requested review from tech4GT and a team February 12, 2019 00:17
@jywarren jywarren changed the title Opaque Opaque meta module test - colorbar Feb 12, 2019
@tech4GT
Copy link
Member

tech4GT commented Feb 12, 2019

@jywarren Looking into this right now!

@harshkhandeparkar
Copy link
Member

@jywarren how about adding a new util function. Maybe mini-sequencer? This can also be used to create semi-meta modules(modules which use other modules inside them but not fully). And sequencer.createMetaModule can use this util function. Thoughts?

Copy link
Member

@tech4GT tech4GT left a comment

Choose a reason for hiding this comment

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

@jywarren I got it!! Actually the problem is that this is running the new sequencer in browser context and hence it tries to draw the steps for internal sequencer in the browser but runs into an exception, we need to set the inBrowser as false for internalSequencer. Working on a fix now!

@tech4GT
Copy link
Member

tech4GT commented Feb 12, 2019

Hi Jeff, Can you please allow edits from maintainers so I can push in the fix 😅

@tech4GT
Copy link
Member

tech4GT commented Feb 12, 2019

I am not able to push to jywarren:opaque
Wonder why this is happening since it allows to me edit the pr using the inline editor??

@jywarren
Copy link
Member Author

Ah, we made a mistake with inBrowser = inBrowser || true, essentially it could never be made false, added a fix for (inBrowser === true). We should probably document this better, and run a test for it.

@tech4GT
Copy link
Member

tech4GT commented Feb 12, 2019

Yeah, I fixed it!!

@tech4GT
Copy link
Member

tech4GT commented Feb 12, 2019

Do you want me to make a pull request on jywarren:opaque ??

@tech4GT
Copy link
Member

tech4GT commented Feb 12, 2019

Also the function is sequencer.loadImage and not addImage 😅

@jywarren
Copy link
Member Author

sure, and edits by maintainers is already allowed... sorry i am on the run at the moment, but can you add a test to demonstrate it working? That's a good idea anyways! Thanks Varun!

@tech4GT
Copy link
Member

tech4GT commented Feb 12, 2019

I'll add the test and make the pr! This is working like a charm btw!! 🎉

@jywarren
Copy link
Member Author

I tried a fix but didn't get the colorbar module to appear. But very happy to take your fix, thank you! Agreed this is very fun!

We can really make a generator function which essentially parameterizes everything shown in this example module; it should be pretty straightforward.

@tech4GT
Copy link
Member

tech4GT commented Feb 12, 2019

Yeah I am drawing up a design right now! 😃

@jywarren
Copy link
Member Author

You can also just open a new PR building on these commits and close this one if you want. Thanks! Won't be able to check in for a few hours though.

@tech4GT
Copy link
Member

tech4GT commented Feb 12, 2019

Integrating the generator into createMetaModule

@tech4GT
Copy link
Member

tech4GT commented Feb 12, 2019

Oh okay will do! That's okay! 👍

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.

3 participants