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 Wrap methods to MathHelper. #1550

Closed
wants to merge 5 commits into from
Closed

Added Wrap methods to MathHelper. #1550

wants to merge 5 commits into from

Conversation

Chaosus
Copy link
Contributor

@Chaosus Chaosus commented Mar 16, 2013

These methods are not exist in XNA but I found them very useful in many cases. Also im introduced them to SharpDX and this is fine. WrapAngle is too limited.

eg value = Wrap(value+1,0,10); // the value will constantly cycled between min and max.

These methods are very useful in many cases. WrapAngle is too limited for these cases.
@mgbot
Copy link
Member

mgbot commented Mar 16, 2013

Can one of the admins verify this patch?

1 similar comment
@mgbot
Copy link
Member

mgbot commented Mar 16, 2013

Can one of the admins verify this patch?

@danzel
Copy link
Contributor

danzel commented Mar 16, 2013

Pretty certain you could use modulus (%) to make the integer version much smaller and faster.
Something similar to the float version maybe.

@Chaosus
Copy link
Contributor Author

Chaosus commented Mar 16, 2013

The modulo operator cannot be used with float and it also generate error-prone code. Anyways, you could modify this code if my changes will accepted, right ?

@totallyeviljake
Copy link
Contributor

Now that we have a unit test framework in place, can you please write a simple unit test for your addition? Thanks.

@tomspilman
Copy link
Member

Now that we have a unit test framework in place

The unit test framework is still a mess... lots of broken tests, crazy directory structure, and no documentation of any kind as to how to submit good tests. Don't worry about this for now.

@tomspilman
Copy link
Member

@slygamer @dellis1972

How do you guys feel about accepting random new methods into MathHelper?

@dellis1972
Copy link
Contributor

I'm ok with it. Although It might be an ide to flag these somehow so it is
known they are not part of the XNA API. Is there an Attribute we can add
that will do that?

On 17 March 2013 22:45, Tom Spilman notifications@github.com wrote:

@slygamer https://github.com/slygamer @dellis1972https://github.com/dellis1972

How do you guys feel about accepting random new methods into MathHelper?


Reply to this email directly or view it on GitHubhttps://github.com//pull/1550#issuecomment-15032777
.

@tomspilman
Copy link
Member

ide to flag these somehow so it is known
they are not part of the XNA API

I don't think an attribute would be possible or would even provide what we would want here.

The best thing we could do is document it I suspect.

Truth is this is the sort of thing that has no reason being in XNA/MonoGame. It is not platform specific in any way... it really belongs in a separate library that can be reused on any platform. Same with things like Vector3 and Matrix if we're being honest.

Still that is a bigger issue we cannot solve right now... and it is not the point of this PR.

@totallyeviljake
Copy link
Contributor

I've submitted a starter unit test for MathHelper:

#1562

if it gets merged in then please add unit test methods for your new methods, if your PR gets pulled in.

@Chaosus
Copy link
Contributor Author

Chaosus commented Mar 18, 2013

"if it gets merged in then please add unit test methods for your new methods, if your PR gets pulled in."

Ok

I just didnt notice about conservative idea of this project - preserve all as XNA does - without fresh additions

@tomspilman
Copy link
Member

I just didnt notice about conservative idea of this project

  • preserve all as XNA does - without fresh additions

That is not accurate.

We in the past have only supported what was in XNA. We are now starting the process of adding things beyond XNA. This is why we are discussing your submission and not just rejecting it.

@Chaosus
Copy link
Contributor Author

Chaosus commented Mar 18, 2013

Ah. Glad to hear this than. :) Im appreciate

@totallyeviljake
Copy link
Contributor

One thing we could try is a partial class for MathHelper extensions. I know you don't like partial classes @tomspilman but I think that would keep the core caps pure XNA and let us support extensions. Then we could do partial unit test classes that would also allow the implementor to create their own unit tests without having to disrupt the proven tests.

@KonajuGames
Copy link
Contributor

Documentation would indeed be the best place to call out APIs beyond the
original XNA API set. This is where the Monodoc documentation also would
be helpful (yet another we should do as this project gets more exposure and
a wider audience).

