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

Added Circle #3419

Closed
wants to merge 7 commits into from
Closed

Added Circle #3419

wants to merge 7 commits into from

Conversation

killerrin
Copy link

I've noticed that MonoGame (and by extension XNA) doesn't have a base Circle class. On the 3D side we have a BoundingBox, BoundingSphere and even a BoundingFrustrum and on the 2D Side we only have a Rectangle.

I've always found this odd with XNA considering that Circles play an integral role in collision detection within the 2D Space. Not only are they fast to compute, but they can easily handle common situations in games such as rotated sprites. Since MonoGame is an opened source project, I figured I would see if I could get this basic addition added to the MonoGame framework.

Within this pull request I have written the Circle and Tests pertaining to it. If there are any problems let me know and I'm willing to solve them because I think that a fully featured Circle would be an extremely useful addition to MonoGame

@tomspilman
Copy link
Member

I agree it is an interesting missing primitive and this is a high quality PR with unit tests and all.

@KonajuGames - What do you think?

@@ -0,0 +1,509 @@
// MIT License - Copyright (C) The Mono.Xna Team
// This file is subject to the terms and conditions defined in
// file 'LICENSE.txt', which is part of this source code package.
Copy link
Member

Choose a reason for hiding this comment

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

That is the wrong license header for new contributions. It should be...

// MonoGame - Copyright (C) The MonoGame Team
// This file is subject to the terms and conditions defined in
// file 'LICENSE.txt', which is part of this source code package.

@Chaosus
Copy link
Contributor

Chaosus commented Jan 17, 2015

@killerrin 👍 I would like to see this and Triangle too in future. Note - you could add an example or describe your new class here - https://github.com/mono/MonoGame/blob/develop/Documentation/improvements.md
Than it will displayed here - http://www.monogame.net/documentation/?page=New_Features

@killerrin
Copy link
Author

@tomspilman I went ahead and fixed the license headers on all the files

@Shqrdx Good idea, I'll write up a small example documentation for it... Should I include it under "Math" or should I create a new header titled "Framework" and include it under that?

@Chaosus
Copy link
Contributor

Chaosus commented Jan 17, 2015

@killerrin Hmm.. I think "Math" should be renamed to "Math and geometry routines" than it will be ready to accept things like this..

@killerrin
Copy link
Author

Alright, I just pushed the first draft of the documentation. Let me know what I can fix up, add or remove

Removed Redundant Information
@nkast
Copy link
Contributor

nkast commented Jan 18, 2015

@Shqrdx A triangle can also be in 3D space, I use a struct like that mostly for preprocessing a mesh.
So, are we having a Triangle/Triangle3D or Triangle2D/Triangle3D?
Even circle could be defined in 3D space (Position,Radius,Normal), just never had a use for it.

@Chaosus
Copy link
Contributor

Chaosus commented Jan 18, 2015

@nkast The Triangle could be for both 2D and 3D (in one class). Of course triangle intended to be 3D first and support all kind of intersecting. Circle could be in 3D but maybe it should called BoundingCircle than ? Because BoundingSphere already exist.

@Viperfourfive
Copy link

For 2D spaces, isn't using pixel collision methods just as fast and more accurate anyways?

@killerrin
Copy link
Author

@Viperfourfive Not if you do it every frame. Thats why you use a Rectangle or a Circle before hand because it allows you to filter when its necessary to do the intensive task. The benefit of a Circle though is in dealing with rotated sprites, a very common case in 2D. If a Sprite will rotate, it is much easier to create your circle once and just modify its center position than having to create a Rectangle that is either way to big or having to recreate it every time you rotate the sprite to keep your sprite inside the rectangle.

@Shqrdx The logic behind calling it Circle instead of say BoundingCircle is mainly to keep it aligned name-wise with Rectangle. Really it could be called anything, but I feel that by keeping a similar style of name it just makes it easier for newcomers to use.

@Chaosus
Copy link
Contributor

Chaosus commented Jan 18, 2015

@killerrin Circle is fine, i said about Circle for 3D to @nkast

@killerrin
Copy link
Author

@Shqrdx Ah my bad

@mgarstenauer
Copy link
Contributor

