Circular gradient #869

niravasher · 2019-03-17T15:34:46Z

Fixes #[Add issue number here.]

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with npm test
code is in uniquely-named feature branch and has no merge conflicts
PR is descriptively titled
ask @publiclab/is-reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

niravasher · 2019-03-17T15:35:14Z

@publiclab/is-reviewers please have a look at it

harshkhandeparkar · 2019-03-17T16:10:49Z

@publiclab/is-reviewers

src/modules/CircularGradient/Module.js

harshkhandeparkar · 2019-03-17T16:14:26Z

The best example for a module is drawRectangle

https://github.com/publiclab/image-sequencer/blob/main/src/modules/DrawRectangle/module.js
And

https://github.com/publiclab/image-sequencer/blob/main/src/modules/DrawRectangle/DrawRectangle.js

niravasher · 2019-03-18T20:12:08Z

I hope the PR got updated. Also keeping the save function for now to test something, will remove later

harshkhandeparkar

Can you provide an image for the output. Also have you cleared the cache?

niravasher · 2019-03-18T20:27:29Z

The putput is a weird looking gradient in the top-left quadrant. If I include the top-left quadrant, it just overlaps the top left quadrant with a linear gradient
@harshkhandeparkar
Yes I cleared the cache twice

jywarren

Hi @niravasher!! Try these suggestions, then use the monochrome output and add the colormap module. You'll see how the monochrome will map to various color maps in the next step!

Great work, this is almost there! Love the trigonometry 👍 😄 📐

src/modules/CircularGradient/Module.js

src/modules/CircularGradient/info.json

harshkhandeparkar · 2019-03-20T09:34:33Z

Can you commit your new code?

niravasher · 2019-03-20T14:15:27Z

Updated my PR @harshkhandeparkar @jywarren

harshkhandeparkar

Please do a rebase

jywarren

OK, went though and left some requests and tips, thank you @niravasher this is great work!

jywarren · 2019-03-20T14:52:36Z

src/modules/CircularGradient/circularGradient.js

+        width = width-1;
+    }
+    var increment = width/2;
+    console.log(width);


Oops, let's take out the log statements!

Suggested change

console.log(width);

jywarren · 2019-03-20T14:52:51Z

package.json

@@ -36,6 +36,7 @@
    "image-sequencer-invert": "^1.0.0",
    "imagejs": "0.0.9",
    "imgareaselect": "git://github.com/jywarren/imgareaselect.git#v1.0.0-rc.2",
+    "infinite-gradients": "0.0.1",


Are you using this? if not, let's take it back out, thanks!

jywarren · 2019-03-20T14:54:40Z

src/modules/CircularGradient/circularGradient.js

+
+    let defaults = require('./../../util/getDefaults.js')(require('./info.json'));
+
+    let fillColor = options.fillColor || defaults.fillColor;


OK so here we can just set fillColor to white, so we can remove all the fillColor code, make sense? we only want monochrome for now.

Suggested change

let fillColor = options.fillColor || defaults.fillColor;

jywarren · 2019-03-20T14:55:01Z

src/modules/CircularGradient/circularGradient.js

+            var distance = Math.sqrt(Math.pow(distX, 2) + Math.pow(distY, 2));
+            var brightness = 255 * (distance / (2*width));
+            if (distance-1 > width) {distance = width;}
+            pixels.set(i, j, 0, fillColor[0]*1.5*distance/width);


Right so on these three lines, we can just remove the fillcolor[.] parts, and it'll fill evenly with white.

Suggested change

pixels.set(i, j, 0, fillColor[0]*1.5*distance/width);

pixels.set(i, j, 0, 255 * distance / width);

jywarren · 2019-03-20T14:55:22Z

src/modules/CircularGradient/info.json

