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

459 Layer / Keyframe opacity #1355

Merged
merged 30 commits into from
Mar 10, 2021
Merged

Conversation

davidlamhauge
Copy link
Contributor

@davidlamhauge davidlamhauge commented May 21, 2020

Here is how I think layer opacity should be implemented in Pencil2D.
As usual, I've not speculated on how software x, y or z has made their implementation. I've made it so it follows what I consider the philosophy of Pencil2D.
Tell me what you think,and let's make this feature available for our users soon.
closes #459
https://youtu.be/LcY6M6PKfFQ

@Jose-Moreno
Copy link
Member

@davidlamhauge We already talked about this in the discord server and I think most of the feedback got in, so that's good.

The only thing i'd like to suggest is to invert the layer opaque / transparency values. Switch the value for full opacity to be 100% and the value for transparency to be 0%. This is to keep consistency with the color box alpha slider and the onion skin transparency % spinboxes.

Other than that, great job! 🎉

@Jose-Moreno
Copy link
Member

Jose-Moreno commented May 22, 2020

Just as an afterthought, I'd like to ask some consideration regarding potential future enhancements related to this feature, at least after this is merged:

  1. The layer opacity "slider" should always be visually related to the layers, in this case what could work is to have a similar pop-up menu to the one David implemented for the layer color switcher as part of his layer highlight color PR
  2. That the individual frame opacity has an isolated slider that can be invoked when right clicking any given keyframe.
  3. Have the "fade effect" separated into a new (Layer) Effects menu. I think a simple effects menu could hold stuff like the "line recolor" feature as well as other simple animation utility effects in the future.

Maybe No.3 could even be dealt with as soon this PR is merged, as it'd only require moving the related action buttons to a new menu. Let me know what you guys think 🙂

@davidlamhauge
Copy link
Contributor Author

Good to read your remarks @Jose-Moreno .
If we reverse the values, so 0% is fully transparent, then it's not an 'opacity slider', but an 'opaqity slider'. I prefer it as it is, but it can be reversed, if that's the general feeling.
About integrating the features in separate places, I can see pros and cons:
Pros)
You have the features where they logically belong, and everything seems/feels good.
Cons)
I see an avalance of questions like "How do I fade in my background?" or "Layer opacity is great, but what if I only want two of my frames to be semi-transparent?".

By keeping the features together, I think it's more intuitive to use, to understand what to do, and to know how you accomplish what you want.
A final remark: Since we still hope for a timeline rewrite, I've deliberately avoided to much integration with the current timeline.

@MrStevns
Copy link
Member

MrStevns commented May 22, 2020

I agree with Jose about the 0% being transparent and 100% fully opaque, it should follow the same principle as the alpha color inspector slider.

@chchwy
Copy link
Member

chchwy commented May 22, 2020

We can call it transparency slider rather than opacity slider to avoid the confusion

@MrStevns MrStevns self-requested a review March 7, 2021 18:21
@MrStevns MrStevns self-assigned this Mar 7, 2021
@MrStevns
Copy link
Member

MrStevns commented Mar 7, 2021

FYI. i'm currently working on this.

- Removed a lot of duplicate code
- Removed unused code
- Refactoring...
- Disable ability to change opacity for selection when no frames are selected.
Not even sure what I was thinking here.
Copy link
Member

@MrStevns MrStevns left a comment

Choose a reason for hiding this comment

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

After making changes based on my own usage of the feature, tweaking the functionality and fixing a couple of bugs as well as simplified the code.

I think it's good to go.

@MrStevns
Copy link
Member

MrStevns commented Mar 8, 2021

I will do one final test tomorrow and if everything go as planned, then I'll merge it.

@Jose-Moreno
Copy link
Member

Jose-Moreno commented Mar 9, 2021

@candyface I was briefly testing this. It's interesting how you've polished the behavior. It also feels more stable.

While using I was wondering if the layer should be a panel rather than a floating window though. It seems I have to keep it open consistently when changing the layer opacity for my own use and now it doesn't feel like a visual effect you apply on a whim anymore, but rather a vital part of the layer & frame management on the interface. Even the individual frame opacity part of this feature seems to benefit from having the window open to see he relative opacity per frame.

One UX bug I ran into was that using the layer opacity would change all frames correctly, but if you duplicate or create a new frame after this, the layer opacity is set to 100% which is inconsistent with the currently applied layer opacity and could cause confusion. I think new frames should inherit the current layer opacity.

Let me know what you think about this 👍

@MrStevns
Copy link
Member

MrStevns commented Mar 9, 2021

That's a good point about the keyframe duplication not inheriting the opacity. I will look into that.

As for the floating window vs panel discussion, As it is currently, I think it's taking up too much space to be a docked widget, however I do think eventually that we should move it to a smaller widget, and separate the fade in/out logic from the widget.

@Jose-Moreno
Copy link
Member

@candyface That's alright. I'd rather not ask you to change much so this PR can get through soon. I don't particularly dislike how it's working right now from a UI standpoint and all of these ideas can nurture the timeline / interface redesign later on.

@MrStevns
Copy link
Member

MrStevns commented Mar 9, 2021

The opacity inheritance problem should be fixed now. I will once again wait till tomorrow with merging.

@Jose-Moreno
Copy link
Member

@candyface That's great 😄 I was just thinking right now, do you think for a future PR follow-up it could be possible to have the fade-in / fade-out use the interpolation settings applied to the rendering? That way we wouldn't need to have extra duplicate frames just to create a smooth transition 😁


As a side note I'm not sure however how much work would other layer types require to have their base types allow for this kind of rendering interpolation, particularly thinking to link that with actual motion interpolation for raster and vector.

I've been thinking that bitmaps might require a vector surface that displays the bitmap after it's been drawn and "fixed" to the canvas, but then the blitting or whatever the canvas painter is doing, would have to occur to that vector surface first and then to the canvas itself 🤔

Anyway thanks again, great work both of you! 🙇

This should probably not be emitted from the layer model.. although it's convenient, maybe we should do this from timelinecells instead?
@MrStevns
Copy link
Member

Alright here goes!

@MrStevns MrStevns merged commit 30ed7eb into pencil2d:master Mar 10, 2021
@MrStevns
Copy link
Member

@davidlamhauge thanks for being patient :)

@MrStevns MrStevns added this to the v0.6.7 milestone Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Opacity slider for Layers
5 participants