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 rotatability to sprites. #214

Merged
merged 5 commits into from
Apr 5, 2019
Merged

Conversation

pathunstrom
Copy link
Collaborator

@pathunstrom pathunstrom commented Mar 25, 2019

First pass at this. Will still need to add the functionality to rotate the images themselves.

Closes #142

ppb/sprites.py Outdated Show resolved Hide resolved
@pathunstrom pathunstrom changed the title [WIP] Add rotatability to sprites. Add rotatability to sprites. Mar 27, 2019
@pathunstrom
Copy link
Collaborator Author

Assuming it passes Travis, this should be ready to go.


@rotation.setter
def rotation(self, value):
self._rotation = value % 360
Copy link
Contributor

Choose a reason for hiding this comment

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

This can still be negative; should this use the same convention as ppb-vector and have -180 < _rotation <= 180?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the case of this interface, dunno if it matters. The tests have some negative rotation and it gives the equivalent positive rotation, which is a reasonable result, but if both you and @astronouth7303 feel like I should port that over here, I will.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pathunstrom I don't think it's much of an issue either way, I just wanted to check that was intentional.

ppb/sprites.py Outdated Show resolved Hide resolved
@@ -140,7 +140,34 @@ def _attribute_gate(self, attribute, bad_sides):
raise AttributeError(message)


class BaseSprite(EventMixin):
class Rotatable:
Copy link
Contributor

Choose a reason for hiding this comment

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

Rotatable sounds weird to me, but I'm a silly non-native speaker. shrugs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Native speaker, sounds weird to me, too, but lots of mixins are named based on their capabilities.

Copy link
Member

Choose a reason for hiding this comment

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

-able is approximately the right convention, but it leads to lots of weird words.

I would also accept CanBe*? Also weird.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So like CanBeRotated?

Copy link
Contributor

@nbraud nbraud Mar 30, 2019

Choose a reason for hiding this comment

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

Or just CanRotate? In any case, I don't think Rotatable is a big issue, I was just very confused whether there was a typo, or it was just weird, or I was just bad at English.

Co-Authored-By: pathunstrom <pathunstrom@gmail.com>
Copy link
Member

@AstraLuma AstraLuma left a comment

Choose a reason for hiding this comment

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

We should probably add transformed resource caching.

Otherwise, looks good.

@AstraLuma AstraLuma merged commit edd7499 into ppb:master Apr 5, 2019
@pathunstrom pathunstrom deleted the sprite_rotation branch October 24, 2020 20:54
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.

3 participants