Add thumbnail images to channel docker
AbandonedPublic

Authored by eingerman on Jun 15 2016, 4:31 PM.

Details

Reviewers
dkazakov
rempt
Summary
  1. Added thumbnail to channed docker . This is what it looks now:


  1. Changed selection behavior. Checkboxes work they way they used to. However, double clicking on thumbnail or channel name will deselect all other channels (except alpha).

Thumbnail generation uses fast (but not very accurate) algorithm and is done in a timer thread, so it should have minimal impact on overall ui responsiveness. Profiling shows negligible time used generating thumbnails in regular use.

Test Plan
  1. Enable channel docker. Observe thumbnail images.
  2. Change image color space (to say CMYK or Lab). Observe thumbnails labels change.
  3. Select/deselect channels.
  4. Double click thumbnail image (should only keep this channel and alpha channel selected).

Diff Detail

Repository
R37 Krita
Lint
Lint Skipped
Unit
Unit Tests Skipped
eingerman updated this revision to Diff 4513.Jun 15 2016, 4:31 PM
eingerman retitled this revision from to Add thumbnail images to channel docker.
eingerman updated this object.
eingerman edited the test plan for this revision. (Show Details)
eingerman added reviewers: rempt, dkazakov.
eingerman set the repository for this revision to R37 Krita.
rempt edited edge metadata.Jun 15 2016, 9:04 PM

As I said on irc, KisPaintDevice::createThumbnailDevice should be used. I like this approach: it's better that what we have there, and if the approach is used there, it'll improve Krita in other places, like the overview docker or the layer thumbnails.

That said -- Dmitry's remarks about the fastest way to iterate over pixels are relevant, too.

plugins/dockers/channeldocker/channelmodel.cpp
205

If this is a faster/better way of creating thumbnails then this should be implemented in KisPaintDevice::createThumbnailDevice, and that method should be used here. I do like the idea of taking 4x size version and scaling that, instead of the pure nearest neighbour code of KisPaintDevice::createThumbnailDevice -- that is well worth pursuing.

eingerman updated this revision to Diff 4602.Jun 18 2016, 6:41 AM
eingerman edited edge metadata.

Added benchmark for thumbnail generation.
Modified channel thumbnail code to match the fast implementation.
Benchmark shows 20ms to generate thumbnails for 6000x8000 pixel image on my 3 year old laptop. This is a bit slower than createThumbnail call due to oversampling. However, quality is much improved due to antialiasing.
Method is based in part on the discussion with Dmitry.

rempt requested changes to this revision.Jun 18 2016, 11:01 AM
rempt edited edge metadata.

As I said, an improved way of generating thumbnails is awesome: this code should be integrated in KisPaintDevice::createThumbnailDevice so it gets used automatically everywhere.

plugins/dockers/channeldocker/channelmodel.cpp
211

Mind your coding style: a space goes between if and (, but not between ( and the condition.

This revision now requires changes to proceed.Jun 18 2016, 11:01 AM
eingerman updated this revision to Diff 4617.Jun 18 2016, 4:27 PM
eingerman edited edge metadata.
  1. In this patch I rewrote the thumbnail generator algorithm for channel docker. My implementation in the first version of this patch was actually a lot slower than createThumbnail. Mea culpa, please ignore it. Current implementation uses KisPaintDevice::createThumbnailDevice.
  1. channelDocker cannot use createThumbnail, because createThumbnail returns RGB8 QImage. Instead, channelDocker needs each color plane as a grayscale images (original image may be CMYK, or XYZ - different color spaces than rgb). Creating multiple grayscale images is done in ChannelModel::updateThumbnails.
  1. The only useful improvement I can contribute to KisPaintDevice::createThumbnail from channelDocker is antialiasing. However, it slows down createThumbnail a bit and also it can be accomplished in 1 line in code using createThumbnail e.g:

image = m_dev->createThumbnail(OVERSAMPLE * THUMBNAIL_WIDTH, OVERSAMPLE * THUMBNAIL_HEIGHT);
image = image.scaled(THUMBNAIL_WIDTH, THUMBNAIL_HEIGHT, Qt::KeepAspectRatio, Qt::SmoothTransformation);

  1. I fixed indentation in this patch using kdelibs astyle.
rempt added a comment.Jun 18 2016, 5:46 PM

I am not talking about createThumbnail, but about

KisPaintDeviceSP createThumbnailDevice(qint32 w, qint32 h, QRect rect = QRect()) const;

Which gives you a thumnail-sized KisPaintDevice in the original colorspace, which is exactly what you want.

dkazakov requested changes to this revision.Jun 20 2016, 1:16 PM
dkazakov edited edge metadata.

Hi, @eingerman!

The feature itself is really nice, but I would really like to have the coding style to be get in sync with what we use in Krita. The main problem is auto's. They should be used in an emergency case only.

There is also a "bug" that the thumbnails are updated even when the docker is not visible. That may create troubles for the users when they so the quick painting and even don't need the channels at all.

benchmarks/kis_thumbnail_benchmark.cpp
132

Q_FOREACH is still preferrable

plugins/dockers/channeldocker/channeldocker_dock.cpp
90

Please make sure if the docker is visible when this signals arrives! We should not waste the cpu time when the user even doesn't see the docker.

And don't forget to update the docker state when it gets shown on screen finally! :)

plugins/dockers/channeldocker/channelmodel.cpp
106

Please avoid using autos except when working with iterators in the loops. See HACKING file in the root of Krita source tree.

(Which reminds me, we should update the wiki article about the C++14 usage)

218

Usage of qreal is preferrable over float, because QSize multiplication operator has only 'qreal' operator overloads, not 'float'.

This revision now requires changes to proceed.Jun 20 2016, 1:16 PM

Hi, @eingerman!

This one was also merged, right?

eingerman abandoned this revision.Aug 17 2016, 4:16 PM

Yes, this change was made part of D1979 revision. Closing.