DDMAL / pixel_wrapper

Rodan Pixel.js Wrapper
Other
0 stars 1 forks source link

Isolate pixel_wrapper from Pixel so we can use pixel as a submodule #10

Closed EricHanLiu closed 6 years ago

EricHanLiu commented 6 years ago
EricHanLiu commented 6 years ago

See separate pixel branch for current progress.

So far, we can interact with the webpacked file by binding the class to the window as a variable and accessing this variable in pixel_wrapper.js (in static). However, it's very difficult (if not impossible with webpack) to properly create an instance of this class, which all methods in pixel_wrapper.js require.

EricHanLiu commented 6 years ago

I'm pausing further work on this since it's not as important as getting the background layer working properly (another problem arose).

EricHanLiu commented 6 years ago

List of attempts:

EricHanLiu commented 6 years ago

Tried exposing Pixel as a library and interacting with module.exported functions, but the main issue remains instantiating the pixelInstance at the appropriate time and making sure it exists when trying to create the wrapper elements. I even check for if the instance is undefined, but there seems to be an issue with that since it passes as defined yet throws an undefined console error...

Branch

EricHanLiu commented 6 years ago

Update!

I can just isolate all methods into a PixelWrapper class and instantiate this class in the pixel.js code when the plugin is activated (activatePlugin()), and then write some script that injects this exact code so we don't have to actually alter the standalone Pixel.

We'd just have the new pixel_wrapper.js file which contains all the Rodan-related methods, and a script which injects the instantiation code into pixel.js once it's imported as a submodule (and we can just add documentation to run this script when setting up pixel_wrapper).

The file structure could look something like this

pixel_wrapper/source/js/plugins/
  -> Pixel.js/
    -> pixel_wrapper.js (as the only file)

Then we use the git submodule somehow within this Pixel.js directory and run the script.

vigliensoni commented 6 years ago

Excellent!

On Fri, May 25, 2018 at 11:03 AM, Eric Han Liu notifications@github.com wrote:

Update!

I can just isolate all methods into a PixelWrapper class and instantiate this class in the pixel.js code when the plugin is activated ( activatePlugin()), and then write some script that injects this exact code so we don't have to actually alter the standalone Pixel.

We'd just have the new pixel_wrapper.js file which contains all the Rodan-related methods, and a script which injects the instantiation code into pixel.js once it's imported as a submodule (and we can just add documentation to run this script when setting up pixel_wrapper).

The file structure could look something like this

pixel_wrapper/source/js/plugins/ -> Pixel.js/ -> pixel_wrapper.js (as the only file)

Then we use the git submodule somehow within this Pixel.js directory and run the script.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/DDMAL/pixel_wrapper/issues/10#issuecomment-392086384, or mute the thread https://github.com/notifications/unsubscribe-auth/AAeCxTpbRUEtwGgvbyar8KwvUYvKB_JGks5t2B0tgaJpZM4T55td .

EricHanLiu commented 6 years ago

I'm gonna stop progress on the separate_pixel_webpack branch which attempted to separate pixel_wrapper elements by interacting with the webpacked file (as a library) since it mostly didn't work.

Further work will be done on the separate_pixel branch, where I'll try the method mentioned in my last comment.

napulen commented 6 years ago

Sounds good, any idea on how to perform the injection?

EricHanLiu commented 6 years ago

Maybe a python script that parses through the pixel.js file and inserts the few lines needed where necessary!

As of now all pixel_wrapper methods are separated into a file pixel-wrapper.js inside the plugins/ folder. I'll start with cloning the standalone Pixel into my plugins/ folder and see if I can interact with that, then eventually turn that into a submodule.

I'll have to edit one portion of Pixel though (the ignore background layer changes I made in the fix_background_zoom branch earlier), but it won't affect the standalone version's functionality (it only affects things once it's imported in the wrapper).

napulen commented 6 years ago

I would encourage finding a solution within javascript. I am not familiar with this, but for the fragment of code you showed me, this looks like it could be solved with a "callback" function. If you can by some way get a variable that points to the pluginActivate() function, then you can make another function (sort of a wrapper, which we usually know as a callback) that calls the original one, and adds a few extra code afterwards.

Read this and let us know if it sounds reasonable.

http://javascriptissexy.com/understand-javascript-callback-functions-and-use-them/

EricHanLiu commented 6 years ago

Ok I've implemented Pixel.js as a git submodule in the plugins/ directory. I've manually added the few lines that needed to be changed for the wrapper to currently work, but I'll work on finding a way to inject them using JavaScript.

