search

mypaint / libmypaint

libmypaint, a.k.a. "brushlib", is a library for making brushstrokes which is used by MyPaint and other projects.
http://mypaint.org
Other
310 stars 86 forks source link

Fix changing alpha with alpha lock #180

Closed gwojcik closed 3 years ago

gwojcik commented 3 years ago

Deleted line should not change value of alpha. This line, with lossless precision, can be written as: alpha = alpha*mask*opacity + alpha*(1 - mask*opacity) and simplified to: alpha = alpha * 1 but use of fixed point math introduces loss of precision.

Problem can be reproduced in MyPaint:

  1. paint some shape with less than 100% opacity
  2. enable alpha lock: Menu -> Brush -> Paint mode -> alpha lock
  3. paint over shape witch alpha lock
  4. after some time opacity will start to decrease, it is more visible on layers with pigment mode
jplloyd commented 3 years ago

Thanks!

Yeah, looks like a copy/paste error to me. @briend can you confirm this, or was there a reason for changing the destination alpha?

briend commented 3 years ago

https://github.com/mypaint/libmypaint/blob/master/mypaint-tiled-surface.c#L548

I’m not sure this will work. . .the modes aren’t Boolean even though lock alpha probably should be. So, what if lock_alpha is 0.5, shouldn’t the alpha only be locked halfway? 🤯

jplloyd commented 3 years ago

Ah, that's true. I forgot that it's not an on/off setting. I guess a special case could be made for 100% alpha lock (which is, realistically, what will be used most of the time), though that means either adding a parameter and a conditional, or another mostly identical function.

Having a setting called "Lock Alpha" not be boolean is definitely not very intuitive. Another breaking change to consider for libmypaint 2.0 I think.

gwojcik commented 3 years ago

If I correctly understand, change in destination alpha is caused by rounding error in fixed point math. Change in alpha is not depending on function parameters. In my tests i most cases integer value of alpha is changed by 1.

briend commented 3 years ago

Oh can you try just adding 0.5 to the alpha value to avoid the rounding down issue with float->int? We do that elsewhere maybe it’s just missing here.

gwojcik commented 3 years ago

new commit fixes rounding error. I think line that sets rgba[3] could be deleted, because value of rgba[3] will not change.

jplloyd commented 3 years ago

Sorry for the delay. I think you are correct, and that your original patch is the better solution. If you update the PR with the old patch I will merge it right away (and backport it to the 1.x branch).

gwojcik commented 3 years ago

I reverted patch to original version.