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

Conversation

Projects
None yet
3 participants
@hodgestar
Member

hodgestar commented Oct 9, 2016

Work in progress.

@hodgestar

This comment has been minimized.

Member

hodgestar commented Oct 9, 2016

@ajoubertza I created a pull request for this so we can see how builds are going and have a unified place to make comments. Thanks for working on this!

@hodgestar

I left a bunch of small comments. Generally looking like a good start though!

@@ -1,6 +1,6 @@
# Implement the pygame API for the bitmask functions
from math import atan2, pi
import math

This comment has been minimized.

@hodgestar

hodgestar Oct 9, 2016

Member

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.

This comment has been minimized.

@ajoubertza

ajoubertza Oct 10, 2016

Contributor

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?

def __getitem__(self, key):
"""Provide indexed read access (vec[0] is x vec[1] is y)."""
if isinstance(key, slice):

This comment has been minimized.

@hodgestar

hodgestar Oct 9, 2016

Member

I wonder if we could implement this whole function as return [self.x, self.y][key]?

This comment has been minimized.

@ajoubertza

ajoubertza Oct 10, 2016

Contributor

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

except TypeError:
# Doesn't seem to be vector2-like, so NotImplemented
return False
return False

This comment has been minimized.

@hodgestar

hodgestar Oct 9, 2016

Member

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).

This comment has been minimized.

@hodgestar

hodgestar Oct 9, 2016

Member

If pygame already returns False here it's probably slightly better to follow its implementation though.

This comment has been minimized.

@ajoubertza

ajoubertza Oct 10, 2016

Contributor

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

return False
def __ne__(self, other):
return not self.__eq__(other)

This comment has been minimized.

@hodgestar

hodgestar Oct 9, 2016

Member

I think we need to do something like return not (self == other) here so that the comparison dispatch can do its thing.

This comment has been minimized.

@hodgestar

hodgestar Oct 9, 2016

Member

If pygame already implements it this way, then maybe it's better to keep it this way.

self.y = args[1]
def __repr__(self):
return "<Vector2({}, {})>".format(self.x, self.y)

This comment has been minimized.

@hodgestar

hodgestar Oct 9, 2016

Member

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

This comment has been minimized.

@ajoubertza

ajoubertza Oct 10, 2016

Contributor

Good point. Will do.

@ajoubertza

This comment has been minimized.

Contributor

ajoubertza commented Oct 10, 2016

Thanks for the review and suggestions @hodgestar. I'll have a look at in the course of the week.

@hodgestar

This comment has been minimized.

Member

hodgestar commented Feb 10, 2017

@ajoubertza How is this going? :)

@ajoubertza

This comment has been minimized.

Contributor

ajoubertza commented Feb 11, 2017

Hi @hodgestar. Hasn't been going for a while, but am looking at it again now!
The Vector3 class is still required. Easiest will be a copy and paste job from the Vector2 code. Generalising for N-dimensions doesn't seem worth the effort. Is that OK?

@hodgestar

This comment has been minimized.

Member

hodgestar commented Feb 11, 2017

@ajoubertza Definitely happy with that for a first pass. We can decide later if it's worth refactoring.

ajoubertza added some commits Feb 13, 2017

Fix some Vector2 elementwise operations
- Comparisons with NaNs and small deltas were not handled the same
  as the original pygame implementation.
- Some of the "elementwise op vector" tests were passing even
  though the comparison functions returned NotImplemented.  Added
  reverse checks that break in that case.  Fixed the elementwise
  operations in those cases.
Fix some failures in math and draw tests
- Fixed imports for math_test, so it can also be run as part of
  the whole test module, as Travis does.
- Draw tests run after test_circle_limits were failing because
  subsequent Surface constructor calls got back an 8 bpp surface,
  from sdl.SDL_GetVideoSurface() instead of making a new one.
  With the 8 bpp surface an exception is raised in
  _get_default_masks(..., alpha=True).
  Fix is to use the surface already created in setUp() instead of
  accessing the screen.
- Fix some Python 2/3 compatibility issues.
Merge branch 'master' into issue-53-add-pygame-math
# Conflicts:
#	pygame/draw.py
@@ -0,0 +1,1529 @@
""" Math module """

This comment has been minimized.

@drnlm

drnlm Mar 19, 2017

Member

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

This comment has been minimized.

@ajoubertza

ajoubertza Mar 19, 2017

Contributor

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

This comment has been minimized.

