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

Sprite Layer API #322

Merged
merged 14 commits into from
Aug 21, 2019
Merged

Sprite Layer API #322

merged 14 commits into from
Aug 21, 2019

Conversation

pathunstrom
Copy link
Collaborator

@pathunstrom pathunstrom commented Jul 15, 2019

Implements a basic layering API.

Sprites can have a layer attribute that should be an integer, but due to implementation details could be a float.

Does not change the renderer as part of this change, will do so in a followup PR.

This is one step on the way to #276

Proposal:

  • BaseScene.sprite_layers() which returns all sprites based on their layering order.
  • Layers are simple attributes on Sprites.
  • You can name layers via BaseScene.named_layers which will default to incrementing each layer number by 1.
  • You can have greater control over your layer definition by providing BaseScene.defined_layers as Dict[str, int].
  • There is a BaseSprite.default_layer which a Sprite without a layer attribute defaults to. It can be a string or an int as preferred by the end user.

@pathunstrom pathunstrom self-assigned this Jul 15, 2019
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.

Ok, some takes on this API direction

  • So far, we've avoided putting too much logic in BaseSprite and BaseScene and I'm not sure I want that to change? (I assume you want BaseScene.__iter__() to produce sprites in layer order)
  • BaseScene.define_layer() is totally against the declarative direction we've been moving in an encouraging
  • The thing I had been picturing for string layers is just BaseScene.layers as a list of strings, and the order in the list defines the order of layers? I guess a dict works but it feels clunkier? Maybe I just don't like it because it's not my idea.

tests/test_layering.py Outdated Show resolved Hide resolved
@pathunstrom
Copy link
Collaborator Author

So far, we've avoided putting too much logic in BaseSprite and BaseScene and I'm not sure I want that to change?

That's definitely true of BaseSprite, I think BaseScene is just a matter of the only functionality we've needed is container and iteration and lookups.

(I assume you want BaseScene.iter() to produce sprites in layer order)

Yes. That's primarily a backwards compatibility thing: We don't have to change renderer logic by making sure __iter__ is in layer order.

I'm going to ponder on the rest of this. I've got ideas.

@AstraLuma
Copy link
Member

ok, so here's the concrete things against implementing ordering in __iter__():

  • It makes assigning to properties have a side-effect (and not just a side-effect of "update this cached pre-compute")
  • It really doesn't fit with the discussions we've had about non-sprite children

@pathunstrom pathunstrom changed the title Adds tests to define layering API. Layering API Jul 15, 2019
tests/test_layering.py Outdated Show resolved Hide resolved
tests/test_layering.py Outdated Show resolved Hide resolved
tests/test_layering.py Outdated Show resolved Hide resolved
tests/test_layering.py Outdated Show resolved Hide resolved
tests/test_layering.py Outdated Show resolved Hide resolved
@pathunstrom pathunstrom changed the title Layering API Sprite Layer API Aug 7, 2019
@pathunstrom pathunstrom marked this pull request as ready for review August 7, 2019 02:39
@pathunstrom pathunstrom requested a review from a team as a code owner August 7, 2019 02:39
@pathunstrom
Copy link
Collaborator Author

An implementation! Once this is merged, I can start work on both a layering mixin for Sprites and start the work on the Renderer rebuild.

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.

I was expecting the render-side changes to be part of this PR, too? Unless you're immediately planning to do a bunch of renderer work at once.

But what I do see is fine.

tests/test_layering.py Outdated Show resolved Hide resolved
@AstraLuma
Copy link
Member

One last comment: Did we want to add layer: int = 0 to the base sprite class?

@pathunstrom
Copy link
Collaborator Author

That's reasonable. Will add it.

@AstraLuma
Copy link
Member

bors r+

bors bot added a commit that referenced this pull request Aug 21, 2019
322: Sprite Layer API r=astronouth7303 a=pathunstrom

Implements a basic layering API.

`Sprites` can have a `layer` attribute that should be an integer, but due to implementation details could be a float.

Does not change the renderer as part of this change, will do so in a followup PR.

This is one step on the way to #276


Proposal:

* `BaseScene.sprite_layers()` which returns all sprites based on their layering order.
* Layers are simple attributes on `Sprites`.
* You can name layers via `BaseScene.named_layers` which will default to incrementing each layer number by 1.
* You can have greater control over your layer definition by providing `BaseScene.defined_layers` as Dict[str, int].
* There is a `BaseSprite.default_layer` which a Sprite without a layer attribute defaults to. It can be a string or an int as preferred by the end user.

Co-authored-by: Piper Thunstrom <pathunstrom@gmail.com>
@bors
Copy link
Contributor

bors bot commented Aug 21, 2019

Build failed

  • Windows python:3.7-windowsservercore-1809

@pathunstrom
Copy link
Collaborator Author

bors retry

bors bot added a commit that referenced this pull request Aug 21, 2019
322: Sprite Layer API r=astronouth7303 a=pathunstrom

Implements a basic layering API.

`Sprites` can have a `layer` attribute that should be an integer, but due to implementation details could be a float.

Does not change the renderer as part of this change, will do so in a followup PR.

This is one step on the way to #276


Proposal:

* `BaseScene.sprite_layers()` which returns all sprites based on their layering order.
* Layers are simple attributes on `Sprites`.
* You can name layers via `BaseScene.named_layers` which will default to incrementing each layer number by 1.
* You can have greater control over your layer definition by providing `BaseScene.defined_layers` as Dict[str, int].
* There is a `BaseSprite.default_layer` which a Sprite without a layer attribute defaults to. It can be a string or an int as preferred by the end user.

Co-authored-by: Piper Thunstrom <pathunstrom@gmail.com>
@bors
Copy link
Contributor

bors bot commented Aug 21, 2019

Build succeeded

  • docs
  • Linux python:3.6-slim
  • Linux python:3.7-slim
  • macOS PYTHON:3.6.8
  • macOS PYTHON:3.7.2
  • pep517
  • Windows python:3.6-windowsservercore-1809
  • Windows python:3.7-windowsservercore-1809

@bors bors bot merged commit c891f37 into ppb:master Aug 21, 2019
@pathunstrom pathunstrom deleted the layering branch August 22, 2019 00:18
bors bot added a commit that referenced this pull request Aug 24, 2019
350: Renderer renders in layer order. r=astronouth7303 a=pathunstrom

Includes a new visual test.

It was a one liner to add after #322.

Co-authored-by: Piper Thunstrom <pathunstrom@gmail.com>
@pathunstrom pathunstrom restored the layering branch April 15, 2020 00:44
@pathunstrom pathunstrom deleted the layering branch April 15, 2020 00:45
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.

2 participants