We need to make sure additional APIs are of value and not just become a
repository for all manner of methods. That said, I think these Wrap
methods are of value though the implementations could be more efficient.

int temp = value;
while (temp >= max)
{
temp -= r;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a huge performance problem. iterations like this are trouble since you can easily compute the perturbation to bring your value into the range. Can you replace this iteration with a calculation instead? For instance, Wrap(123434343324, 1, 2) would iterate forever and make us all quite upset with your code. When you write the unit tests for these methods, you will need to include boundary tests like that.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the int version, that would be better replaced with something like

return ((value - min) % (max - min)) + min;

Note that is just off the top of my head, so it may contain bugs, but is
along the lines of how it should be written.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@slygamer Yeah, it contains bug - Wrap(-125, -100, 100); != 75

@Chaosus
Copy link
Contributor Author

Chaosus commented Mar 18, 2013

Im not really understand the solution from scratch. If I found it i post it right to my PR. If you could post it here - im implement it instantly.

@tomspilman
Copy link
Member

@totallyeviljake

One thing we could try is a partial class for MathHelper extensions

Can you make a static class a partial class too? I thought that was not allowed.

keep the core caps pure XNA and let us support extensions

That is not my point at all.

I'm not trying to keep it pure XNA... I am not looking to support extensions.

My point is maybe the community as a whole is better served by this type of code not being part of MonoGame at all. As part of MonoGame this code cannot be easily used in WP7 or Xbox projects. As a separate library it could be used on any project including MonoGame or even Unity.

Then we could do partial unit test classes that would also allow
the implementor to create their own unit tests without having to
disrupt the proven tests.

I'm not sure what the point would be.

You can implement a unit test anywhere... there is no rule of one class can only have one unit test class.

Even then I see no issue having a singular MathHelperTest class if we wanted it either.

@Chaosus
Copy link
Contributor Author

Chaosus commented Mar 18, 2013

I posted fix which erase performance problem - thanks for slygamer algorithm.

@KonajuGames
Copy link
Contributor

@tomspilman , would you think perhaps a MonoGame.Extras assembly or similar?

@totallyeviljake
Copy link
Contributor

I think these extensions could go into their own MonoGame.Extensions library. Then this wrap method, along with clamp and such, could look like:

public static float Wrap(this float me, float min, float max) {
// do the wrap algorithm
}

Then we only need to write

float foo;
foo = 44545f;
foo = foo.Wrap(100, 200);

No extra intermediary class required.

@totallyeviljake
Copy link
Contributor

@tomspilman you can have static methods in partial classes:

http://msdn.microsoft.com/en-us/library/wa80x488.aspx

Scroll down to the bottom of the page, find:

•Partial methods can have static and unsafe modifiers.

While this refers to methods, it also means you can have static methods in a partial class. The compiler just combines the partial class definitions into a final class during the final stage of the library compilation. Partial is not a runtime consideration, just a compile time directive.

@Chaosus
Copy link
Contributor Author

Chaosus commented Mar 19, 2013

Ok. For these extra features I better create my own Monogame based library..

@dellis1972
Copy link
Contributor

@slygamer is this something that we can put in the MonoGame-Support repo?

On 19 March 2013 18:08, Shqrdx notifications@github.com wrote:

Ok. For these extra features I better create my own Monogame based
library.. Please answer on my issue here - #1565#1565


Reply to this email directly or view it on GitHubhttps://github.com//pull/1550#issuecomment-15132571
.

@nkast
Copy link
Contributor

nkast commented Mar 19, 2013

I hate the idea to have more things into the Microsoft.Xna namespace than aren't already into the official api.
This will break compatibility for those that develop on top of monoGame and try to later move the code to XNA.

This belongs to a separate library. I don't see the need to have a new library/extensions, and I wouldn't use it if I couldn't use it on pure XNA as well. There are already many complete libraries for maths, particles, rendering, etc that run on top of XNA/monoGame.

@KonajuGames
Copy link
Contributor

We can definitely add it to MonoGame-Support.

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

8 participants