@drnlm

drnlm Mar 19, 2017

Member

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.

return Vector2(self.vector.x ** other.x, self.vector.y ** other.y)
elif isinstance(other, ElementwiseVector2Proxy):
return Vector2(self.vector.x ** other.vector.x,
self.vector.y ** other.vector.y)

This comment has been minimized.

@drnlm

drnlm Mar 19, 2017

Member

pygame explicity checks for a complex result and returns a ValueError to handle python3's behaviour. We should probably do the same here (pygame/src/math.c:3395, for example)

This comment has been minimized.

@ajoubertza

ajoubertza Mar 19, 2017

Contributor

Thanks @drnlm. I have done something like that in f849efd. Please have a look.

ajoubertza added some commits Mar 19, 2017

Fix power operator compatibility issue in math tests
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, as
per the math unit tests, requires a ValueError instead of a complex
result.
@drnlm

There are a few discrepencies between pygame and our behaviour that need to be either fixed or documented, but overall this looks really good.

format(args[0]))
else:
raise TypeError("Invalid string argument - not like __repr__ ({}).".
format(args[0]))

This comment has been minimized.

@drnlm

drnlm Mar 20, 2017

Member

pygame supports initiliasing Vector2 with a single number:

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

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

This comment has been minimized.

@ajoubertza

ajoubertza Mar 20, 2017

Contributor

Sure, I'll add that, and a test. Same for Vector3.

self.x = args[0]
self.y = args[1]
self.z = args[2]

This comment has been minimized.

@drnlm

drnlm Mar 20, 2017

Member

Likewise pygame supports initiliasing Vector3 with either 1 or 2 numbers. Again this should be supported or the discrepency should be documented.

r = draw.circle(screen, (0xff, 0xff, 0xff), (((2**16)//2 + 1),
((2**16)//2 + 1)), 0)
r = draw.circle(self.surf, (0xff, 0xff, 0xff), (((2**16)//2 + 1),
((2**16)//2 + 1)), 0)
self.assertEqual(r.x, 32769)
self.assertEqual(r.y, 32769)
self.assertEqual(r.w, 0)

This comment has been minimized.

@drnlm

drnlm Mar 20, 2017

Member

I would prefer to handle this bug fix as a separate pull request, rather than including it with the math work.

This comment has been minimized.

@ajoubertza

ajoubertza Mar 20, 2017

Contributor

OK, will do that. I was just trying to get the tests to pass.

This comment has been minimized.

@ajoubertza

ajoubertza Mar 20, 2017

Contributor

See PR #81.

This comment has been minimized.

@ajoubertza

ajoubertza Mar 21, 2017

Contributor

@drnlm Would you like me to revert the draw_test.py changes on this branch as well?

This comment has been minimized.

@hodgestar

hodgestar Mar 22, 2017

Member

I think just merging in master is good enough.

from pygame.compat import string_types, pow_compat
VECTOR_EPSILON = 1e-6 # For equality tests

This comment has been minimized.

@drnlm

drnlm Mar 20, 2017

Member

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.

This comment has been minimized.

@ajoubertza

ajoubertza Mar 20, 2017

Contributor

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?

This comment has been minimized.

@drnlm

drnlm Mar 20, 2017

Member

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.

abs(dx) < VECTOR_EPSILON and
abs(dy) < VECTOR_EPSILON and
abs(dz) < VECTOR_EPSILON)

This comment has been minimized.

@drnlm

drnlm Mar 20, 2017

Member

pygame exposes epsilon as a attribute on Vector2 and Vector3 to control the comparison accuracy

>>> from pygame.math import Vector2
>>> s=Vector2(2,2)
>>> s2=s+Vector2(1e-8,1e-8)
>>> s2 == s
True
>>> s2.epsilon = 1e-10
>>> s2 == s
False
>>> s == s2
True

We should either match this behaviour, or document the discrepency.

This comment has been minimized.

@ajoubertza

ajoubertza Mar 20, 2017

Contributor

Ok, I'll add that too, and a test.

@hodgestar

This comment has been minimized.

Member

hodgestar commented Mar 20, 2017

@drnlm

This comment has been minimized.

Member

drnlm commented Mar 20, 2017

That would also work, yes.

@ajoubertza

This comment has been minimized.

Contributor

ajoubertza commented Mar 20, 2017

Thanks for the review @drnlm

Improve pygame.math public API and conformance
- Rename internals with underscores to indicate usage
- Add epsilon attribute to vectors
- Allow initialisation for partial values (zero the rest)
- Include math module in __init__.py
@drnlm

drnlm approved these changes Mar 22, 2017

Looks good

Thanks

@ajoubertza

This comment has been minimized.

Contributor

ajoubertza commented Mar 22, 2017

Great. Thanks @drnlm. And it only took me a mere 5 months! 🐌

@hodgestar

I've left a couple of additional comments, but in general looks awesome.

There are quite a few places where we might we able to speed things up considerably by avoiding the property look up for vec.x, vec.y, vec.z but I think that is best tackled in a separate branch later if we want to (we can either carefully use _x a lot, or we can remove _x and try use just a setter for x without a getter -- which is a bit fiddly to arrange, but doable).

@@ -1,3 +1,4 @@
from __future__ import absolute_import

This comment has been minimized.

@hodgestar

hodgestar Mar 22, 2017

Member

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

This comment has been minimized.

@ajoubertza

ajoubertza Mar 23, 2017

Contributor

Sure. I like consistency too.

This comment has been minimized.

@hodgestar
# 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.

This comment has been minimized.

@hodgestar

hodgestar Mar 22, 2017

Member

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?

This comment has been minimized.

@drnlm

drnlm Mar 22, 2017

Member

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.

This comment has been minimized.

@ajoubertza

ajoubertza Mar 23, 2017

Contributor

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.

This comment has been minimized.

@hodgestar

hodgestar Mar 23, 2017

Member

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

@@ -18,7 +18,8 @@
# Implement the pygame API for the bitmask functions
from math import atan2, pi
from __future__ import absolute_import
import math

This comment has been minimized.

@hodgestar

hodgestar Mar 22, 2017

Member

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

This comment has been minimized.

@hodgestar

hodgestar Mar 22, 2017

Member

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

r = draw.circle(screen, (0xff, 0xff, 0xff), (((2**16)//2 + 1),
((2**16)//2 + 1)), 0)
r = draw.circle(self.surf, (0xff, 0xff, 0xff), (((2**16)//2 + 1),
((2**16)//2 + 1)), 0)
self.assertEqual(r.x, 32769)
self.assertEqual(r.y, 32769)
self.assertEqual(r.w, 0)

This comment has been minimized.

@hodgestar

hodgestar Mar 22, 2017

Member

I think just merging in master is good enough.

Vector2.__getattr__ = Vector2.__getattr_swizzle__
Vector3.__getattr__ = Vector3.__getattr_swizzle__
if '__oldsetattr__' not in dir(Vector2):

This comment has been minimized.

@hodgestar

hodgestar Mar 22, 2017

Member

I'm not following why we need to have __oldsetattr__ if __getattr__ is not set when swizzle is disabled? We could just set and delete __getattr__ to enable / disable swizzling. If this is just following the Python API pygame exposes, then I think that's a good enough reason to keep them, if not, I'd remove the extra complexity. Also, maybe I'm just missing something entirely. :)

This comment has been minimized.

@ajoubertza

ajoubertza Mar 23, 2017

Contributor

To enable swizzling we need to replace both __getattr__ and __setattr__. When we disable we are deleting the replacement __getattr__, and restoring the old __setattr__. Does that help?

This comment has been minimized.

@hodgestar

hodgestar Mar 23, 2017

Member

Where is the original __setattr__ defined? Possibly I'm still being silly but my Ctrl-F def __setattr__ is not finding anything. :/

This comment has been minimized.

@ajoubertza

ajoubertza Mar 23, 2017

Contributor

We don't define one, it is just the built in one we inherit from our parent, object.

This comment has been minimized.

@hodgestar

hodgestar Mar 23, 2017

Member

/me learns something new about Python objects. :)

@hodgestar

This comment has been minimized.

Member

hodgestar commented Mar 23, 2017

Looks good to me. Landing!

@hodgestar hodgestar merged commit dbcb41e into CTPUG:master Mar 23, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@drnlm drnlm referenced this pull request Mar 23, 2017

Closed

Add support for pygame.math. #53

@ajoubertza

This comment has been minimized.

Contributor

ajoubertza commented Mar 23, 2017

Yay! Thanks for the reviews.

@ajoubertza ajoubertza deleted the ajoubertza:issue-53-add-pygame-math branch Mar 23, 2017

@hodgestar

This comment has been minimized.

Member

hodgestar commented Mar 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment