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

System.Math and OpenToolkit.MathHelper symmetry #897

Merged
merged 7 commits into from
Apr 16, 2019
Merged

System.Math and OpenToolkit.MathHelper symmetry #897

merged 7 commits into from
Apr 16, 2019

Conversation

frederikja163
Copy link
Member

@frederikja163 frederikja163 commented Apr 14, 2019

Purpose of this PR

Add symmetry between the System.Math and OpenToolkit.MathHelper.
For the issue discussing this PR see (#893)

Testing status

Tests haven't been written yet.

Comments

This is to help users (like myself) not having to look through two libraries as none of them are complete with all the features you need. This way you would only ever have to use MathHelper to do all your math stuff

Cos, Sin and Tan functions are added, they all both include the normal, arc and the hyperbolic versions of the functions. Note: Only the radian version of the functions exist so far.
@frederikja163
Copy link
Member Author

I think it would also be handy to extend the trigonometry functions, to not only include radian versions but also have a degree version of each of the functions.

Copy link
Contributor

@jvbsl jvbsl left a comment

Choose a reason for hiding this comment

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

Some things I would change. Any second opinions on these?
As well as perhaps changing MathHelper to Math directly, as it does not only contain helper functions, but contains the Math-subset as well?

Edit: degree functions with Degree in name, or a corresponding Degree struct with implicit conversion from/to double?

src/OpenTK.Mathematics/MathHelper.cs Outdated Show resolved Hide resolved
src/OpenTK.Mathematics/MathHelper.cs Outdated Show resolved Hide resolved
src/OpenTK.Mathematics/MathHelper.cs Outdated Show resolved Hide resolved
src/OpenTK.Mathematics/MathHelper.cs Outdated Show resolved Hide resolved
src/OpenTK.Mathematics/MathHelper.cs Outdated Show resolved Hide resolved
src/OpenTK.Mathematics/MathHelper.cs Outdated Show resolved Hide resolved
Made the syntaxs of the trig functions more in line with what they would be mathematically
@frederikja163
Copy link
Member Author

frederikja163 commented Apr 14, 2019

As well as perhaps changing MathHelper to Math directly, as it does not only contain helper functions, but contains the Math-subset as well?

I agree with this, it would also make user coder look a lot cleaner since the name isn't as long. I would think this, however, would require the Mathhelper class to still exist but be marked as obsolete. If I could get the approval of a maintainer I could easily implement it in this PR

Edit: degree functions with Degree in name, or a corresponding Degree struct with implicit conversion from/to double?

As stated in my comment above, I too think this would be a good idea however I would like to get some opinions before I implement it, as well as some opinions on how to implement it.

Added a logarithmic function for the natural logarithm Ln(n) and a logarithmic function with a default base of 10
Changed parameter name of trig functions
@varon
Copy link
Member

varon commented Apr 15, 2019

@Perksey @Nihlus @jvbsl

I'd like to rename MathHelper to Mathf or even just Math. It's way more terse when using these functions.

As they're actual math, not just helpers this seems to be clearer too.

What are some opinions here?

@frederikja163
Copy link
Member Author

Just curious here, if it would be Mathf, what does the f mean? Unity uses the same, I always just assumed it might be some random letter.

@jvbsl
Copy link
Contributor

jvbsl commented Apr 15, 2019

mostly if a "f" is attached it is meant to stand for float, so therefore Mathf would contain math floating point functions. But tbh I would be against that, because:

  1. d is often used for double, and a lot of functions in Math are double not float, therefore one could misinterpret f(even if double is a floating point as well).
  2. Math(which shall implement the complete System.Math subset) contains functions for integer operations as well.

But with the name Math, I would be completely fine though.

@Perksey
Copy link
Contributor

Perksey commented Apr 15, 2019

@varon I definitely agree. Math is fine with me.

@varon
Copy link
Member

varon commented Apr 15, 2019

Seems we're all on the same page. @frederikja163 - feel free to change the class name to Math when you can.

@Perksey
Copy link
Contributor

Perksey commented Apr 15, 2019

Actually, naming it Math will almost definitely require type aliasing as Math is in the System namespace - a commonly referenced namespace. We should investigate other names.

@frederikja163
Copy link
Member Author

How about MathO, for opentk?

@Perksey
Copy link
Contributor

Perksey commented Apr 15, 2019

I think we should just use Mathf.

@varon
Copy link
Member

varon commented Apr 15, 2019

@Perksey - Great catch. Totally slipped past me as F# has no trouble resolving the right methods in this case.

At this point I'd just go for Mathf. It's terse and everyone will know where to look coming from unity anyway. Set up a poll on discord and go with what the contributors want.

A commit containing the majority of the mirror, submitting this now so it can be reviewed while I'm writing the last bits.
@frederikja163
Copy link
Member Author

I would like to note, like i did on discord aswell. In .netcore 3.0 preview it has MathF, it is with a capital F but it might be confusing.

@Perksey
Copy link
Contributor

Perksey commented Apr 15, 2019

We're currently holding a vote in the discord and it will conclude at 4pm tomorrow (UK time)

So far it's looking like it shouldn't be changed.

The math mirror is now complete up to .netcore 2.0.
@frederikja163
Copy link
Member Author

The PR is currently ready with every System.Math function up to .netcore 2.0, there are still some functions from .netcore 3.0 preview that could easily be added, like clamp for every value type. I would like to know if i should complete the PR for those aswell, so we are a bit more future proof.

@Perksey
Copy link
Contributor

Perksey commented Apr 15, 2019

If you're willing to, do it! :D

Copy link
Contributor

@Perksey Perksey left a comment

Choose a reason for hiding this comment

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

Looks good so far, keep it up! I'll officially approve this after the member name vote has concluded in the Discord. Once you're done, remove the WIP tag from the title. :)

@Perksey
Copy link
Contributor

Perksey commented Apr 15, 2019

Oh and also, fix the build errors please :P
image

@frederikja163
Copy link
Member Author

Oh and also, fix the build errors please :P
image

Fixed

@frederikja163 frederikja163 changed the title [Wip] System.Math and OpenToolkit.MathHelper symmetry System.Math and OpenToolkit.MathHelper symmetry Apr 15, 2019
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

4 participants