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

Shape interface #2178

Merged
merged 13 commits into from Sep 7, 2014
Merged

Shape interface #2178

merged 13 commits into from Sep 7, 2014

Conversation

mrgame64
Copy link
Contributor

Unification of gdx.math shape classes under the Shape interface

@mrgame64 mrgame64 changed the title Enchancements Shape interface Jul 29, 2014
@mrgame64
Copy link
Contributor Author

@MobiDevelop I must create new instances, as when the user requests a BoundingBox/Sphere/Vector3 and modifies it, upon next request the instance returned will be the same, thus containing modified values instead of the correct ones.
Also look at Polygon.getBoundingRectangle().

@MobiDevelop
Copy link
Member

Creating new instances each time these methods are called is going to set the GC on fire. You could have the user pass in an BoundingBox or Sphere (even though I still think it is weird to use 3D stuff for 2D shapes). That way it is up to the user to decide if they want to create new objects all the time.

What's special about Polygon#getBoundingRectangle?

@badlogic
Copy link
Member

I would obly merge if Shape is a tagging interface. getCenter would be ok,
everything else must go
On Jul 30, 2014 3:24 PM, "Justin Shapcott" notifications@github.com wrote:

Creating new instances each time these methods are called is going to set
the GC on fire. You could have the user pass in an BoundingBox or Sphere
(even though I still think it is weird to use 3D stuff for 2D shapes). That
way it is up to the user to decide if they want to create new objects all
the time.

What's special about Polygon#getBoundingRectangle?


Reply to this email directly or view it on GitHub
#2178 (comment).

@mrgame64
Copy link
Contributor Author

I can also have the class keep final instances of BoundingBox/Sphere/Vector3 and have their values update each time the Shape's getter methods are called. Of course, I could also let the user choose whether to pass their own objects to write values to.

As I said earlier, I chose 3D bounding shapes for compatibility, but it's not vital.

Polygon#getBoundingRectangle generates a new Rectangle every time it's called.

@mrgame64
Copy link
Contributor Author

@badlogic Sure, I can remove the other methods if they're not considered necessary.

@MobiDevelop
Copy link
Member

If you keep getCenter, avoid creating new objects each time. Your Polyline#getCenter creates a Polygon and number of vertices instances of Vector2, and a Vector3.

@MobiDevelop
Copy link
Member

Also, Polygon#getBoundingRectangle only creates a new Rectangle if bounds is null, which it only is the first time the method is called.

@mrgame64
Copy link
Contributor Author

Sure, will do.

@nooone
Copy link
Contributor

nooone commented Jul 30, 2014

I agree that a common Shape interface could be useful.

However I'm not even sure whether getCenter() is necessary or not. A simple tagging interface would be enough as well. If we keep getCenter() though, it should return a Vector2, not Vector3 imho.

@JesseTG
Copy link
Contributor

JesseTG commented Jul 30, 2014

Why not a method like getVertices() to make it easier to generate a Mesh or a Box2D shape or something just given a Shape?

@mrgame64
Copy link
Contributor Author

The following changes were made in commit 7fd147c :

  • Left only getCenter() in the Shape interface, removing the method implementations in Shape implementing classes
  • Made getCenter() return a Vector2
  • Optimized getCenter() by removing the initiation of several Vector2 objects in Polyline and Polygon
  • Removed the Polygon object from Polyline#getCenter and implemented area calculation directly
  • Created a "private Vector2 center" field in every class implementing Shape; the Vector2 is initiated only at the first call of getCenter(), otherwise its values are just updated

@nooone
Copy link
Contributor

nooone commented Jul 30, 2014

@JesseTG That would not make much sense, since you'd probably want to have a real BoxShape (or at least use setAsBox(...) which you could not do anymore in a convenient way when you only know the vertices, since you need the actual width/height).

Furthermore: Which vertices would Circle return?

@MobiDevelop
Copy link
Member

Like @nooone, I'm on the fence as to whether getCenter actually adds any value.

@mrgame64
Copy link
Contributor Author

@JesseTG I agree with @nooone on this. Sure, there are many methods (getArea, getPerimeter, getVertices) which could be implemented, but we shouldn't get carried away by the possibilities. The original purpose was just to have a tagging interface

@xoppa
Copy link
Member

xoppa commented Aug 2, 2014

Since it's a 2D shape, I would prefer naming it Shape2D.

@mrgame64
Copy link
Contributor Author

mrgame64 commented Aug 2, 2014

@xoppa Does this mean we can hope for a future Shape3D implementation?

@xoppa
Copy link
Member

xoppa commented Aug 2, 2014

Sure, we can always hope.

@MobiDevelop
Copy link
Member

@badlogic Can you make a decision here? I personally don't really see any value with the getCenter() method but you said it was OK, so you get to merge if you are OK with it as is.

@badlogic
Copy link
Member

Let's kill getCenter() (sorry, was on honeymoon) and make it a tagging interface called Shape2D.

@mrgame64
Copy link
Contributor Author

mrgame64 commented Sep 2, 2014

getCenter() killed.
I hope you had a nice honeymoon :)

@davebaol
Copy link
Member

davebaol commented Sep 2, 2014

Yeah congratulations, Mario. :)

@MobiDevelop
Copy link
Member

Might as well kill the private Vector2 center; in each Shape2D implementation since it will not be used. Also, we could clean up the imports.

@mrgame64
Copy link
Contributor Author

mrgame64 commented Sep 2, 2014

I think it should be good to go now.

@badlogic
Copy link
Member

badlogic commented Sep 7, 2014

Sorry, just go back, merged, thanks!

badlogic added a commit that referenced this pull request Sep 7, 2014
@badlogic badlogic merged commit b9ee5e6 into libgdx:master Sep 7, 2014
@nooone
Copy link
Contributor

nooone commented Sep 7, 2014

13 commits for a tagging interface, lol

@mrgame64
Copy link
Contributor Author

mrgame64 commented Sep 7, 2014

@nooone Yeah, stuff happens

@MobiDevelop
Copy link
Member

This would have been a good candidate for squashing commits.

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

7 participants