Made a new method to return Geometry with no internal colors. by perminder-17 · Pull Request #6498 · processing/p5.js

perminder-17 · 2023-10-26T10:59:46Z

Resolves #6384

Changes:

PR.mp4

Description:

let shap01;
function setup() {
renderer = createCanvas(600, 600, WEBGL);
buildShape02();
}
function draw() {
background(0);
fill("yellow");
model(shape02);
}

function buildShape02() {
beginGeometry();
fill("red"); //The box is red
box(50);
shape02 = endGeometry();
shape02.clearColors(); //After calling this the box becomes yellow as it removes internal colors.
}

PR Checklist

npm run lint passes
Inline documentation is included / updated
Unit tests are included / updated

davepagurek

Nice work on this! I think we just want to add some documentation to the new method now.

davepagurek · 2023-10-26T11:28:51Z

src/webgl/3d_primitives.js

+ * Clears the internal Color of the Geometry.
+ * @method clearColors
+ */
+p5.prototype.clearColors = function () {


I think maybe we can take out this method now that we have one on p5.Geometry.

davepagurek · 2023-10-26T11:31:17Z

src/webgl/p5.Geometry.js

    this.dirtyFlags = {};
  }

+  clearColors() {


Do you think you can add a doc comment around this one? It would be nice to add your example in the video in your PR description. Maybe we can add two examples: one showing the behaviour without clearColors(), and one showing the behaviour with clearColors()?

Here's an example of a doc comment with an example sketch on another class. Adding one here would look similar:

p5.js/src/webgl/p5.Framebuffer.js

Lines 750 to 794 in 716554f

/**

* Removes the framebuffer and frees its resources.

*

* @method remove

*

* @example

* <div>

* <code>

* let framebuffer;

* function setup() {

* createCanvas(100, 100, WEBGL);

* }

*

* function draw() {

* const useFramebuffer = (frameCount % 120) > 60;

* if (useFramebuffer && !framebuffer) {

* // Create a new framebuffer for us to use

* framebuffer = createFramebuffer();

* } else if (!useFramebuffer && framebuffer) {

* // Free the old framebuffer's resources

* framebuffer.remove();

* framebuffer = undefined;

* }

*

* background(255);

* if (useFramebuffer) {

* // Draw to the framebuffer

* framebuffer.begin();

* background(255);

* rotateX(frameCount * 0.01);

* rotateY(frameCount * 0.01);

* fill(255, 0, 0);

* box(30);

* framebuffer.end();

*

* image(framebuffer, -width/2, -height/2);

* }

* }

* </code>

* </div>

*

* @alt

* A rotating red cube blinks on and off every second.

*/

remove() {

Okay, I will add a doc comment around this one.

perminder-17 · 2023-10-26T18:58:59Z

@davepagurek , I have added a doc comment with two examples. Can you check and suggest what else we need?

davepagurek

Nice work so far, and great comments in the example! I just have a few more comments to clean up the example a bit.

davepagurek · 2023-10-26T21:46:26Z

src/webgl/p5.Geometry.js

+   *   for (let vec of points) vertex(vec.x * 200, vec.y * 200, vec.z * 200);
+   *   endShape(CLOSE);
+   *   shape02 = endGeometry();
+   *   // You can also call shape02.clearColors() here.


How do you feel about putting the clearColors() here? It also works having it in draw, but it only needs to be called once, so it feels like only doing it once when building the shape fits more cleanly.

davepagurek · 2023-10-26T21:47:06Z

src/webgl/p5.Geometry.js

+   * let points = [];
+   *
+   * function setup() {
+   *   renderer = createCanvas(600, 600, WEBGL);


I think we can take out the renderer = because we don't use it elsewhere.

Oh yess... I will take out that.

davepagurek · 2023-10-26T21:47:51Z

src/webgl/p5.Geometry.js

  }
-
+  /**
+   * Removes the internal Colors of p5.Geometry


Maybe we can add one more sentence after this explaining that this lets you supply new colors with fill() before drawing it? And otherwise, fill() gets ignored?

Yes. I was also thinking of that... Okay going to add this.

One other small request: could we add a period at the end of the sentence so it fits with the style of the next bit of text? other than that I think everything looks good!

davepagurek · 2023-10-26T22:51:46Z

src/webgl/p5.Geometry.js

   * Removes the internal Colors of p5.Geometry
   * Using clearColors, you can use 'fill()' to supply new colors before drawing each shape.
-   * If clearColors() is not used, the shapes will use their internal colors.
+   * If clearColors() is not used, the shapes will use their internal colors by ignoring 'fill()'.


If you use backticks around the function names, they'll get rendered like code:

by ignoring `fill()`.

...becomes:

by ignoring fill().

Okay...gonna do it.

perminder-17 · 2023-10-26T23:02:03Z

@davepagurek Does it looks good now?

perminder-17 · 2023-10-26T23:04:29Z

Any other change you want @davepagurek ?

davepagurek

Found two tiny things, and after that we're good to merge!

davepagurek · 2023-10-26T23:06:57Z

src/webgl/p5.Geometry.js

+  /**
+   * Removes the internal Colors of p5.Geometry.
+   * Using clearColors, you can use `fill()` to supply new colors before drawing each shape.
+   * If clearColors() is not used, the shapes will use their internal colors by ignoring `fill()`.


One more spot where we can use backticks:

Suggested change

* If clearColors() is not used, the shapes will use their internal colors by ignoring `fill()`.

* If `clearColors()` is not used, the shapes will use their internal colors by ignoring `fill()`.

davepagurek · 2023-10-26T23:07:19Z

src/webgl/p5.Geometry.js

  }
-
+  /**
+   * Removes the internal Colors of p5.Geometry.


oh also, we have an extra capital in here:

Suggested change

* Removes the internal Colors of p5.Geometry.

* Removes the internal colors of p5.Geometry.

Ok gonna do ASAP, wait

Btw nice observation 😅

davepagurek

Everything looks good to go now, thanks! 😁

perminder-17 · 2023-10-26T23:16:17Z

Sorry @davepagurek for these all mistakes. And thankyou so much for the constant support.

davepagurek · 2023-10-26T23:18:54Z

No worries! Don't think of it so much as finding mistakes, rather it's just always helpful having an editor/reviewer to catch things that slip by. Hopefully it relieves some of the pressure knowing you don't have to get everything right yourself the first time because someone else is also helping to check!

perminder-17 · 2023-10-26T23:20:50Z

No worries! Don't think of it so much as finding mistakes, rather it's just always helpful having an editor/reviewer to catch things that slip by. Hopefully it relieves some of the pressure knowing you don't have to get everything right yourself the first time because someone else is also helping to check!

Yes, Thankyou so much. Love to contribute more like this.

perminder-17 and others added 2 commits October 26, 2023 16:23

ClearColors method applied

d9370c1

Update p5.Geometry.js

716554f

davepagurek reviewed Oct 26, 2023

View reviewed changes

perminder-17 added 4 commits October 26, 2023 17:21

Removed method from 3D_primitives.js

e33224a

Update p5.Geometry.js

df35dc0

Update p5.Geometry.js

f79a6f2

Update p5.Geometry.js

8d04276

davepagurek reviewed Oct 26, 2023

View reviewed changes

perminder-17 added 3 commits October 27, 2023 04:08

Update p5.Geometry.js

bf98913

Update p5.Geometry.js

341d692

Update p5.Geometry.js

0791f46

davepagurek reviewed Oct 26, 2023

View reviewed changes

Update p5.Geometry.js

c997c45

davepagurek reviewed Oct 26, 2023

View reviewed changes

Update p5.Geometry.js

92c90d1

davepagurek approved these changes Oct 26, 2023

View reviewed changes

davepagurek merged commit 611941a into processing:main Oct 26, 2023

perminder-17 deleted the clearColor branch October 26, 2023 23:21

perminder-17 mentioned this pull request Jan 18, 2024

Defaults for buildGeometry #6722

Open

17 tasks

	/**
	* Removes the framebuffer and frees its resources.
	*
	* @method remove
	*
	* @example
	* <div>
	* <code>
	* let framebuffer;
	* function setup() {
	* createCanvas(100, 100, WEBGL);
	* }
	*
	* function draw() {
	* const useFramebuffer = (frameCount % 120) > 60;
	* if (useFramebuffer && !framebuffer) {
	* // Create a new framebuffer for us to use
	* framebuffer = createFramebuffer();
	* } else if (!useFramebuffer && framebuffer) {
	* // Free the old framebuffer's resources
	* framebuffer.remove();
	* framebuffer = undefined;
	* }
	*
	* background(255);
	* if (useFramebuffer) {
	* // Draw to the framebuffer
	* framebuffer.begin();
	* background(255);
	* rotateX(frameCount * 0.01);
	* rotateY(frameCount * 0.01);
	* fill(255, 0, 0);
	* box(30);
	* framebuffer.end();
	*
	* image(framebuffer, -width/2, -height/2);
	* }
	* }
	* </code>
	* </div>
	*
	* @alt
	* A rotating red cube blinks on and off every second.
	*/
	remove() {

	* If clearColors() is not used, the shapes will use their internal colors by ignoring `fill()`.
	* If `clearColors()` is not used, the shapes will use their internal colors by ignoring `fill()`.

	* Removes the internal Colors of p5.Geometry.
	* Removes the internal colors of p5.Geometry.

Uh oh!

Comments

Conversation

perminder-17 commented Oct 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Uh oh!

davepagurek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

perminder-17 commented Oct 26, 2023

Uh oh!

davepagurek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

perminder-17 commented Oct 26, 2023

Uh oh!

perminder-17 commented Oct 26, 2023

Uh oh!

davepagurek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davepagurek left a comment

Choose a reason for hiding this comment

Uh oh!

perminder-17 commented Oct 26, 2023

Uh oh!

davepagurek commented Oct 26, 2023

Uh oh!

perminder-17 commented Oct 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

perminder-17 commented Oct 26, 2023 •

edited

Loading