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

Solves issue #6787 #6809

Merged
merged 12 commits into from
Mar 13, 2024
Merged

Solves issue #6787 #6809

merged 12 commits into from
Mar 13, 2024

Conversation

Garima3110
Copy link
Member

@Garima3110 Garima3110 commented Feb 13, 2024

Resolves #6787

Changes:
I have tried to implement the calculateBoundingBox() method for p5.Geometry objects,
This aims at simplifying tasks like collision detection and intersection testing. This enhancement also makes it easier to manipulate objects based on their spatial characteristics as it provides convenient access to essential properties such as min , max , size, and offset which are nothing but p5.Vector.

Sample usage for calculateBoundingBox() :

const myBox = buildGeometry(() => box(30, 40, 50));
const boundingBox = myBox.calculateBoundingBox();
console.log('Bounding Box:', boundingBox);

Here's a sample sketch to test how it works:
https://editor.p5js.org/Garima3110/sketches/BXhqwTIRe

I would love to have suggestions to this PR . Thankyou!

PR Checklist

Copy link
Contributor

@davepagurek davepagurek left a comment

Choose a reason for hiding this comment

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

The implementation looks great, thanks for taking this on!

Before calling it done, could we maybe add:

  • A unit test that verifies the output, so that we can tell if anything breaks it in the future
  • A doc comment above the method explaining to users how to use it, maybe with a code example?

@Garima3110
Copy link
Member Author

Garima3110 commented Feb 16, 2024

Before calling it done, could we maybe add:

  • A unit test that verifies the output, so that we can tell if anything breaks it in the future
  • A doc comment above the method explaining to users how to use it, maybe with a code example?

Yeah sure! I'll just work on these and get back to you soon...!

@Garima3110
Copy link
Member Author

image

Hey @davepagurek ! Just a bit doubtful why this error is coming, couldn't we use createVector() directly
Here's the screenshot of the specified lines in the error:

image

return this.boundingBoxCache; // Return cached result if available
}

let minVertex = createVector(
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, right, createVector called like this assumes p5 is in global mode, but it needs to also work in instance mode (where you have a p5 object, and all the previously global functions are referenced via p5.createVector.) Rather than trying to detect which mode we're in, maybe instead we can use the constructor syntax, which should work regardless of mode:

let minVertex = new p5.Vector(Number.MAX_VALUE, Number.MAX_VALUE, Number.MAX_VALUE);

@davepagurek
Copy link
Contributor

Nice work on the test! I think once we switch the vector syntax then that'll work. After that, the next thing to add would be a doc comment above the method you added with a brief description and brief example.


for (let i = 0; i < this.vertices.length; i++) {
let vertex = this.vertices[i];
minVertex.x = min(minVertex.x, vertex.x);
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see tests are still failing, min and max are also p5 globals and won't be defined in instance mode. I think Math.min and Math.max will fix it though

@Garima3110
Copy link
Member Author

Garima3110 commented Feb 27, 2024

Coming to adding an example code to the inline docs showing the usage of calculateBoundingBox() ,
would this example go along ?!
(The one which I used in the sketch : https://editor.p5js.org/Garima3110/sketches/BXhqwTIRe)

let particles;
let button;

function setup() {
  createCanvas(500, 500, WEBGL);
  button = createButton('New');
  button.mousePressed(makeParticles);
  makeParticles();
}

function makeParticles() {
  if (particles) freeGeometry(particles);

  particles = buildGeometry(() => {
    for (let i = 0; i < 60; i++) {
      push();
      translate(
        randomGaussian(0, 200),
        randomGaussian(0, 100),
        randomGaussian(0, 150)
      );
      sphere(10);
      pop();
    }
  });
  const boundingBox = particles.calculateBoundingBox();
  console.log('Bounding Box:', boundingBox);
}

function draw() {
  background(255);
  noStroke();
  lights();
  orbitControl();
  model(particles);
}

@davepagurek
Copy link
Contributor

I think that's a good start! For the examples in the reference, could we maybe make the canvas size 100x100 to fit better next to the code, and maybe use createP() to make a text box to write the results into rather than logging it so that it can be more visible?

Copy link
Contributor

@davepagurek davepagurek left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! I think the last suggestion I have would be to format the JSON in a slightly more readable way. If you add some parameters to JSON.stringify to add , null, 2 after the object being stringified, it will prett-print the output with 2-space tabs. Then, for newlines in the source code to be rendered as newlines in the browser, we can use a pre tag ("preformatted") instead of p.

* button = createButton('New');
* button.mousePressed(makeParticles);
*
* resultParagraph = createP('').style('width', '180px' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* resultParagraph = createP('').style('width', '180px' );
* resultParagraph = createElement('pre').style('width', '180px' );

* });
*
* const boundingBox = particles.calculateBoundingBox();
* resultParagraph.html('Bounding Box: \n' + JSON.stringify(boundingBox));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* resultParagraph.html('Bounding Box: \n' + JSON.stringify(boundingBox));
* resultParagraph.html('Bounding Box: \n' + JSON.stringify(boundingBox, null, 2));

Copy link
Contributor

@davepagurek davepagurek left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes, we're good to go! Looking forward to using this in the next release 🙂

@davepagurek davepagurek merged commit 7bf795a into processing:main Mar 13, 2024
2 checks passed
@Garima3110 Garima3110 deleted the boundingBox branch May 31, 2024 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement bounding box calculation for p5.Geometry objects
2 participants