Skip to content

Include a background to all canvas previews #206

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

Closed
novhack opened this issue Apr 16, 2020 · 9 comments · Fixed by #318
Closed

Include a background to all canvas previews #206

novhack opened this issue Apr 16, 2020 · 9 comments · Fixed by #318
Labels
change Proposes changes on how something works enhancement New feature or request

Comments

@novhack
Copy link
Contributor

novhack commented Apr 16, 2020

Currently previews in the export dialog and in the timeline frames have all transparent backgrounds so the drawings are with some themes hard to see.
I propose to add a checker board background to all previews mentioned (or more solid color for timeline frame previews).
Screenshot_20200417_005000
It should be considered if it's actually necessary to show a preview in the frame button since it mostly becomes unrecognizable (maybe a simple glyph that signifies that there is something in the layer would be better and more visible).
Screenshot_20200417_004949

@OverloadedOrama OverloadedOrama added change Proposes changes on how something works enhancement New feature or request labels Apr 17, 2020
@dphaldes
Copy link
Contributor

We can use the shader from #210 for the background.

@matthewpaul-us
Copy link
Contributor

matthewpaul-us commented Aug 25, 2020

I was poking around the code for this, and I see that flip image already has a transparent checker background. I tried replicating the setup on the desaturate as a test, but it seems that the desaturation action causes the transparent checker to be affected by the desaturation as well. This might not be too bad for desaturation, but some of the other dialogues it could be wierd. Do we know why this filter would affect both the image and the transparent checker?
image

@matthewpaul-us
Copy link
Contributor

Also, looking at the project, it seems like this could be tied in pretty tightly with the project card related to effect previews.

Is this something I should hold off on for the effects? I can do the animation cels and the export dialog only and leave the effect previews for @OverloadedOrama.

@OverloadedOrama
Copy link
Member

I'm not sure why the desaturation effect affects the checker too. I'm going to replicate it too soon, I'll see if there's something wrong. Right now I'm working on creating an ImageEffect parent class from which most image effects will inherit. This'll make editing stuff that are common for image effects much easier. Then, I'll add checker backgrounds for the rest of the effects.

Feel free to do the animation cels and the export dialog if you want @matthewpaul-us!

OverloadedOrama added a commit that referenced this issue Aug 25, 2020
Also made them inherit ImageEffect, resulting in much less and cleaner code. Only RotateImage remains. Partially addresses #206.
@OverloadedOrama
Copy link
Member

I managed to put a checker background to the rest of the image effects (except rotation) in f121c39 and it seems to be working fine. The filters do not affect the background from what I see, including the desaturation effect.

@dphaldes
Copy link
Contributor

@OverloadedOrama The solution you mentioned in my PR is the most modular one in my opinion and I think that is how it should be implemented. It also won't affect the image that way.

@OverloadedOrama
Copy link
Member

@OverloadedOrama The solution you mentioned in my PR is the most modular one in my opinion and I think that is how it should be implemented. It also won't affect the image that way.

Indeed, that is what I ended up doing.

@matthewpaul-us
Copy link
Contributor

That's odd, as I think I did what was in the PR (checker as a child of preview, with show behind parent set). After the code gets merged, I'll take a look and see what I didn't do correctly. Has that code been merged yet?

I think adding a parent class for the image effect would be great!
image

@OverloadedOrama
Copy link
Member

Yeah the code has been merged in the latest commit in master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change Proposes changes on how something works enhancement New feature or request
Projects
None yet
4 participants