+    "name": "Circular Gradient",
+    "description": "Gives a circular gradient of the image",
+    "inputs": {
+        "fillColor": {


And let's remove this parameter here, we'll be able to do all color work in the next module!

Just reminding that we should remove this whole parameter, down to line 10!

jywarren · 2019-03-20T14:56:30Z

src/modules/Gradient/Module.js

-                callback();
-            });
-        });
+        function extraManipulation(pixels) {


Uh oh, would you mind leaving the existing Gradient module as it is for the time being? We can work on it in a follow-up step, so lets keep this PR simpler. Thanks!

Removed the changes. I added the feature for choosing the direction of gradient. Will update this in next PR

jywarren · 2019-03-20T14:57:31Z

src/modules/Gradient/info.json

@@ -1,6 +1,13 @@
 {
    "name": "Gradient",
    "description": "Gives a gradient of the image",
-    "inputs": {},
+    "inputs": {


Oh i see! OK, well, we can leave this in if you like, but perhaps in the future it's best to do only one concrete thing in each PR, so we have less to review and it's more self-contained. Make sense? Thank you!

This is a cool addition to this module though 😄

Hehe, this is funny. Will take care for such kind of changes from the next time. 😄 😄

niravasher · 2019-03-21T13:38:11Z

Pushed all new suggested changes to this. Why am I still getting those conflicts? @harshkhandeparkar @jywarren

Divy123 · 2019-03-22T20:06:47Z

There are conflicts due to some other PRs already merged after your last rebase.
So you can again make a rebase but lets work on the reviews by the reviewers first.

Divy123

Please look into the suggested changes.
Awesome job !! 😄

Divy123 · 2019-03-22T20:51:34Z

src/Modules.js

@@ -8,6 +8,7 @@ module.exports = {
  'blur': require('./modules/Blur'),
  'brightness': require('./modules/Brightness'),
  'channel': require('./modules/Channel'),
+  'circularGradient': require('./modules/circularGradient'),


It should be ./modules/CircularGradient inside require here.

Divy123 · 2019-03-22T20:52:03Z

src/modules/CircularGradient/Module.js

+
+        };
+
+        function output(image, datauri, mimetype) {


We can remove unused variables here .

Divy123 · 2019-03-22T20:57:55Z

src/modules/CircularGradient/circularGradient.js

@@ -0,0 +1,37 @@
+module.exports = exports = function(pixels, options) {


Its recommended not to assign a value to exports directly.
In this case you can directly do module.exports = function(...)
exports should be used only to add propoerties to it not assigning it directly.

Suggested change

module.exports = exports = function(pixels, options) {

module.exports = function(pixels, options) {

Divy123 · 2019-03-22T21:01:43Z

src/modules/CircularGradient/circularGradient.js

+            var distX = Math.abs(options.centreX - i);
+            var distY = Math.abs(options.centreY - j);
+            var distance = Math.sqrt(Math.pow(distX, 2) + Math.pow(distY, 2));
+            var brightness = 255 * (distance / (2*width));


I think brightness is unused here.
So this can be removed.

Suggested change

var brightness = 255 * (distance / (2*width));

niravasher · 2019-03-23T23:54:51Z

Ill make all the changes suggested by you @Divy123 , I am just not able to get rid of these merge conflicts. I am making unwanted commits.

Divy123 · 2019-03-25T19:53:50Z

@niravasher what can be done here is you can go one commit behind on this.
I think some wrong files have changed here due to rebase done incorrectly.
You can go behind one or two commits back when there was only your changes and I suggest there is no need to rebase until you are done with your work completely.
It should be the last step.
Please go a commit behind to that version where there were only your changes.

Divy123 · 2019-03-25T19:54:17Z

We will see the merge conflicts once you are on the rght version.

jywarren · 2019-03-30T23:55:58Z

Hi! I spent some time to extract /only/ the circular gradient work from here, and make just one commit. @niravasher it may be helpful to read through some guides on Git workflow -- it can get pretty complex, but it's worth it and I know you'll be an expert in no time 😄 👍

http://publiclab.org/developers#Resources has some good links, and https://dev.to has lots of nice guides too!

I'll make some suggestions now in the simplified commit.

jywarren

Hi @niravasher I went through and made suggestions for every change necessary, and then the removal of the fillColor parameter in info.json. If you can make these changes and upload one last screenshot, we should be able to merge this PR! Thank you so much!!!

jywarren · 2019-03-30T23:56:31Z

src/Modules.js

@@ -36,6 +38,5 @@ module.exports = {
  'saturation': require('./modules/Saturation'),
  'text-overlay': require('./modules/TextOverlay'),
  'threshold': require('./modules/Threshold'),
-  'tint': require('./modules/Tint'),
-  'color-temperature': require('./modules/ColorTemperature')
+  'tint': require('./modules/Tint')


don't worry about these, i just alphabetized them!

jywarren · 2019-03-30T23:57:18Z

src/modules/CircularGradient/circularGradient.js

@@ -0,0 +1,37 @@
+module.exports = exports = function(pixels, options) {


Suggested change

module.exports = exports = function(pixels, options) {

module.exports = function(pixels, options) {

jywarren · 2019-03-30T23:57:44Z

src/modules/CircularGradient/circularGradient.js

+
+    let defaults = require('./../../util/getDefaults.js')(require('./info.json'));
+
+    let fillColor = options.fillColor || defaults.fillColor;


Suggested change

let fillColor = options.fillColor || defaults.fillColor;

jywarren · 2019-03-30T23:57:57Z

src/modules/CircularGradient/circularGradient.js

+        width = width-1;
+    }
+    var increment = width/2;
+    console.log(width);


Suggested change

console.log(width);

jywarren · 2019-03-30T23:58:08Z

src/modules/CircularGradient/circularGradient.js

+    }
+    var increment = width/2;
+    console.log(width);
+    console.log(height);


Suggested change

console.log(height);

jywarren · 2019-03-30T23:58:48Z

src/modules/CircularGradient/circularGradient.js

+            var distX = Math.abs(options.centreX - i);
+            var distY = Math.abs(options.centreY - j);
+            var distance = Math.sqrt(Math.pow(distX, 2) + Math.pow(distY, 2));
+            var brightness = 255 * (distance / (2*width));


Suggested change

var brightness = 255 * (distance / (2*width));

jywarren · 2019-03-30T23:59:36Z

src/modules/CircularGradient/circularGradient.js

+            var distance = Math.sqrt(Math.pow(distX, 2) + Math.pow(distY, 2));
+            var brightness = 255 * (distance / (2*width));
+            if (distance-1 > width) {distance = width;}
+            pixels.set(i, j, 0, fillColor[0]*1.5*distance/width);


Suggested change

pixels.set(i, j, 0, fillColor[0]*1.5*distance/width);

pixels.set(i, j, 0, 255 * distance / width);

jywarren · 2019-03-31T00:00:01Z

src/modules/CircularGradient/circularGradient.js

+            var brightness = 255 * (distance / (2*width));
+            if (distance-1 > width) {distance = width;}
+            pixels.set(i, j, 0, fillColor[0]*1.5*distance/width);
+            pixels.set(i, j, 1, fillColor[1]*1.5*distance/width);


Suggested change

pixels.set(i, j, 1, fillColor[1]*1.5*distance/width);

pixels.set(i, j, 1, 255 * distance / width);

jywarren · 2019-03-31T00:00:12Z

src/modules/CircularGradient/circularGradient.js

+            if (distance-1 > width) {distance = width;}
+            pixels.set(i, j, 0, fillColor[0]*1.5*distance/width);
+            pixels.set(i, j, 1, fillColor[1]*1.5*distance/width);
+            pixels.set(i, j, 2, fillColor[2]*1.5*distance/width);


Suggested change

pixels.set(i, j, 2, fillColor[2]*1.5*distance/width);

pixels.set(i, j, 2, 255 * distance / width);

jywarren · 2019-03-31T00:00:56Z

src/modules/CircularGradient/info.json

+    "name": "Circular Gradient",
+    "description": "Gives a circular gradient of the image",
+    "inputs": {
+        "fillColor": {


Just reminding that we should remove this whole parameter, down to line 10!

niravasher · 2019-04-01T08:36:08Z

I am deleting this repository and forking once again, will make a new pull request @jywarren New PR will have all the changes suggested by you

harshkhandeparkar · 2019-04-01T08:36:39Z

Wait!

harshkhandeparkar · 2019-04-01T08:36:56Z

You don't have to retork

harshkhandeparkar · 2019-04-01T08:39:00Z

You can run git reset --hard upstream/main to make your current branch perfectly even with upstream/main.

You can run this command on your own main branch and then create a new branch from it

harshkhandeparkar · 2019-10-26T11:37:58Z

Another pr for the same is open.

harshkhandeparkar requested changes Mar 17, 2019

View reviewed changes

src/modules/CircularGradient/Module.js Outdated Show resolved Hide resolved

harshkhandeparkar reviewed Mar 18, 2019

View reviewed changes

jywarren requested changes Mar 20, 2019

View reviewed changes

src/modules/CircularGradient/Module.js Outdated Show resolved Hide resolved

src/modules/CircularGradient/Module.js Outdated Show resolved Hide resolved

src/modules/CircularGradient/info.json Outdated Show resolved Hide resolved

harshkhandeparkar requested changes Mar 20, 2019

View reviewed changes

jywarren requested changes Mar 20, 2019

View reviewed changes

jywarren mentioned this pull request Mar 21, 2019

Seo tags for google search #914

Merged

4 tasks

jywarren requested a review from Divy123 March 22, 2019 19:48

Divy123 requested changes Mar 22, 2019

View reviewed changes

just circular gradient

c0cf6fb

jywarren requested changes Mar 31, 2019

View reviewed changes

jywarren mentioned this pull request Jun 15, 2019

new circular gradient without merge conflicts #1076

Closed

4 tasks

harshkhandeparkar closed this Oct 26, 2019

niravasher mentioned this pull request Jan 14, 2020

circular gradient module #1496

Merged

5 tasks


		let defaults = require('./../../util/getDefaults.js')(require('./info.json'));

		let fillColor = options.fillColor \|\| defaults.fillColor;

	pixels.set(i, j, 0, fillColor[0]1.5distance/width);
	pixels.set(i, j, 0, 255 * distance / width);

		@@ -0,0 +1,37 @@
		module.exports = exports = function(pixels, options) {

	module.exports = exports = function(pixels, options) {
	module.exports = function(pixels, options) {

	pixels.set(i, j, 1, fillColor[1]1.5distance/width);
	pixels.set(i, j, 1, 255 * distance / width);

	pixels.set(i, j, 2, fillColor[2]1.5distance/width);
	pixels.set(i, j, 2, 255 * distance / width);

Circular gradient #869

Circular gradient #869

Conversation

niravasher commented Mar 17, 2019

niravasher commented Mar 17, 2019

harshkhandeparkar commented Mar 17, 2019

harshkhandeparkar commented Mar 17, 2019 • edited Loading

niravasher commented Mar 18, 2019

harshkhandeparkar left a comment

Choose a reason for hiding this comment

niravasher commented Mar 18, 2019 • edited Loading

jywarren left a comment

Choose a reason for hiding this comment

harshkhandeparkar commented Mar 20, 2019

niravasher commented Mar 20, 2019

harshkhandeparkar left a comment

Choose a reason for hiding this comment

jywarren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niravasher Mar 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niravasher commented Mar 21, 2019

Divy123 commented Mar 22, 2019 • edited Loading

Divy123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niravasher commented Mar 23, 2019

Divy123 commented Mar 25, 2019

Divy123 commented Mar 25, 2019

jywarren commented Mar 30, 2019

jywarren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niravasher commented Apr 1, 2019

harshkhandeparkar commented Apr 1, 2019

harshkhandeparkar commented Apr 1, 2019

harshkhandeparkar commented Apr 1, 2019

harshkhandeparkar commented Oct 26, 2019

harshkhandeparkar commented Mar 17, 2019 •

edited

Loading

niravasher commented Mar 18, 2019 •

edited

Loading

niravasher Mar 20, 2019 •

edited

Loading

Divy123 commented Mar 22, 2019 •

edited

Loading