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

Ellipse random() is incorrect #522

Closed
budda opened this issue Apr 9, 2018 · 9 comments
Closed

Ellipse random() is incorrect #522

budda opened this issue Apr 9, 2018 · 9 comments

Comments

@budda
Copy link
Contributor

budda commented Apr 9, 2018

This Issue is about

  • A bug in the API:
    • Phaser version(s): 2.10.3

    • What should happen: points returned up to the circumference of the defined ellipse
      screen shot 2018-04-09 at 21 44 34

    • What happens instead: points returned in the centre with a large padding around ellipse edge.
      screen shot 2018-04-09 at 21 16 50

budda added a commit to budda/phaser that referenced this issue Apr 9, 2018
random() didn't return a point anywhere in the Ellipse area.

Full details and screenshots (before & after) can be found on phaserjs/phaser-ce#522 assuming the issue is the same in Phaser 3.
@budda
Copy link
Contributor Author

budda commented Apr 9, 2018

Test code - https://codepen.io/budda/pen/jzdboZ

@budda
Copy link
Contributor Author

budda commented Apr 9, 2018

Comment from @photonstorm phaserjs/phaser#3534 (comment)

Actually, I think that codepen is showing a different issue. The ellipse has been defined with a size of 200x300, but the graphics.drawEllipse has drawn it at 400x600.

@samme
Copy link
Collaborator

samme commented Apr 10, 2018

The confusion is because the arguments are different:

Phaser.Ellipse(left, top, width, height)

graphics.drawEllipse(centerX, centerY, halfWidth, halfHeight)

Nonetheless the points returned by Ellipse#random seem to be placed incorrectly. They are centered on (x, y), but they should be centered on (x + width / 2, y + y / 2). https://codepen.io/samme/pen/ZxwMMN.

Phaser.Utils.Debug#geom seems to have the same problem.

samme added a commit that referenced this issue Apr 10, 2018
This reverts commit a91c473, reversing
changes made to 5144cec. (#522, #523)
@samme samme added this to the 2.10.4 milestone Apr 10, 2018
@budda
Copy link
Contributor Author

budda commented Apr 10, 2018

Okay, new test random code sorts out the random points - https://codepen.io/budda/pen/KoJOXB

I'm not familiar with the geom debug stuff, I presume it's something wrong on the line

this.context.ellipse(object.x - this.game.camera.x, object.y - this.game.camera.y, object.width/2, object.height/2, 0,2 * Math.PI,false);

But i'm not sure what this is doing with the camera stuff?

@budda
Copy link
Contributor Author

budda commented Apr 10, 2018

Also, inline documentation needs correcting as its not halfWidth & halfHeight that should be passed in.

Current, incorrect in Graphics.js ...

/**
 * Draws an ellipse.
 *
 * @method Phaser.Graphics#drawEllipse
 * @param x {Number} The X coordinate of the center of the ellipse
 * @param y {Number} The Y coordinate of the center of the ellipse
 * @param width {Number} The half width of the ellipse
 * @param height {Number} The half height of the ellipse
 * @return {Graphics}
 */
Phaser.Graphics.prototype.drawEllipse 

Because I believe, the width & height is then divided in half on our behalf in the function:

this.drawShape(new Phaser.Ellipse(x-(width/2), y-(height/2), width, height));

@samme
Copy link
Collaborator

samme commented Apr 10, 2018

@param width {Number} The half width of the ellipse
@param height {Number} The half height of the ellipse

The descriptions are correct. They should probably be renamed as halfWidth and halfHeight.

@budda
Copy link
Contributor Author

budda commented Apr 10, 2018

But that would suggest that the final X & Y coord is a quarter as it divides the width again inside the method?

My head is mangled following this Ellipse lark now!

@samme
Copy link
Collaborator

samme commented Apr 10, 2018

Divides the width again inside which method?

@budda
Copy link
Contributor Author

budda commented Apr 10, 2018

Phaser.Graphics.prototype.drawEllipse

@samme samme changed the title Ellipse random() doesn't return a point within the whole available area Ellipse random() is incorrect Apr 21, 2018
@samme samme closed this as completed in 22f0c58 Apr 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants