-
Notifications
You must be signed in to change notification settings - Fork 120
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
Implementation of DateTime and PShape #46
Conversation
BOX = 41 # primitive | ||
|
||
|
||
class PShape(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're dropping the P
prefix in p5. So Processing's "PShape" becomes "Shape" in p5. We already have a Shape
class and this new code should go in there.
MAX_POINT_ACCURACY = 200 | ||
POINT_ACCURACY_FACTOR = 10 | ||
|
||
OPEN, CLOSE = 1, 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a nicer way of doing this other than assigning numbers to constants?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may use strings instead.
But numbers are faster to work with.
Also, the variables have been named quite appropriately.
Plus, these are the same constant names as used in the Java code, so there is a less of a learning curve/ less work while switching from Java to p5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering of other solutions that do not necessarily use these variables. For instance, instead of defining OPEN
and CLOSE
we could just use keyword arguments: def end_shape(self, open=True)
We may use strings instead. But numbers are faster to work with.
Correct. But this doesn't seem to influence things that much. I looked around a bit and at least in the popular scientific computing libraries this seems to be quite common. In scipy for instance, one would often do scipy.optimize.minimize(f, x0, method='CG')
. I really don't think something like self._kind == 'group'
would be an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like if n1 attempts to convert a Java or even a Python Mode sketch into p5py is gonna know hell! :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GoToLoop, ¯\_(ツ)_/¯
self._vertices = [] | ||
self.close = False | ||
|
||
def beginShape(self, kind = None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're trying to use context managers in p5. So instead of
foo.beginShape()
# code
foo.endShape()
we would like to have
with foo.begin_shape():
# code
See the code for push_matrix()
in p5/transforms.py for the implementation details. And the contextlib docs: https://docs.python.org/3/library/contextlib.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
That would be more pythonic.
My main aim was initially just to make it function just like the Java code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main aim was initially just to make it function just like the Java code.
We don't have to. We're just keeping the higher-level api similar, the internals can definitely change! And of course, we always try to use some of the Python features whenever possible :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I'll keep that in mind.
This is the same reason why I used beginShape()
instead of begin_shape()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
def endShape(self, mode=OPEN): | ||
if self._kind == GROUP: | ||
warnings.warn("Cannot end GROUP shape") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throwing an error here makes more sense, no?
This is quite nice! But a couple of general comments:
|
|
||
def beginShape(self, kind = None): | ||
self._vertices = [] | ||
if type is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type
is a Python built-in!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry. My bad😌
Please resolve conflicts with main branch |
The features in this pull request have been incorporated in the current version of p5py |
This code was written by me as a prototype for my GSoC 2018 proposal.
I was not selected, but I would want you to please merge this with the main repo.
The code contains: