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

Add support for pygame.math. #74

Merged
merged 16 commits into from Mar 23, 2017
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions benchmarks/run_benchmark.py
@@ -1,3 +1,4 @@
from __future__ import absolute_import
Copy link
Member Author

Choose a reason for hiding this comment

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

I think we're missing a blank line after the future import (at least we seem to do that elsewhere).

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure. I like consistency too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tx.

import math
import sys
import time
Expand Down
2 changes: 1 addition & 1 deletion pygame/__init__.py
Expand Up @@ -28,7 +28,7 @@
from pygame import (
display, color, surface, time, event, constants, sprite,
mouse, locals, image, transform, pkgdata, font, mixer,
cursors, key, draw, joystick
cursors, key, draw, joystick, math
)
from pygame.base import (
init, quit, HAVE_NEWBUF, get_sdl_version, get_sdl_byteorder,
Expand Down
23 changes: 23 additions & 0 deletions pygame/compat.py
Expand Up @@ -114,3 +114,26 @@ def next_(i, *args):
import itertools.imap as imap_
except ImportError:
imap_ = map


# The behaviour of the power operator, '**' and built-in pow() function,
# changed in Python 3.x. In Python 3.x raising a negative number to a
# fractional power results in a complex number, while in earlier versions
# it raised a ValueError. Matching the behaviour of pygame requires
# the ValueError instead of a complex result. The compatibility function
# defined below is provided for this purpose.
Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like math.pow(-2.2, 1.1) raises the ValueError on both Python 2.7.12 and 3.5.2, so maybe we can just use math.pow instead of our own function?

Copy link
Member

Choose a reason for hiding this comment

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

While math.pow will return a ValueError, it is also using the underlying C math library and will have a different precision from pow.

pygame is explicitly using pow underneath it's implementation, so if we use math.pow(), there will be differences between the implementations. Whether these will matter is another question, since they'll arguably only show up in odd corner cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried math.pow(), but there's an issue with 0 ** -1, which raises a ZeroDivisionError, while the math module raises a ValueError. The unit tests have a check or the former.
Interesting point about the implementation differences.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tx for the discussion. Happy for it to stay this way then.

try:
(-2.2) ** 1.1

def pow_compat(x, y):
"""Return x ** y, with Python 2.x compatible exceptions."""
if x < 0 and not float(y).is_integer():
raise ValueError("negative number cannot be raised to a fractional power")
else:
return x ** y

except ValueError:

def pow_compat(x, y):
"""Return x ** y, with Python 2.x compatible exceptions."""
return x ** y
2 changes: 2 additions & 0 deletions pygame/draw.py
Expand Up @@ -20,6 +20,8 @@
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
# MA 02110-1301 USA

from __future__ import absolute_import

from pygame.surface import locked
from pygame.color import create_color
from pygame.compat import xrange_
Expand Down
9 changes: 5 additions & 4 deletions pygame/mask.py
Expand Up @@ -18,7 +18,8 @@

# Implement the pygame API for the bitmask functions

from math import atan2, pi
from __future__ import absolute_import
import math
Copy link
Member Author

Choose a reason for hiding this comment

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

If this is intended to import from pygame.math it should use an explicit relative import, otherwise we should probably for from __future__ import absolute_import to avoid confusion.

Copy link
Contributor

Choose a reason for hiding this comment

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

mask.py is intending to import the standard math module - this code was there before the local math module was added. Should I still add the absolute_import? That's default behaviour isn't it, or are we aiming to support Python < 2.7?

Copy link
Member Author

Choose a reason for hiding this comment

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

Another place where I think we should have a blank line for consistency.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are a few more of these cases, but I'm not going to comment on them all individually. :)


from pygame._sdl import sdl, ffi
from pygame.surflock import locked
Expand Down Expand Up @@ -65,11 +66,11 @@ def angle(self):
if tot:
xc = xs // tot
yc = ys // tot
theta = atan2(2 * (xy / tot - xc * yc),
(xx / tot - xc * xc) - (yy / tot - yc * yc))
theta = math.atan2(2 * (xy / tot - xc * yc),
(xx / tot - xc * xc) - (yy / tot - yc * yc))
# covert from radians
# We copy this logic from pygame upstream, because reasons
theta = -90.0 * theta / pi
theta = -90.0 * theta / math.pi
return theta
return 0.0

Expand Down