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

feat: add paints support #89

Merged
merged 63 commits into from
Jun 28, 2022
Merged

Conversation

LosFarmosCTL
Copy link

@LosFarmosCTL LosFarmosCTL commented May 15, 2022

Description

This is a continuation building upon #86, since the original author is not working on that PR anymore:

Discord screenshots

image
image

Checklist

  • fetch paints from the API
  • linear-gradient paints
    • translate css angle to qt's 2-point style gradients
    • figure out how exactly css deals with repeating gradients and their color stops, to translate that to qt as well
  • radial-gradient paints
  • drop shadows (i might have a way to get these to work, its only theoretical though and i have not confirmed if it works 100%)
  • url paints
    • with animations (maybe)
  • setting to toggle paints off
  • separate the colon from the username itself and only draw paint on the nametag itself to match webchat behavior
  • some refactors xd

@AnatoleAM
Copy link

AnatoleAM commented May 15, 2022

oh and no idea how i got zneix commit merged in there, absolutely no Clue about how I messed that up myself

Seems that you forked the original Chatterino2, rather than this repo. This may or may not make it difficult to merge this pull request in the future. Might be best to fork the correct repo and open a new PR to avoid complications / merging dankness

@LosFarmosCTL
Copy link
Author

LosFarmosCTL commented May 15, 2022

Seems that you forked the original Chatterino2, rather than this repo. This may or may not make it difficult to merge this pull request in the future. Might be best to fork the correct repo and open a new PR to avoid complications / merging dankness

I have cloned the c7 repo, it might be some dankness around the github CLI still pushing that branch to my normal c2 fork since I cant make another fork from a fork, maybe I mixed up something while pulling (accidentally pulling something from upstream rather than origin or something 🤷).
It's definitely fine for merging, I have double-checked and this PR does not contain any changes from upstream c2, just the reference to this one commit, so there won't be any conflicts.

@AnatoleAM
Copy link

I can confirm you forked c2 as clicking on the branch in this PR takes you there;
image

Though in theory this shouldn't be an issue as long as you pull upstream from c7.

@LosFarmosCTL
Copy link
Author

I can confirm you forked c2 as clicking on the branch in this PR takes you there;

Yeah my mistake, I meant cloned, the branch is then just pushed to that repo since you can't fork the same (upstream)repo multiple times and I still need the normal c2 fork.

@AnatoleAM
Copy link

Also regarding the "animation" tickbox; animations were deprecated after it was discovered creating a cross-platform standard for them would be too difficult. All current animated paints are actually just using the ImageURL type.

@LosFarmosCTL
Copy link
Author

Also regarding the "animation" tickbox; animations were deprecated after it was discovered creating a cross-platform standard for them would be too difficult. All current animated paints are actually just using the ImageURL type.

Ah alright, yeah I had not looked into animations yet at all, just if it would be possible to implement in Qt, but only dealing with the URL type definitely makes things easier, good to know.

@LosFarmosCTL
Copy link
Author

Figuring out how to translate the angle of a linear-gradient from css to qt's gradient system turned out to be a gigantic pain(t) in the ass, but after trying out & throwing away 10 different solutions, plus learning some more about qt's geometry classes, I now figured out how to replicate the css gradient correctly. Still needs some work on calculating the correct end point for repeating gradients, but the main problem is fixed.

Paints are now actually displayed in chat as well.

@LosFarmosCTL
Copy link
Author

Drop shadows are now working, still planning to look into some edge cases and rendering will definitely not be exactly the same as in browser, but imo it works well enough.

I also had a new idea for rendering animated paints which I'll try out, but no idea if it's actually going to work. If not I'll postpone that to some later day and finish up whats currently working to at least get normal paints merged in.

@Excellify
Copy link

Animated paints are currently only used in 2 custom paints, used by 2 people, so its not a priority

@LosFarmosCTL
Copy link
Author

Animated paints are currently only used in 2 custom paints, used by 2 people, so its not a priority

ye exactly anatole also told me on twitch its still kinda experimental but if my idea works they would be easy to implement, so why not.

Otherwise yeah, its not a priority and I'd happily say this PR is finished without them.

@AnatoleAM
Copy link

Animated paints are currently only used in 2 custom paints, used by 2 people, so its not a priority

Only 1 of them is actually animated, but both are WEBP images.

@LosFarmosCTL I updated the "Staff Shine" paint with a lower resolution file.

Otherwise yeah, its not a priority and I'd happily say this PR is finished without them.

Feel free to remove the draft status when you'd like us to check this out 👍

@LosFarmosCTL
Copy link
Author

@LosFarmosCTL I updated the "Staff Shine" paint with a lower resolution file.

Nice, I'll try that out later today, thanks.

Feel free to remove the draft status when you'd like us to check this out 👍

Still have some minor things and refactoring I'd like to do, but probably won't take longer than at most a few days.

@Melonify
Copy link

@LosFarmosCTL There's now a public image url paint, Crazy Confetti, just wanted to let you know just in case that changes whether or not you want to implement url paints in this patch.

@LosFarmosCTL
Copy link
Author

LosFarmosCTL commented Jun 13, 2022

There's now a public image url paint, Crazy Confetti, just wanted to let you know just in case that changes whether or not you want to implement url paints in this patch.

Yeah, I have just seen the reminder from anatole on twitch. I was planning to include non-animated URL paints anyways, only the animated stuff is problematic.

Already looked at the new paint, should not even need changes to what I have implemented locally right now, URL paints are just drawn above the normal username anyways, so that should all work just fine.

@TroyKomodo
Copy link

PagMan ?

@Felanbird
Copy link

PagMan ?

from unrelated upstream PR comment:
image

@LosFarmosCTL
Copy link
Author

Sorry for the lack of updates, as Felanbird reposted from upstream I have been out most of the time and basically only got home to sleep and chill a little, didn't want to actually work on anything.

I have updated the checklist in the PR description with the last things that need to be done and will have more time this weekend and next week to hopefully finish this PR.

@AnatoleAM
Copy link

Sorry I think this ci failure is my fault, my branch was affecting pull requests

@LosFarmosCTL
Copy link
Author

Figured out animated paints (havent separated the colon yet, I want to extract that code first to avoid duplications):

Chatterino 2022-06-26 at 14 37 01

Functionality wise this PR is ready now, I just want to refactor some more things, remove code duplication, clean up, etc. Might even be finished today, but shouldn't take longer than tomorrow to have everything ready for review 😄

@AnatoleAM
Copy link

image

Impressive work, can't wait to try this out

@AnatoleAM
Copy link

@Felanbird any idea why ci is still failing?

@Felanbird
Copy link

@Felanbird any idea why ci is still failing?

Looks like the same issue from the staging build, but I'm unsure why it's happening here.

@Felanbird
Copy link

but I'm unsure why it's happening here.

Actually, I have a feeling that due to the annotated tag created for the staging branch, when trying to use --tags, said branch is being borrowed - which causes the issue.
image

@AnatoleAM
Copy link

I deleted the tag since it's no longer needed anyway, hopefully that helps

@Felanbird
Copy link

I deleted the tag since it's no longer needed anyway, hopefully that helps

We are probably just better off stripping --tags from the main build, the issue from #81 seems to be resolved without the --tags variable
image

@TroyKomodo
Copy link

Feel free to move it to ready and ill review.

@TroyKomodo
Copy link

TroyKomodo commented Jun 27, 2022

Do you mind doing a git rebase on this branch?
The steps will be like

git checkout feat/paints
git remote add c7 https://github.com/SevenTV/chatterino7
git pull c7
git rebase c7/chatterino7
git push -f

Should be the output
https://pastebin.com/6TiVS0uc

The reason behind this is to remove things from this pr that are not supposed to be here.
Please only do it once your branch is ready to be reviewed.

@TroyKomodo
Copy link

TroyKomodo commented Jun 28, 2022

@LosFarmosCTL Seems good, whisper doesnt work yet tho.
image
And also sending whispers doesnt work might not be part of your pr since well nothing works on sending.
image
Very nice work!
Something i noticed