At the current point I am against adding a new 2D primitive for the following reasons:

  • Personally, I don't think we need it in the core API. Circles can be handled in an external library as well, whereas Rectangle is needed and fulfills other tasks in XNA/MonoGame.
  • Rectangle works for basic 2D collision detection. For everything more complex, I don't think adding a Circle is the right approach. (For advanced collision detection we should look to external libraries such as Box2D.)
  • It breaks compatibility with XNA. That's not a bad thing -- just something to consider.
  • Before adding a new primitive, someone needs to present the "big picture". For example, what if other primitives such as Triangle2D, Capsule2D, etc. are needed. Does each primitive need to implement Intersects(Circle), Intersects(Rectangle), Intersects(Triangle2D), ... this would lead to an explosion of method overloads. As someone who has written a complete 3D collision detection library, I can say that this is not the right approach. I'd like to know the complete API first.
  • Once the API is public, it can't be changed ... without upsetting some users. ;-)
  • Does MonoGame have a policy for handling/approving new API features? I am just curious.

But of course, I leave the decision up to the MonoGame team. I just wanted to voice my opinion -- I certainly don't want to discourage anyone from working on new features.

@Viperfourfive
Copy link

@killerrin

I'm not sure how feasible, but if you looking for greater accuracy with 2D collisions and I'll be honest, I'm very new to Monogame, and what overhead is created with using pixel collisions. But, I envision something like adding a 2D library that utilizes Bounding Boxes, until a collision was detected; then handing the work over to a pixel collision method for the remainder (or until the bounding boxes were no longer intersected).

I've been playing around with pixel collisions the last few days and really like the interaction. Then again, I only have 4 sprites being rendered @ 600x800.

@killerrin
Copy link
Author

@mgarstenauer

Rectangle works for basic 2D collision detection. For everything more complex, I don't think adding > a Circle is the right approach. (For advanced collision detection we should look to external
libraries such as Box2D.)

When it comes to 2D Collisions, its mainly a two pronged street. On one corner you have Rectangles as the number 1 in usage, although on the other hand you have Circles which are a close second in usage and fill a secondary need which is getting accurate, low cost collisions on a rounded or constantly rotated object.

Now on the Triangle front, I'm not all that sure how much one would be used, but I feel that at the bare minimum MonoGame should have at least the top two primitive bounds/structures (Rectangles and Circles) built right into the framework, if only to fill most the use cases a developer would need. We offer three+ different and popular ways of checking a collision in 3D but only offer half of the equation in 2D.

It breaks compatibility with XNA. That's not a bad thing -- just something to consider.

If the compatibility reason is big enough, we could just change the namespace to Microsoft.XNA.Framework.Extensions or MonoGame.Framework.

Although just to play devils advocate about compatibility, sooner or later we will have to begin breaking compatibility as we improve MonoGame to stay up to date or with the times. MonoGame can't stay XNA 4.0 forever, what about 1, 3, 5 years into the future. Yes we should strive to keep it whenever possible, but in this case I think the benefits of having a native Circle outweigh the detriments.

Before adding a new primitive, someone needs to present the "big picture". For example, what if
other primitives such as Triangle2D, Capsule2D, etc. are needed. Does each primitive need to
implement Intersects(Circle), Intersects(Rectangle), Intersects(Triangle2D), ... this would lead to an > explosion of method overloads. As someone who has written a complete 3D collision detection
library, I can say that this is not the right approach. I'd like to know the complete API first.

Just to counter, this is pretty much exactly what gets done on MonoGame's 3D Bounds. For a couple of them the actual collision for each type is written only once then within the methods for the other classes they just call where the method is actually written.

@tomspilman
Copy link
Member

So my big concern here is deciding when to stop. Triangle? Ellipsoid? Polygon? Line? I can come up with dozens of additional primitives we could add. Does that mean we add them?

I think @mgarstenauer has a point here...

Rectangle is needed and fulfills other tasks in XNA/MonoGame

Mostly all the other primitives exist because XNA needed them to fulfill its own API. That said you could argue BoundingFrustum, BoundingBox, and BoundingSphere are not required parts of XNA.

Does MonoGame have a policy for handling/approving new API features? I am just curious.

@mgarstenauer - We don't have any official policy other than we approve things on a case by case basis.

