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

Improve SamplingOptions, and adding test cases #240

Open
HinTak opened this issue Apr 21, 2024 · 7 comments
Open

Improve SamplingOptions, and adding test cases #240

HinTak opened this issue Apr 21, 2024 · 7 comments

Comments

@HinTak
Copy link
Collaborator

HinTak commented Apr 21, 2024

Cc @pavpanchekha

Pull #236 now includes the complete SamplingOptions code from pull #181, plus a little extra/update. The last of the emulation of the previous FilterQuality is:

  The last of SkFilterQuality emulation is:
  FilterQuality.kHigh_SkFilterQuality   -> SamplingOptions(CubicResampler.Mitchell())
  FilterQuality.kMedium_SkFilterQuality -> 
SamplingOptions(FilterMode.kLinear, MipmapMode.kNearest)    // cpu
                                           or SamplingOptions(FilterMode.kLinear, MipmapMode.kLinear)  // gpu
  FilterQuality.kLow_SkFilterQuality    -> SamplingOptions(FilterMode.kLinear, SkMipmapMode.kNone)
  FilterQuality.kNone_SkFilterQuality   -> SamplingOptions(SkFilterMode.kNearest, SkMipmapMode.kNone)

So it turns out that the 4 filterquality settings is equivalent to 5 of the 2×3x2 =12 SamplingOptions combo (one extra, because one of them depending on whether it is rendering through CPU or GPU), and we might as well add the whole thing.

Please test the build artefact when it finishes building.

Still missing are some test code, and possibly docstring documentation too.

@HinTak
Copy link
Collaborator Author

HinTak commented Apr 21, 2024

@pavpanchekha the above snippet (duplicated from inline comments) will go into the m124 release note when it gets written. For now, this snipplet is the only "migration" guide. If you could contribute some test code (to go into the pytest "tests" directory), preferably using images from skia's resources/images directory (most of our current tests are written that way, using test files from skia/resources/), that would be useful.

@HinTak
Copy link
Collaborator Author

HinTak commented Apr 27, 2024

#236 now contains tests for the 5 combos which corresponds to the old m87 FilterQuality settings. More to do, but these are sufficient to satisfy migration needs from m87. #236 can go out, while this stays open.

@pavpanchekha
Copy link
Contributor

Just to clarify—the old FilterQuality options will no longer be supported, and users should migrate to SamplingOptions?

@pavpanchekha
Copy link
Contributor

What kind of tests would be useful? Actually checking that images have been resized with various quality levels? Or just checking that drawing an image with a given sampling quality is possible?

@HinTak
Copy link
Collaborator Author

HinTak commented Apr 30, 2024

Yes:
https://github.com/HinTak/skia-python/blob/m124-public/relnotes/README.m124.md

For now I have only added those which had m87 equivalent, and check that the 6 are valid:
https://github.com/HinTak/skia-python/blob/m124-public/tests/test_samplingoptions.py

Some actually non-trivial tests closer to how users might use them would be nice.

@pavpanchekha
Copy link
Contributor

Just a nit—in your migration instructions above you wrote SkFilterOptions and SkMipmapMode a few times, it's not supposed to have the Sk prefix.

@HinTak
Copy link
Collaborator Author

HinTak commented Apr 30, 2024

Yes, I realised the extra sk afterwards - 7f6e2bb - the readme is copied from the tests/tests_samplingoptions.py and should work literally (after adding some "skia." prefix, or doing from skia import * [Not recommended, as there might be collisions/shadowing]).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants