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

Overlay Emote Modifier #6590

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Overlay Emote Modifier #6590

wants to merge 8 commits into from

Conversation

Jake2500
Copy link

Adds a o! modifier that makes the modified emote an "overlay".

I'm not very creative, but an example: Waiting o! Clap
Screenshot 2024-01-25 230905

Screenshot 2024-01-25 230614

I have no idea whether o! is a good choice.
Suggestions and review comments welcome.

@night
Copy link
Owner

night commented Jan 27, 2024

Thanks for the PR. To prevent abuse of this, you'll need to add a restriction preventing multiple emotes from stacking on top of a single emote. I think a sane limit is only 1 emote can stack on top of the prior emote.

Additionally, what happens if there isn't a prior emote to stack on top of? Doesn't that move the emote out of view?

@Jake2500
Copy link
Author

I think a sane limit is only 1 emote can stack on top of the prior emote.

Additionally, what happens if there isn't a prior emote to stack on top of? Doesn't that move the emote out of view?

This is a good point. I've covered both these cases by requiring there to be a prior emote and that it does not have the overlay modifier.

message before after
peepoHappy o! Clap 1 1
o! Clap 2 2
peepoHappy foo o! Clap 3 3
peepoHappy o! Clap o! Clap2 4 4

break;
}
modifiers.push(partMetadataItem);
parts[k] = null;
}

const overlayModifierPredicate = ({modifier}) => modifier === 'o!';
const negate = (pred) => (x) => !pred(x);
Copy link
Owner

Choose a reason for hiding this comment

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

can we pull these function definitions out of the hot path? they look re-usable

Copy link
Author

Choose a reason for hiding this comment

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

I've moved these out and near the definitions of the modifiers.

isOverlay[j] = true;
} else {
// Strip the overlay modifier from the part.
modifiers = modifiers.filter(negate(overlayModifierPredicate));
Copy link
Owner

Choose a reason for hiding this comment

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

stripping it works, but it also results in the modifier going missing from the output. for other modifiers, we do not show the modified emote but we show the modifier was sent when used incorrectly. is there a way to replicate this behavior here?

image

Copy link
Owner

Choose a reason for hiding this comment

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

as an aside, do you have an image asset for this modifier? if not, we can probably have one made

Copy link
Author

Choose a reason for hiding this comment

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

is there a way to replicate this behavior here?

I've modified it so that if an overlay modifier is removed, it replaces the corresponding parts in the part array.
Screenshot 2024-02-17 094835

do you have an image asset for this modifier?

I'm no artist, so feel free to use or not use this.
overlay2

Copy link
Owner

Choose a reason for hiding this comment

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

@dclstn any ideas on the icon for this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Something like so perhaps:

Copy link
Author

Choose a reason for hiding this comment

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

That's so much better 👏

Copy link
Contributor

Choose a reason for hiding this comment

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

When the image is small in emote form, it kind of just looks like 1 square. Maybe make it so that the top square doesn't overlap the bottom one quite so completely?

Copy link
Owner

@night night Mar 12, 2024

Choose a reason for hiding this comment

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

agreed with Dallas' feedback. you can't tell what this is when smaller. it also resembles a copy button, which is not the intention.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How's this?

downsized for reference:


Copy link
Owner

Choose a reason for hiding this comment

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

this looks better

@night
Copy link
Owner

night commented Mar 16, 2024

It doesn't look like overlay emotes seem to work when you post them. but they do appear when others post them. might be related to how twitch is rendering messages you post.

@Jake2500
Copy link
Author

It doesn't look like overlay emotes seem to work when you post them. but they do appear when others post them. might be related to how twitch is rendering messages you post.

Screenshot 2024-03-16 173012
That's a screenshot of a message posted by me in twitch chat.

What are you seeing and how can I reproduce it?

@night
Copy link
Owner

night commented Mar 18, 2024

What are you seeing and how can I reproduce it?

Make sure you disable other browser extensions, as perhaps another extension is affecting things. But essentially, posting the overlay modifier results in

image

It seems when other users post it, or you reload, it renders properly:

image

My guess here is Twitch is rendering what is posted differently when you post.

@Jake2500
Copy link
Author

Jake2500 commented Mar 20, 2024

That's very odd. I can't reproduce this in Chrome 122.0.6261.113,
No extensions enabled.

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.

4 participants