diff --git a/src/providers/seventv/paints/LinearGradientPaint.cpp b/src/providers/seventv/paints/LinearGradientPaint.cpp
index 77dc774e..49789526 100644
--- a/src/providers/seventv/paints/LinearGradientPaint.cpp
+++ b/src/providers/seventv/paints/LinearGradientPaint.cpp
@@ -59,8 +59,8 @@ QBrush LinearGradientPaint::asBrush(const QColor userColor,
 
     QPointF gradientStart;
     QPointF gradientEnd;
-    gradientAxis.intersects(colorStartAxis, &gradientStart);
-    gradientAxis.intersects(colorStopAxis, &gradientEnd);
+    gradientAxis.intersect(colorStartAxis, &gradientStart);
+    gradientAxis.intersect(colorStopAxis, &gradientEnd);
 
     if (this->repeat_)
     {

For whatever reason my version of qt had a different header files, not sure if this is documented could u provide some clarity on which is newest.

And also under building for linux, the package is not qt5-image-formats-plugin for ubuntu its qt5-image-formats-plugins
could u also update that in this pr.
Thanks @Felanbird

@TroyKomodo
Copy link

Oh and 1 more small side note, not a big deal
image
image
when we don't have paints enabled the colon is colored too.
Is there a reasoning behind not coloring it such as readability or something.

@TroyKomodo
Copy link

Other than that really good work man thank you a lot for your contributions please tag us in discord when u read this and we will set u up with the contributors badge and role <3 We really appreciate the effort on this!

@Felanbird
Copy link

Is there a reasoning behind not coloring it such as readability or something.

Looks to be to match webchat behavior | also efb4aec

image

@Felanbird
Copy link

And also under building for linux, the package is not qt5-image-formats-plugin for ubuntu its qt5-image-formats-plugins could u also update that in this pr.

#98

@LosFarmosCTL
Copy link
Author

For whatever reason my version of qt had a different header files, not sure if this is documented could u provide some clarity on which is newest.

Seems like it was called intersect up until Qt 5.14, actually not sure how to handle this since Qt 5.12 is still supposed to be supported. @Felanbird you know any examples from upstream for situations like that?

https://doc.qt.io/archives/qt-5.12/qlinef.html#intersect
https://doc.qt.io/qt-5/qlinef.html#intersects

Is there a reasoning behind not coloring it such as readability or something.

Yeah it's basically just me looking at how it's handled in webchat and matching that, since the paints extending over the colon in chatterino would change their appearance compared to webchat. IMO it only makes sense for nametags that actually do have a paint rendered on them, since the colored colon might conflict with the paints design (thats why I didn't change anything about how nametags without paints are drawn, didn't want to change c2's general design there)

whisper doesnt work yet tho.

Fixed what I could, for outgoing whispers + your own name when receiving there is no link information included, which is what we need to determine if whatever is drawn right now is actually a nametag element. Might make sense to add one, but that would be an upstream PR (if those are added, paints should be rendered correctly without further changes)

Other than that really good work man thank you a lot for your contributions please tag us in discord when u read this and we will set u up with the contributors badge and role <3 We really appreciate the effort on this!

Thanks a lot for that feedback :) This PR was actually really fun to work on, learned a ton of stuff while creating it as well.

@Felanbird
Copy link

Seems like it was called intersect up until Qt 5.14, actually not sure how to handle this since Qt 5.12 is still supposed to be supported. @Felanbird you know any examples from upstream for situations like that?

No idea, but it's likely we can sort of ignore 5.12 as it's already been dropped here due to to issues with the animated pfp build.
5.12 having issues with 2/2 of the Major PRs - as well as I believe Qt6 has been discussed more recently, so by the time chatterino7 -> chatterino2 happens we might just not have to deal with it. 😁

@TroyKomodo TroyKomodo enabled auto-merge (squash) June 28, 2022 07:21
@TroyKomodo TroyKomodo disabled auto-merge June 28, 2022 07:21
@TroyKomodo TroyKomodo merged commit 73a8a8d into SevenTV:chatterino7 Jun 28, 2022
@LosFarmosCTL LosFarmosCTL deleted the feat/paints branch June 28, 2022 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants