skia-python icon indicating copy to clipboard operation
skia-python copied to clipboard

Paint.setXYZ methods doesn't accept nullptr (None)

Open sherlockdoyle opened this issue 4 years ago • 2 comments

Describe the bug Several setter methods of skia.Paint are documented to accept nullptr to remove the filter/effect from the paint. However, this is not the case as passing None to these methods throws a type error. This is because these methods are pybind-ed with a lambda function. The methods listed below have this problem.

To Reproduce Steps to reproduce the behavior: Run the following code

import skia

paint = skia.Paint(
    ImageFilter=skia.ImageFilters.Blur(1.0, 1.0),
    ColorFilter=skia.LumaColorFilter.Make(),
    MaskFilter=skia.MaskFilter.MakeBlur(skia.kNormal_BlurStyle, 1.),
    PathEffect=skia.DashPathEffect.Make([2., 1.], 0),
    Shader=skia.Shaders.Empty(),
)

print(paint.getImageFilter())  # prints <skia.ImageFilter object at 0x...>
paint.setImageFilter(None)  # this works
print(paint.getImageFilter())  # prints None

paint.setColorFilter(None)  # fails with TypeError: setColorFilter(): incompatible function arguments. The following
                            # argument types are supported: # 1. (self: skia.Paint, colorFilter: SkColorFilter) -> None
paint.setMaskFilter(None)  # fails with TypeError
paint.setPathEffect(None)  # fails with TypeError
paint.setShader(None)  # fails with TypeError

Expected behavior Each of these methods should accept None, execute without error, and remove the corresponding filter/effect.

Desktop (please complete the following information):

  • OS: Ubuntu 20.04.1 LTS
  • Python: 3.8
  • skia-python version: 87.3

Additional context Note that paint.setImageFilter(None) works as expected. This is because setImageFilter is not pybind-ed with a lambda function.

sherlockdoyle avatar Oct 17 '21 10:10 sherlockdoyle

@kyamagu What was the philosophy behind these setXYZ functions? Why are they copying their parameters (with serialize and deserialize)? From what I understand, we can fix this bug by removing all these copying (which also slows execution). Being objects, it should be the user's responsibility to handle references.

sherlockdoyle avatar Nov 17 '21 18:11 sherlockdoyle

@sherlockdoyle Copying is to workaround the complicated ownership between python and C++. In pybind11, objects are owned by python, and can be destroyed outside of the binding. For this reason, any non const pointer argument to the function first clone the object. Serialize/deserialize is simply a cloning operation.

kyamagu avatar Nov 18 '21 00:11 kyamagu