I can push a separate branch to Pixel.js with the addition of ignoring the background layer for exportToData/CSV, and the extra code doesn't break the standalone Pixel.js either, and perhaps a pull request if the changes should be implemented in the standalone pixel? (this would make it easier to work with the wrapper)

OR what I can do is completely scrap the background button, and only have it generate on submitToRodan. This way I wouldn't need to change the standalone Pixel code, just the pixel-wrapper.js code, and it would only need to be generated once the user submits and Pixel.js is done.

EricHanLiu commented 6 years ago

Actually I think that's better (removing the button), so it only gets generated once submitting to Rodan. This fixes a separate bug with the current standalone (up to date) Pixel I'm using, and I won't have to alter the pixel code on its own

I'll still need to find a way to inject those few lines, but by removing the background button it removes a bug with the createNewLayer button

napulen commented 6 years ago

So the button will not exist in standalone Pixel.js, am I following that correctly?

napulen commented 6 years ago

After the discussions I have seen lately, I am starting to think it would be productive to sit down and delimit what should and should not be in the standalone and wrapper versions. As of now, it seems blurry where the line is between the two in terms of features and implementation.

EricHanLiu commented 6 years ago

No extra buttons will exist in the standalone Pixel.js that don't already exist there. In the pixel_wrapper, I will only add the "Submit To Rodan" button, which will generate the background layer before submitting (pretty much how I had it originally).

I'm testing it now to see if the changes I've made are compatible with Rodan

napulen commented 6 years ago

Right. Sounds good.

EricHanLiu commented 6 years ago

Nestor and I tried today to see if we could omit having to use a script that inserts code into the pixel.js file, by interacting with the pixel-wrapper.js file and listening to an event that's thrown when Pixel is instantiated, and the eventListener then instantiates PixelWrapper.

However (and this seems kind of obvious in hindsight), without including the pixel-wrapper.js in pixel.js, pixel-wrapper.js never gets compiled by webpack (since pixel.js is the entrypoint). But we want to avoid having to include the pixel-wrapper.js file in the standalone Pixel, and changing the webpack entrypoint to pixel-wrapper.js also presents a plethora of other issues with Diva, so I think we'll have to resort to maybe a python script that adds the lines of code to the standalone Pixel file.

EricHanLiu commented 6 years ago

Works with every method except the import method (which takes the input layers and displays them as canvases).

I think this is because the standalone Pixel.js by default only creates 1 layer, so the actual layers don't get displayed because of this (there's no console error anywhere though).

I'll try setting the default layers to 3 (which is needed for manuscript classification anyway), and they can be deleted if needed. Maybe I'll add in the python script to create these layers so I don't have to alter the standalone Pixel.js

EricHanLiu commented 6 years ago

Set the default layers to 3, still not displaying the inputted layers correctly. They are uploaded, the proper RGB values are printed when I log the layer's colours, but they're not being displayed for some reason.

EricHanLiu commented 6 years ago

Fixed! It was an issue with the preBinarizedImageCanvas not being named the same thing in the standalone Pixel version.

EricHanLiu commented 6 years ago

I believe the default number of layers should be 3 to start, not just 1. It's set to 3 in the wrapper, and 1 in the standalone Pixel.js.

@vigliensoni should I send a pull request to the Pixel repo to have this reverted back to having 3 layers to start, or should I just have the python script add the new layers so that there'll be 3 layers only in the wrapper Pixel.js? I think the standalone Pixel.js set the default to just 1 layer for generalizability, so maybe I shouldn't change that?

EricHanLiu commented 6 years ago

Added the default layer modification code to the add_wrapper.py script instead of altering the Pixel.js repo.

EricHanLiu commented 6 years ago

It's done!

Pixel.js is now a submodule with the latest commit version from master being linked to pixel_wrapper. All Rodan methods have been modified to work with the latest version of Pixel, and adapted to fit in the same class (as they were originally split across numerous classes). README instructions have been updated with the new/reordered commands for building everything properly.

The wrapper is activated with a python script which adds instantiation lines to the pixel.js file. This is the only method that I found worked (after trying the several methods listed above).

Exporting to Rodan can only be done if there are 3 layers present (an alert is sent otherwise), since we assume that now the CM requires 3 layers + the background. Of course, the CM requirements can change from case to case, but for now since the actual CM job only accepts/outputs 4 layers (and Jorge and I are unsure how variablize the number of ports). Otherwise, the job will crash if Rodan doesn't receive the right number of produced files (in this case 4 files)

After speaking with Jorge it seemed it'd be hard to generalize this number of input/output ports to change based on the number of Pixel layers, so that might be a future issue if we wish to generalize everything to n input/output ports (though it should be doable).

Pull request created.

EricHanLiu commented 6 years ago

Pull request accepted, closing