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

Bounds2 should be used instead of the duck-typed coords #49

Closed
jonathanolson opened this issue Dec 9, 2014 · 4 comments
Closed

Bounds2 should be used instead of the duck-typed coords #49

jonathanolson opened this issue Dec 9, 2014 · 4 comments
Assignees

Comments

@jonathanolson
Copy link
Contributor

Multiple places have code like:

var coords = {};
coords.leftX = ...
coords.topY = ...
coords.rightX = ...
coords.bottomY = ...

I'd recommend instead using Bounds2, which is made for that purpose:

var bounds = new Bounds2( leftX, topY, rightX, bottomY );

which can then be accessed with:

bounds.left
bounds.right
bounds.top
bounds.bottom

For code review #29

@AshrafSharf
Copy link

The object literal is used because the Bounds2 doesn't have a setter for left or right so it forces us to create a new Bounds2 instance every time (for 1000s of particles) which degrades performance due to GC. However I have renamed "leftX" to "left". and made changes to top,right and bottom along the same lines which is consistent with the naming convention adopted by other scenery classes.
@jonathanolson Let me know if it is fine

@jonathanolson
Copy link
Contributor Author

'left' and 'right' are aliases for 'minX' and 'maxX', would things be cleaner with using setMinX() and setMaxX()?

@jbphet jbphet assigned AshrafSharf and unassigned jonathanolson Dec 17, 2014
@AshrafSharf
Copy link

Replaced duckType object properties with Bounds2 instance for Texture and Vertext coordinates.
used the approach suggested in the previous comment(setMinX)

@jbphet
Copy link
Contributor

jbphet commented Sep 22, 2015

This appears to have been fully addressed, closing.

@jbphet jbphet closed this as completed Sep 22, 2015
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

3 participants