I normally would have instantly rejected this PR, but because it included docs and unit tests it made me think twice.

I am still waiting to see what @KonajuGames thinks.

@KonajuGames
Copy link
Contributor

It can be a slippery slope. Personally I think MonoGame is not a collision
detection library, and that is best handled by more dedicated libraries
such as Farseer and Bepu. That said, Circle is a simple primitive and is
often used in larger games for 2D culling purposes. I think a Circle3D
(more correctly a Disc) is going too far into dedicated collision library
territory.

The axis-aligned Rectangle that came from XNA is used for SpriteBatch for
rendering. BoundingBox, BoundingSphere and BoundingFrustum are used for
draw culling.

Maintaining compatibility with XNA is primarily for moving from XNA to
MonoGame, not back the other way. Compatibility should not exclude us from
adding features, but then that's not an open invitation to add any feature.

I think a Circle type rides the boundary. Because it is a type so commonly
used in 2D game development, and this PR is well written, I would be in
favour of adding it. I believe anything beyond this belongs in a dedicated
collision library though.

/// <returns><c>true</c> if the provided coordinates lie inside this <see cref="Circle"/>; <c>false</c> otherwise.</returns>
public bool Contains(float x, float y)
{
return ((new Vector2(x, y) - Center).Length() <= Radius);
Copy link
Contributor

Choose a reason for hiding this comment

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

Length() does a square root internally, which is inefficient. It is more common to do something like

return ((new Vector2(x, y) - Center).LengthSquared() <= Radius * Radius);

@tomspilman
Copy link
Member

@dellis1972 - Thoughts?

1 similar comment
@tomspilman
Copy link
Member

@dellis1972 - Thoughts?

@dellis1972
Copy link
Contributor

I agree with @KonajuGames on this one.

@prollin
Copy link
Contributor

prollin commented Jan 26, 2015

To be mathematically correct what is defined here is a disk, not a circle :)

Joke aside, i personally prefer the idea of MonoGame remaining a minimal gaming framework; everything that is missing can easily be added by the programmer (and there are many extensions available). XNA did a great job to keep the complexity to a minimum and I think MonoGame should follow the same path.

@KakCAT
Copy link
Contributor

KakCAT commented Jan 31, 2015

IMHO @killerrin is right in one thing: MonoGame, sooner or later, will have to expand beyond XNA. If I'm not wrong MonoGame main admins have said this several times too, and it's logical to be this way.

So, my question is... how will this be handled?

Some extensions will be mandatory to be made into MonoGame.Framework (i.e. Geometry Shader implementation) but some others you may want to add (I don't know, maybe a SSAO implementation) do not need to be in the Framework.

Maybe this would be a good point and place to discuss how this kind of things will be handled in the future expansions, maybe a separate extension project (which this Circle implementation could be the starting point) or everything still inside the Monogame.Framework, or just discarding the generic SSAO implementation to become part of MonoGame.

@Chaosus
Copy link
Contributor

Chaosus commented Jan 31, 2015

I hope that MonoGame will remove much of XNA legacy(namespace names for example) sooner or later and keep and maintain legacy as separate version. I also see no reason why not extend it for now. Circle is a common primitive in computer graphics and all serious engines and framework have it inside.

@prollin
Copy link
Contributor

prollin commented Jan 31, 2015

@KakCAT Having a separate repository for extensions would be best in my opinion. It could contains useful extensions methods, GameComponents and Pipeline extensions. Adding this in MonoGame itself is a slippery slope since for each concept there are many way to implement them.

@Chaosus
Copy link
Contributor

Chaosus commented Feb 8, 2015

@tomspilman So, how about implementing Circle ? I wanted try it in my projects...

@killerrin
Copy link
Author

@tomspilman So is there a verdict on if Circle would be fine to merge? Is there anything I need to do?

@tomspilman
Copy link
Member

I'm still mulling it over here.

On one hand this type is basically completely independent of MonoGame and could live in a 3rd party library. It isn't used internal to MonoGame and it might encourage some to submit more collision primitives which we're not looking for.

On the other hand I hate saying no to stuff all the time and this is high quality work.

@tomspilman
Copy link
Member

We're not adding primitives like this to MonoGame. It is better handled by extensions.

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