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 2 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
8 changes: 4 additions & 4 deletions pygame/mask.py
@@ -1,6 +1,6 @@
# Implement the pygame API for the bitmask functions

from math import atan2, pi
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 @@ -47,11 +47,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
310 changes: 310 additions & 0 deletions pygame/math.py
@@ -0,0 +1,310 @@
""" Math module """

Copy link
Member

Choose a reason for hiding this comment

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

Could you add a suitable LGPL header to this file, please?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure. Whose names do I include?
For math_test.py, most of it came from the original pygame repo, so leave it as is?

Copy link
Member

Choose a reason for hiding this comment

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

You've done the vast majority of the work on math.py, so add whichever names you think are relevant.

For the test cases, we've been leaving largely unchanged, so leaving math_test.py as is seems fine.

from numbers import Number


swizzling = False

Copy link
Member

@drnlm drnlm Mar 20, 2017

Choose a reason for hiding this comment

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

in pygame, VECTOR_EPSILON is not directly exposed to the module since it's part of the C implementaton.

We may want to use __all__ to define the public API we expose explicitly.

The same concern applies to the pow_compat helper.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that you want to hide some of these details, but not sure how to use __all__ for this. If I add something like __all__ = ['enable_swizzling', 'disable_swizzling', 'Vector2', 'Vector3'] to math.py, then the dir() and ipython autocomplete output is still the same. It only makes a difference if I do from pygame.math import *. Adding __all__ to the top level __init__.py file would be hard, since I'd have to list every single function in the package.

How closely do we need to match pygame?
E.g. It provides:

In [15]: dir(pygame.math)
Out[15]: 
['Vector2',
 'Vector3',
 'VectorElementwiseProxy',
 'VectorIterator',
 '_PYGAME_C_API',
 '__doc__',
 '__file__',
 '__name__',
 '__package__',
 'disable_swizzling',
 'enable_swizzling']

So far, this module provides:

In [2]: dir(pygame.math)
Out[2]: 
['ElementwiseVector2Proxy',
 'ElementwiseVector3Proxy',
 'ElementwiseVectorProxyBase',
 'Number',
 'VECTOR_EPSILON',
 'Vector2',
 'Vector3',
 '__builtins__',
 '__doc__',
 '__file__',
 '__name__',
 '__package__',
 '_rotate_2d',
 'absolute_import',
 'disable_swizzling',
 'division',
 'enable_swizzling',
 'math',
 'pow_compat',
 'string_types']

I prefer @hodgestar's suggestion to use the _ prefix to imply private use. Is the same required for imports, e.g. from numbers import Number -> from numbers import Number as _Number, so that they don't show up so easily?

Copy link
Member

Choose a reason for hiding this comment

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

We don't need to match pygaqme exactly, but we do want to make the public API we expose explicit, so it's clear when people are poking at something internal.

Using _VECTOR_EPSILON is fine, since it's then clear that it's internal. I would personally also mark classes like ElementwiseVector2Proxy, which are not intended to be used outside of math.py as such, even though that difffers from what pygame itself exposes.


def enable_swizzling():
"""Globally enables swizzling for vectors.

Enables swizzling for all vectors until disable_swizzling() is called.
By default swizzling is disabled.
"""
global swizzling
swizzling = True


def disable_swizzling():
"""Globally disables swizzling for vectors.

Disables swizzling for all vectors until enable_swizzling() is called.
By default swizzling is disabled.
"""
global swizzling
swizzling = False


class Vector2(object):
"""A 2-Dimensional Vector."""

def __init__(self, *args):
if len(args) == 0:
self.x = 0.0
self.y = 0.0
elif len(args) == 1:
if isinstance(args[0], Vector2):
self.x = args[0].x
self.y = args[0].y
elif isinstance(args[0], basestring):
if args[0].startswith("<Vector2(") and args[0].endswith(")>"):
tokens = args[0][9:-2].split(",")
try:
self.x = float(tokens[0])
self.y = float(tokens[1])
except ValueError:
raise TypeError("Invalid string arguments - not floats ({}).".
format(args[0]))
else:
raise TypeError("Invalid string argument - not like __repr__ ({}).".
format(args[0]))
Copy link
Member

Choose a reason for hiding this comment

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

pygame supports initiliasing Vector2 with a single number:

>>> Vector2(2)
<Vector2(2, 0)>

This should either be supported, or the discrepency should be noted.

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'll add that, and a test. Same for Vector3.

elif len(args[0]) == 2:
self.x = args[0][0]
self.y = args[0][1]
else:
raise TypeError("Invalid argument length ({}).".format(len(args[0])))
else:
self.x = args[0]
self.y = args[1]

def __repr__(self):
return "<Vector2({}, {})>".format(self.x, self.y)
Copy link
Member Author

Choose a reason for hiding this comment

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

We should probably use self._x and self._y whenever we read x or y internally to avoid going through the property lookup.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. Will do.


def __str__(self):
return "[{}, {}]".format(self.x, self.y)

def __len__(self):
return 2

def __getitem__(self, key):
"""Provide indexed read access (vec[0] is x vec[1] is y)."""
if isinstance(key, slice):
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 wonder if we could implement this whole function as return [self.x, self.y][key]?

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, that's a simple way to do it! I tried it, and it works.

return [self[ind] for ind in xrange(*key.indices(len(self)))]
elif isinstance(key, int):
if key < 0:
key += len(self)
if key == 0:
return self.x
elif key == 1:
return self.y
else:
raise IndexError("Out of range index requested: {}".format(key))
else:
raise TypeError("Invalid argument type")

def __setitem__(self, key, value):
"""Provide indexed modification."""
if isinstance(value, Vector2):
value = [value.x, value.y]
if isinstance(key, slice):
indices = range(*key.indices(len(self)))
if len(value) <= len(self) and len(value) == len(indices):
for count, index in enumerate(indices):
self[index] = value[count]
else:
raise ValueError("Invalid slice or value arguments ({}).".format(key))
elif isinstance(key, int):
if key < 0:
key += len(self)
if key == 0:
self.x = value
elif key == 1:
self.y = value
else:
raise IndexError("Out of range index requested: {}".format(key))
else:
raise TypeError("Invalid argument type")

def __delitem__(self):
"""Item deletion not supported."""
raise TypeError("Items may not be deleted.")

def __eq__(self, other):
if isinstance(other, Vector2):
return (self.x == other.x) and (self.y == other.y)
elif hasattr(other, '__iter__'):
try:
other_v = Vector2(other)
return self == other_v
except TypeError:
# Doesn't seem to be vector2-like, so NotImplemented
return False
return False
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 this should return NotImplemented instead of return False in both cases (see https://docs.python.org/2/library/stdtypes.html#the-notimplemented-object).

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 pygame already returns False here it's probably slightly better to follow its implementation though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tried with NotImplemented, but the existing unit tests prefer returning False, so leaving it that way.


def __ne__(self, other):
return not self.__eq__(other)
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 need to do something like return not (self == other) here so that the comparison dispatch can do its thing.

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 pygame already implements it this way, then maybe it's better to keep it this way.


def __add__(self, other):
if isinstance(other, Vector2):
return Vector2(self.x + other.x, self.y + other.y)
elif hasattr(other, '__iter__'):
try:
other_v = Vector2(other)
return self + other_v
except TypeError:
# Doesn't seem to be vector2-like, so NotImplemented
return NotImplemented
return NotImplemented

def __radd__(self, other):
return self.__add__(other)

def __sub__(self, other):
if isinstance(other, Vector2):
return Vector2(self.x - other.x, self.y - other.y)
elif hasattr(other, '__iter__'):
try:
other_v = Vector2(other)
return self - other_v
except TypeError:
# Doesn't seem to be vector2-like, so NotImplemented
return NotImplemented
return NotImplemented

def __rsub__(self, other):
if isinstance(other, Vector2):
return Vector2(other.x - self.x, other.y - self.y)
elif hasattr(other, '__iter__'):
try:
other_v = Vector2(other)
return other_v - self
except TypeError:
# Doesn't seem to be vector2-like, so NotImplemented
return NotImplemented
return NotImplemented

def __mul__(self, other):
if isinstance(other, Number):
return Vector2(self.x * float(other), self.y * float(other))
return NotImplemented

def __rmul__(self, other):
return self.__mul__(other)

def __div__(self, other):
if isinstance(other, Number):
return Vector2(self.x / float(other), self.y / float(other))
return NotImplemented

def __floordiv__(self, other):
if isinstance(other, Number):
return Vector2(self.x // other, self.y // other)
return NotImplemented

def __bool__(self):
"""bool operator for Python 3."""
return self.x != 0 or self.y != 0

def __nonzero__(self):
"""bool operator for Python 2."""
return self.__bool__()

def __neg__(self):
return Vector2(-self.x, -self.y)

def __pos__(self):
return Vector2(self.x, self.y)

@property
def x(self):
"""Vector x value."""
return self._x

@x.setter
def x(self, value):
if isinstance(value, Number):
self._x = value
else:
raise TypeError("Value {} is not a valid number.".format(value))

@property
def y(self):
"""Vector y value."""
return self._y

@y.setter
def y(self, value):
if isinstance(value, Number):
self._y = value
else:
raise TypeError("Value {} is not a valid number.".format(value))

def dot(self, vec):
"""calculates the dot- or scalar-product with the other vector.

dot(Vector2) -> float.
"""
if not isinstance(vec, Vector2):
vec = Vector2(vec)
return self.x * vec.x + self.y * vec.y

def cross(self):
"""calculates the cross- or vector-product."""
pass

def length(self):
"""returns the euclidic length of the vector."""
pass

def length_squared(self):
"""returns the squared euclidic length of the vector."""
pass

def normalize(self):
"""returns a vector with the same direction but length 1."""
pass

def normalize_ip(self):
"""normalizes the vector in place so that its length is 1."""
pass

def is_normalized(self):
"""tests if the vector is normalized i.e. has length == 1."""
pass

def scale_to_length(self):
"""scales the vector to a given length."""
pass

def reflect(self):
"""returns a vector reflected of a given normal."""
pass

def reflect_ip(self):
"""reflect the vector of a given normal in place."""
pass

def distance_to(self):
"""calculates the euclidic distance to a given vector."""
pass

def distance_squared_to(self):
"""calculates the squared euclidic distance to a given vector."""
pass

def lerp(self):
"""returns a linear interpolation to the given vector."""
pass

def slerp(self):
"""returns a spherical interpolation to the given vector."""
pass

def elementwise(self):
"""The next operation will be performed elementwize."""
pass

def rotate(self):
"""rotates a vector by a given angle in degrees."""
pass

def rotate_ip(self):
"""rotates the vector by a given angle in degrees in place."""
pass

def angle_to(self):
"""calculates the angle to a given vector in degrees."""
pass

def as_polar(self):
"""returns a tuple with radial distance and azimuthal angle."""
pass

def from_polar(self):
"""Sets x and y from a polar coordinates tuple."""
pass


class Vector3(object):
pass