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

Apply alpha blend modes and coloration to textures based on sprite data. #474

Merged
merged 2 commits into from
May 23, 2020

Conversation

ironfroggy
Copy link
Contributor

I know there was debate before about where opacity and color data should live, but i wanted to open the PR with the customizations I've been using to start the discussion again with something concrete.

@ironfroggy ironfroggy requested a review from a team as a code owner May 17, 2020 21:05
Copy link
Collaborator

@pathunstrom pathunstrom left a comment

Choose a reason for hiding this comment

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

I still need to pull this and play around with it, but my first instinct is that the only thing here that concerns me is the standing concern about cluttering the Sprite attribute namespace. While I don't think opacity_mode is likely to be something end users are likely to make their own name, color could be.

I think this is the right place to do this sort of thing. We don't really have a way to store this transforms and cache them that I can see on the face of it, but I'm not sure we'll gain anything by doing so, either.

For now, I think we should discuss the names, and test this workflow in some different contexts. Can you provide some basic descriptions of how to use these features either in the PR or in comments?

Comment on lines 116 to 118
last_opacity: int = 255
last_opacity_mode: str = 'blend'
last_color: Tuple[int, int, int] = (255, 255, 255)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these doing anything? I don't see them referenced elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was part of an attempt I made to only call the SDL Set* functions when these changed, but I didn't get it working right and dropped it. I will clean this up.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Was just checking!

SDL_SetTextureAlphaMod, texture.inner, opacity,
_check_error=lambda rv: rv < 0
)
if opacity_mode == 'add':
Copy link
Collaborator

Choose a reason for hiding this comment

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

Direct implementation is less of a big deal, but I'd definitely avoid magic strings if we can. Either make these class attributes, exported constants, or Flags. (Or enums, but want to check with some educators before we go too far down that route.)

@ironfroggy
Copy link
Contributor Author

ironfroggy commented May 18, 2020

Here's an example of where I use this in Seed Magic to do the particle effects:

@classmethod
def spawn(cls, pos, color, heading=None, tsize=2.5):
        s = cls.sparkles[cls.index]
        cls.index = (cls.index + 1) % cls.size
        if color == COLOR_BLACK:
            s.opacity_mode = ppb.flag.BlendModeNone
            s.opacity = 255
        else:
            s.opacity_mode = ppb.flag.BlendModeAdd
            s.opacity = 128
        s.color = color
        s.position = pos
        s.rotation = randint(0, 260)
        s.size = 1.5
        s.layer = 100
        cls.t.tween(s, 'opacity', 0, 0.5, easing='linear')
        cls.t.tween(s, 'size', tsize, 0.5, easing='linear')
        delay(0.5, lambda: setattr(s, 'size', 0))
        if heading:
            cls.t.tween(s, 'position', heading, 0.5, easing='linear')

@pathunstrom
Copy link
Collaborator

One more question on this, is Sprite.color essentially for tinting the underlying images?

@ironfroggy
Copy link
Contributor Author

@pathunstrom Yes, right. Actually, with this, the generated sprites could be recolored on-demand, which might be useful.

opacity = getattr(game_object, 'opacity', 255)
opacity_mode = getattr(game_object, 'opacity_mode', flags.BlendModeBlend)
opacity_mode = OPACITY_MODES[opacity_mode]
color = getattr(game_object, 'color', (255, 255, 255))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
color = getattr(game_object, 'color', (255, 255, 255))
color = getattr(game_object, 'tint', (255, 255, 255))

Regarding suggestion on discord.

Copy link
Collaborator

@pathunstrom pathunstrom left a comment

Choose a reason for hiding this comment

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

Just marking this, but would love to get it in today.

  1. Needs to clean up the merge conflicts. You can just pull from master or rebase whichever you prefer.
  2. Change .color to .tint

After that I can merge it in.

@ironfroggy ironfroggy force-pushed the render-sprite-rgba branch from d7858c3 to bf332fb Compare May 22, 2020 20:07
Copy link
Collaborator

@pathunstrom pathunstrom left a comment

Choose a reason for hiding this comment

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

This is good. We'll ask users for feedback during the 0.9 release and can iterate in the future.

bors r+

bors bot added a commit that referenced this pull request May 22, 2020
474: Apply alpha blend modes and coloration to textures based on sprite data. r=pathunstrom a=ironfroggy

I know there was debate before about where `opacity` and `color` data should live, but i wanted to open the PR with the customizations I've been using to start the discussion again with something concrete.

Co-authored-by: Calvin Spealman <ironfroggy@gmail.com>
@bors
Copy link
Contributor

bors bot commented May 22, 2020

Build failed:

@pathunstrom
Copy link
Collaborator

bors retry

@bors
Copy link
Contributor

bors bot commented May 23, 2020

@bors bors bot merged commit a83e55e into ppb:master May 23, 2020
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