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

Created p5.Vector versions of all functions with vector examples to boot #28

Merged
merged 6 commits into from
Jun 22, 2020

Conversation

wisehackermonkey
Copy link
Contributor

@wisehackermonkey wisehackermonkey commented Jun 15, 2020

Implemented P5.js Vector version of all functions

I love this library and wanted to give back!
A pattern i've been using in games has been to
wrap Collide2d functions as vector versions. And I thought it might be usefull to others, so I'd add it to the actual library so others can use the vector versions too!

Vector format

Example:
collidePointCircle(...)
becomes
collidePointCircleVector(....)

Here is a live example of using p5.Vector format for p5.collide2d.js

Live demo

example

collideRectCircle(200,200,100,150,mouseX,mouseY,100);

add support for vectors

let mouse    = createVector(mouseX,mouseY);
let circle       = createVector(200,200);
let diameter = 100;
collidePointCircleVector(mouse, circle, diameter)

Improvements/fixes:

Docs

  • added added p5.vector examples for all functions
  • examples now changes color when a hit is detected
  • added description of how to use build script to generate *.min.js version

Node build script

  • added build script that generates *.min.js version of the *.js file

Misc

  • Added license file

Note:

I Tested manually each example to verify that it worked

@bmoren
Copy link
Owner

bmoren commented Jun 15, 2020

this is also great! – will have a deeper look in a little bit. Thank you!

@bmoren
Copy link
Owner

bmoren commented Jun 16, 2020

This generally looks really great. I'm inclined to merge it but I have one thought before going forward and just wanted to get your input / thoughts.

do you think it would make more sense to keep it as it is with one function / collision and adding the vector word to intake vectors.. eg collidePointCircleVector(mouse, circle, diameter)

or...

add a function with something like collisionMode(VECTOR)

that could be instantiated once at the setup and it would switch all the functions into vector mode and all of the arguments would be adjusted to take in vectors instead. I totally recognize that this could lead to a lot of confusion since the x/y vs vector arguments would not line up so it wouldnt be easy to toggle/switch modes without completely re-writing the code which is not the normal behavior of how something like ellipseMode(CENTER) vs ellipseMode(CORNER) works...

I guess all this to ask, can you imagine a way that this functionality could be implemented as a mode vs an entire new set of functions?

again I'm not opposed to the current approach, just trying to think about all the options!

let me know what you think!
-Ben

@wisehackermonkey
Copy link
Contributor Author

wisehackermonkey commented Jun 17, 2020

thanks ben for the feedback!

i totally agree, when i started the pullrequest I was defaulting to

collidePointCircleVector(....)

for a lack of a better convention.

i really like the collisionMode(VECTOR)

Other alternatives

Option 1) adding vector support as a optional argument like

 collidePointCircleVector(mouse, circle, diameter)

becomes

                                            vector_mode
 collidePointCircle(mouse, circle, diameter,<true/false>)

Option 2)

 collidePointCircle(mouse, circle, diameter,"VECTOR")

pros

  • sticks to p5.js's style of function options reather than settings

cons

  • duplicate ","VECTOR") for every function

Option 3) or could be passed as a options object like some libraries do.

let options = {"vector":true}
 collidePointCircle(mouse, circle, diameter,options)

pros

  • allows for more settings to be added later

cons

  • duplicate ",options)" for every function
  • unused settings object A.T.M.

collisionMode(VECTOR)

To add to your idea about
collisionMode(VECTOR)

as you stated, a problem with arguments could get confusing when switching between vector/non-vector

Example of confused code

//setup
collisionMode(VECTOR)
---------------------------------
//draw
 collidePointCircle(mouse, circle, diameter,options)
//... later in draw
 collidePointCircle(mouseX, mouseY, 100,100, 50)
//oops now its mixed up

possible solution
not sure how to implement this but do something similar to push and pop with graphics calls, or with rectMode(CENTER)

//setup
collisionMode(VECTOR)
---------------------------------
//draw
collidePointCircle(mouse, circle, diameter,options)

collision_push()

     //isolated from other collision calls
     collidePointCircle(mouseX, mouseY, 100,100, 50)

collision_pop()

Pro/cons

advantage

  • flexible: allows for mixing types of non-vector and vector version in code
  • fits with convection already established with push/pop
  • lends nicely to expansion to collisionMode(CORNER), or collisionMode(VECTOR, CORNER) and mesh nicely with this requested feature issue RectMode Center #27

cons

  • a little verbose

alt names for 'collision_push"

  • push("VECTOR")
  • pushCollision()
  • pushCollide()
  • CollidePush()
  • collide_push()

thanks for the feedback, im curious what your thoughts are on the different code style options.

look forward to hearing what you have to say.

-oran

@wisehackermonkey wisehackermonkey mentioned this pull request Jun 17, 2020
@bmoren
Copy link
Owner

bmoren commented Jun 20, 2020

some great Ideas here!

I'm going to go ahead and put all of my weight behind the collisionMode(VECTOR) && the original idea you presented as the 2 viable options, I don't think the arguments (option 2&3) are really a clear solution and add too much verboseness without enough benefits.

one thing I was thinking with your push/pop idea is that we could likely just show examples where re-setting the mode would effectively have the same effect w/o having to implement any other changes to the code, or state saving:

eg:

function setup(){

collisionMode(VECTOR)

}


function draw(){

//uses vector inputs initialized in the setup
collidePointCircle(mouse, circle, diameter,options)

//change back to non vector mode, uses x/y value inputs
collisionMode(XYZ)
collidePointCircle(mouseX, mouseY, 100,100, 50)

collisionMode(VECTOR) // go back to vector mode.
collidePointCircle(mouse2, circle2, diameter,options)
}

This solves some if the issues between interchange of the 2 ideas, but it still creates 2 parallel universes for the arguments which is maybe problematic, and is easily avoided by implementing a whole additional set of vector specific functions.

Another question that is maybe important is if the additional check that will be required to determine the mode will cost in performance, my suspicion is not, but it's worth a thought.

This would dovetail with the corner vs center issue #27 as well, but there are other issues there and the reason I did not implement that from the start is that the defaults in p5.js are mixed (corner vs center) so you'd have to do quite a bit of checking on states of things to do it, or force it, which seems like it could get things out of sync of drawing vs. collision.

I think it might help to get some other opinions on this, I'm going to post a link to this discussion into the Coding Train Discord server to see if any folks over there have thoughts or ideas about this.

@bmoren
Copy link
Owner

bmoren commented Jun 20, 2020

following up from the coding train chat, it seems like most folks are most interested in the original implementation you offered so far for it's clarity, ease of use, and ability to co mingle both vector and x/y/z easily.

@wisehackermonkey
Copy link
Contributor Author

wisehackermonkey commented Jun 22, 2020

Thanks for the input.

Although

collisionMode(XYZ)

I cant deny that there is something appealing about this solution, I feel like as some of the coding train members said
(I hope they are ok with me reposting this here worried face emoji)

Seems to me adding separate functions would be the simplest option and would be less confusing. Also it would allow to use both types interchangeably without having to switch modes. [....]

By "Salami"

I do agree is the least confusing option.

Now that I think of it, it will be very infrequent that i want to switch between vector and xyz modes mid script.

My only hesitance with my solution is it feels too verbose.

What are your thoughts on these shorter names:

CollideRectCircleVect()

Or

CollideRectCircleV()

thoughts on collisionMode(VECTOR)

I think the
collisionMode(VECTOR)
Solution feels most elegant when used to switch between corner/ center modes. collisionMode(VECTOR) might be a good way to communicate that the functions return a vector version of the hit location. Like what happens with a few of the functions that don't return a boolean.

Any lingering thoughts?

@bmoren
Copy link
Owner

bmoren commented Jun 22, 2020

I think generally you and coding train are right that the function is simplest. As for it's verbose direction, I don't really have an issue with it, clarity (especially in p5js and for beginners or folks who don't program all the time) is more important in my opinion. You additional thought on changing the type of return is interesting and perhaps something worth considering or looking into as we move into the future.

if you're good with this direction I'll go ahead and merge the PR and we can verify that everything worked out with it and the new build process you implemented!

again, thank you for all of your work on this!

@bmoren bmoren merged commit 5aabeff into bmoren:master Jun 22, 2020
@wisehackermonkey
Copy link
Contributor Author

@bmoren thanks! glad to help.

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.

None yet

2 participants