search

DDMAL / pixel_wrapper

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

Ignore background layer when exporting to CSV / image data PNG #20

Closed EricHanLiu closed 6 years ago

EricHanLiu commented 6 years ago

This is a possible fix to the background layer zoom issue, since the exportToImageData/CSV aren't needed for CM yet still work standalone with Pixel.js.

Now the background layer is only needed for exporting to highlights (so user can view their work) or submitting to Rodan (CM requires the highlights, not the image data).

Ideally it'd be nice to have the background feature work with the image data as well, but there seems to be an ideology conflict with the background layer generation and the image data generation (this is explained in the issue linked above).

I'm guessing that the background layer functionality doesn't need to be a feature in the standalone Pixel.js, but only for the Rodan job.

napulen commented 6 years ago

Alright, just one question:

Are we checking for the background layer in a lot of places around the code? I am worried that we might change the inner text of the DOM element (even just for aesthetic purposes) and break the logic of this or other places. Consider having an object with "state" variables instead, not only for the background layer but for all the boolean states that you might want to be tracking within pixel.js, maybe there is one already? For now, this serves the purpose.

EricHanLiu commented 6 years ago

I think I've accounted for the innertext of the DOM element changing in the different places that the background layer is generated.

Once I manage to get the separate pixel thing implemented it should be a lot clearer with the button.

Oh, okay I think I see what you mean. So you're saying to add another attribute to the button that we can check whether or not it's been clicked instead of checking against the innertext?

napulen commented 6 years ago

Yes, basically what I mean is that you are checking a literal string value "Background Generated!", imagine we change the text that button will display in the future to something else, then you will have to go through an additional step of replacing that string wherever you are using it in if conditions. This would be unnecessary if you control the state of the background layer button with some internal logic, an additional attribute in the button is one option.

EricHanLiu commented 6 years ago

Got it, I can add that right now