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

Added Demo for FisheyeGl Module #62

Merged
merged 10 commits into from
Aug 6, 2017
Merged

Conversation

ccpandhare
Copy link
Collaborator

I have added a demo for the FisheyeGl module and it is now live at https://ccpandhare.github.io/image-sequencer/examples/fisheye

As #27 Suggests, I should add a 'crop+fisheyegl' demo. Should I add that in addition to this or remove this altogether and add just that?

@ccpandhare ccpandhare added the almost-complete PRs that are almost done but little more work. label Jul 26, 2017
@ccpandhare ccpandhare self-assigned this Jul 28, 2017
@ccpandhare
Copy link
Collaborator Author

ccpandhare commented Aug 1, 2017

I have made the Demo like the one in https://jywarren.github.io/fisheyegl/example/
It can be seen at https://ccpandhare.github.io/image-sequencer/examples/fisheye

This has been made with the new UI System

@ccpandhare ccpandhare added ready-to-merge and removed almost-complete PRs that are almost done but little more work. labels Aug 1, 2017
@ccpandhare ccpandhare mentioned this pull request Aug 1, 2017
@ccpandhare
Copy link
Collaborator Author

@ccpandhare
Copy link
Collaborator Author

Once this is merged in, I will add a link to it in the description.
Also we should now have a gh-pages site for publiclab/image-sequencer too

@jywarren
Copy link
Member

jywarren commented Aug 3, 2017

OK - this is super. I have some requests and thoughts that will be relevant to the UI system.

First, specifically on this module's demo, there are some UI features (very recently added) which make the fisheye system much more useful:

  • drag drop
  • url hash used to store the settings, so you can pass a permalink that's a "preset" for a specific camera
  • storing previous images in a grid below, to make batch processing easier

These are all in main.js here: https://github.com/jywarren/fisheyegl/blob/master/example/main.js

I think we could reformat that file to simply be directly re-usable somehow in this demo, so upgrades to that UI code can be available here too. But is this a sustainable workflow?

More generally, many modules may have very specific UIs designed just for their particular use, and which make them much more useful. How can we design the system to allow modules to supply a UI that's hyper-specific in a flexible but standard way?

Could fisheye also publish a separate fisheye-ui? or could there be a baked-in module.ui?

What are your thoughts?

@ccpandhare
Copy link
Collaborator Author

ccpandhare commented Aug 3, 2017 via email

@jywarren
Copy link
Member

jywarren commented Aug 3, 2017 via email

@ccpandhare
Copy link
Collaborator Author

I have some concerns here, is Image Sequencer supposed to provide the UI?
Shouldn't the UI be implementation specific?

Maybe we could provide a standard UI - Users can choose to use it or not.

But honestly, I don't think Image Sequencer should be handling front end code (By that i mean the markup and the stylesheets).
It should definitely have an API for UI development, but maybe not provide UI itself, and serve it's main purpose of image processing.... What do you think about this aspect of Image Sequencer? What are your goals for Image Sequencer in this regard?

@jywarren
Copy link
Member

jywarren commented Aug 4, 2017

Very good thoughts, thanks.

I had to think about this for a bit. I think we definitely ought to provide a standard UI -- one which makes it very easy to try out and use the core modules.

But on consideration, what I'm proposing is not that we add all the features of main.js from fisheyegl to image-sequencer, but that we /remove/ them and come up with a standard way that fisheyegl -- or maybe fisheyegl-ui, a separate module that requires fisheyegl -- can provide a more complex HTML UI. Because I think many modules will have a very complex and refined way they enable people to interact with them in HTML.

I think the UI that fisheyegl provides should have its own CSS + HTML if it wants, and that we provide a minimal standard CSS in image-sequencer so things don't look too inconsistent.

How does that sound? Do you think modules should provide their own UI, or should have a corresponding _____-ui module that provide the UI? I lean towards providing their own as a default; they could opt for a separate UI module if they like, which then wraps the original module.

@jywarren
Copy link
Member

jywarren commented Aug 4, 2017

If you like this proposal, fisheyegl's UI could be tidied up and modularized from its main.js file, and then we could test out this arrangement using this as the first module to provide its own UI. What do you think?

@@ -10,8 +10,8 @@ require('../src/ImageSequencer.js');
//require image files as DataURLs so they can be tested alike on browser and Node.
var sequencer = ImageSequencer({ ui: false });

var test_png = require('../examples/test.png.js');
var test_gif = require('../examples/test.gif.js');
var test_png = require('../examples/images/test.png.js');
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should keep test images in /test/images/ even if it's redundant... what do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes that does sound good. I shall do that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed this in an upcoming commit

@@ -42,7 +42,7 @@ test('loadImages loads a DataURL image and creates a step.', function (t){

if(!sequencer.options.inBrowser)
test('loadImage loads an image from URL and creates a step. (NodeJS)', function (t){
sequencer.loadImage('URL','https://ccpandhare.github.io/image-sequencer/examples/red.jpg', function(){
sequencer.loadImage('URL','https://ccpandhare.github.io/image-sequencer/examples/images/red.jpg', function(){
Copy link
Member

Choose a reason for hiding this comment

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

I like that you want to test this, but I'm concerned about making tests rely on an internet connection. Is there any other way to do this? I guess we can leave it as is for now, but you know what I mean? It just ... feels a bit wrong i guess... but I can't come up with a really really good reason why :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I had thought about this, too.
But I just couldn't come up with a substitute for this.
Maybe we could run this test if and only internet access is available?

Copy link
Collaborator Author

@ccpandhare ccpandhare Aug 6, 2017

Choose a reason for hiding this comment

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

I am checking for internet access in an upcoming commit.
Hope that is sufficient.

@jywarren
Copy link
Member

jywarren commented Aug 4, 2017

I guess also, what does this mean for this PR? Could we, or should we:

  1. just merge this in and think about the custom UI provision separately
  2. use this as the first instance of a module providing its own UI -- test out this idea?

Thanks for thinking through this carefully. It really does get us deeply into the UI phase of the work :-)

@jywarren
Copy link
Member

jywarren commented Aug 5, 2017 via email

@ccpandhare
Copy link
Collaborator Author

Thanks for the review! To sum up and suggest further changes:

  • We had a discussion about whether image sequencer should provide a UI or not. We concluded that Image Sequencer should provide a basic UI and that modules should have the liberty to suggest UI.
    Further thoughts on this: I think the module should specify only the elements and the general CSS of image-sequencer should be responsible for styling. Which means modules specify the UI but not style it. What do you think?

@ccpandhare
Copy link
Collaborator Author

  • I think this is a major upgrade and should go in a different PR

@jywarren
Copy link
Member

jywarren commented Aug 5, 2017 via email

@ccpandhare ccpandhare mentioned this pull request Aug 6, 2017
@ccpandhare
Copy link
Collaborator Author

Okay!

@ccpandhare
Copy link
Collaborator Author

If you are fine with the changes I made, I shall merge this in!

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.

Looks fantastic. Great work! :-) 🎈

@jywarren jywarren merged commit 14b1152 into publiclab:master Aug 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants