At the moment this is broken because QImpagePyramid (which KisDabCache uses for
caching transformed brush tips) only supports the same scale factor for both
X & Y axes. This causes the cached image to be cliped while drawing.
Details
Details
- Reviewers
dkazakov rempt - Maniphest Tasks
- T2826: Bind ratio to the sensor system
Diff Detail
Diff Detail
- Repository
- R37 Krita
- Lint
No Linters Available - Unit
No Unit Test Coverage
Comment Actions
Hi, @nishantjr!
Can I ask you, why do you try to bypass the cache? You can just fix the cache to take the *largest* mipmap plane and scale it as needed. This code is a bit complicated, because it ensures the high quality of any brush from 1px to 1000px scaled to any value.
Comment Actions
Hey
@rempt Sorry about taking so much time about this.
I've finally finished the Ratio Option implementation along with a fair amount of
cleanup and pushed it up to the the nishant/dab-cache-cleanup-and-ratio-option branch. I've got the implementation working so that AutoBrush's "spikiness" increases when using this option, but other image based brushes are just scaled.
I've tested it with the auto brush and the "predefined images", and it looks good to me. You can do some neat things with the auto brush like: http://pasteboard.co/1200KZjdP.png
@dkazakov, In the new changeset I'm still using much of the DabCache code, though I've disabled the actual caching, because it was defeated by renderMirrorMaskSafe, and anyway, the expensive bit wasn't the image generation, but the allocation of memory -- it seems to take a while to allocate memory for large dabs (and several operations involve allocating and freeing large chunks of memory). If you'll like the changes so far, I could start work on using some sort of memory pool reduce those allocations.
I've stopped using the QImagePyramid, since it seemed redundant to peform two transformations with one was enough. I've reimplemented the subpixel smoothing fix in KisBrush, and I've come up with another, simpler solution for the jagged edges created sometimes during rotation. I've added tests for these too.
Let me know what you'll think and if there's any changes you'd like me to make!
@dkazakov Should I file a Qt bug regarding the Subpixel scaling issue?
Comment Actions
I tested the branch and there were artefacts visible to me, and if anything, it felt a bit smoother even. The
krita(19443)/(krita.general) unknown: KisBrushOp: dab bounds is not dab rect. See bug 327156 QSize(65, 65) QSize(56, 57)
messages still happen, and I am not sure how much of a problem this highlights.
Comment Actions
Hi, @nishantjr!
I have tested your branch and I'm sorry to say that some of your ideas (especially about removing the pyramid and the cache) were wrong. The problems are the following:
- You removed painter()->renderMirrorMaskSafe() calls from the painting code. That obviously breaks mirrored painting (activate it using the buttons in the tool bar)
- Removing the Pyramid causes two problems:
- The quality of big predefined brushes became low. Check this preset for example . It happens because Qt uses bilinear sampling algorithm, while the pyramid uses more complex algorithm with more data available.
- The speed of painting with big predefined brushes (300+px), when they are scaled down (by the pressure sensor) to something like 10px, is extremely low. That happens because on each dab you should iterate through the whole 300x300 image instead of a smaller version of it.
- The quality of big predefined brushes became low. Check this preset for example
- Removing the cache causes two problems as well:
- You dropped all the code that did subpixel precision. That code was extremely complicated, because it also handled the case when the canvas it mirrored/rotated. Right now, painting with a 0.5px brush on a mirrored canvas is simply wrong. Check this preset for example:
- Dropping the cache slows down the brushes, which work on lower precision level, say 3.
- You dropped all the code that did subpixel precision. That code was extremely complicated, because it also handled the case when the canvas it mirrored/rotated. Right now, painting with a 0.5px brush on a mirrored canvas is simply wrong. Check this preset for example:
The only solution I can see now is:
- Recover the pyramid and the cache.
- Implement the ratio transformation on the top of the cache, not instead of it. Just make ensure that the cache can handle separate values for X and Y scales correctly. That is the only thing needed.
With the regressions listed the branch cannot be merged into master :(
Comment Actions
Hi @dkazakov,
OK, noted. Thanks for taking the time to explain things. Could you point
out the location in the code where KisQImagePyramid's sampling algorithm is
implemented?
I've started a new branch with an implementation of the ratio
transformation via KisQImagePyramid and not try anything too clever. I'd
like to group angle, ratio and rotation into a single class called
"KisDabShape" since these are passed around together a lot and would be
prone to mixing up the order of parameters. Makes sense?
Comment Actions
Hm... it seems I was wrong (mixed with KisImagePyramid) and the QImage pyramid is always generated from the original image using bilinear transformation in the ctor of KisQImagePyramid. Anyway, when the scaling down happens in the iterative approach (first to mipmap level, then to the actual zoom level), the quality is usually much better (unless we use specially crafted filtering for it).
I've started a new branch with an implementation of the ratio
transformation via KisQImagePyramid and not try anything too clever. I'd
like to group angle, ratio and rotation into a single class called
"KisDabShape" since these are passed around together a lot and would be
prone to mixing up the order of parameters. Makes sense?
Yes, grouping the values seems to be a perfect solution!
Comment Actions
Hi Dmitry, Boud,
I've pushed up a new branch (nishant/ratio-option) that's got the new
implementation. I've introduced the new KisDabShape class (though I'm
wondering if KisDabTransforms is better). I still need to fix the dab
outline when the ratio option is being used, though there shouldn't be any
regressions. Let me know what you'll think.
Dmitry, I was wondering why the KisBrush class hierarchy aren't just
subclasses of KisBrushOp?
Comment Actions
Hi, @nishantjr!
I have tested your patch! Good news are that there are no regressions anymore. Bad news are that I cannot activate the ratio option... :( It looks as if it doesn't work. I'm not sure what is going wrong, but it just doesn't change brush shape in any way :( See the attached video:
Comment Actions
Hey Dmitry,
Give m an hour to check it out. I had pushed another commit yesterday that
got it working for the non gaussian auto brushes, do you have those in your
build?
Comment Actions
Hi, @nishantjr!
Hm... good question. Most probably, I didn't have that commits. I was at 432003f. Let me check further.
Comment Actions
Hi, @nishantjr!
I have tested the latest version of the branch and now it works perfectly fine! :) I'm ok with merging it into master!
When you pushed the commits please add Krita: Manual and Krita: Website and Translations projects as an auditors for the commit that adds a feature. That will let the people know to write a manual entry and a twitter post about the new feature :)