mokafolio / Tarp

176 stars 8 forks source link

add opacity to paint #17

Closed conversy closed 5 years ago

conversy commented 5 years ago

this enables to control the overall opacity without changing the color or all stop colors in a gradient.It adds a multiply operation in the shader though.

mokafolio commented 5 years ago

Hi,

I think this is a useful addition. Can I suggest the following things before merging though:

For solid paints, compute the color on the cpu (i.e. color * opacity) before setting the uniform on the shader.

For gradients, pass opacity as a separate uniform. Only read the uniform in the fragment shader. There is no point in reading it in the vertex shader and passing it along.

It might even make more sense to make an extra struct that hold opacity + gradient handle. That way the paint union won't get any bigger for solid fills and we'd only have to change the texture shader. You'd still be able to control a solid fills opacity through the alpha channel.

Hope that makes sense! Thanks for contributing!

conversy commented 5 years ago

for solid: ok for the compute on the cpu. Do you want me to change the patch? I'm not sure how I can do that (not that knowledgeable with github), but I can try...

mokafolio commented 5 years ago

All good, thank you! I merged it into a separate branch for now. I'll have a stab at the changes I suggested above throughout the day and then merge it into master.

mokafolio commented 5 years ago

Just did all the changes. I decided to only add an extra opacity field if gradients are used. I added tpPaintSetOpacity and tpPaintOpacity if you want to control paint opacity in a type independent way. Also cleaned up the texture/gradient shader in that regard. Initial commit for all of that is here: https://github.com/mokafolio/Tarp/commit/49d005af281c029919dcef1761222d019e8fb55d

Thanks for getting this started! Let me know if my changes make sense!

conversy commented 5 years ago

I could cope with your changes. Actually, in my use, sometimes Opacity is set before Color, and I do not know if it will be a solid color or a gradient. So I had to take that into account. I ended up to something like this, provided that alpha is 1.0 by default:

tpPaintSetOpacity(&style.fill, tpPaintOpacity(&style.fill) * alpha); // in case paint has been set before or style.fill = tpPaintMakeColor(r/255.f, g/255.f, b/255.f, tpPaintOpacity(&style.fill)); // in case opacity has been set before

This was not required with my original commit, but I think it works.

Btw, don't your (legitimate) remark on useless uniform in the vertex shader apply for meshColor as well?

mokafolio commented 5 years ago

Yes, good catch! I think the original version of the shader also had per vertex colors which were multiplied by the mesh color. I will change that!

Regarding the opacity. I actually think I might remove the opacity functions for the reason you are describing above. (You'd have to explicitly set the gradient opacity instead) I'd like to keep it that way to reduce ambiguity and not having to carry around an extra float in tpPaint that will almost never be used in the solid color case.

In your usecase, you'd basically have to know weather it is a solid color or a gradient before you could set the alpha/opacity. Does that make sense?

conversy commented 5 years ago

well actually the helper functions are actually helping me since they abstract out the way to set and get the opacity regardless of the type of paint. But I can use an externalized version of it.

mokafolio commented 5 years ago

Okay, I will think about it. Something about this does not sit right with me :)