Show and configure a reference vanishing point grid

Authored by scottpetrovic on Apr 14 2018, 4:22 PM.


Group Reviewers
Maniphest Tasks
T8652: Krita 4.1.4 Release Task

A lot of other painting applications have this and I thought it would be useful. This patch adds reference lines to the vanishing point assistant. It also adds a slider for people to change the angle increment for it. This slider is in the tool options.

I had to do a couple other things to make this possible.

  1. Add the concept of a "selection" for assistants. The angle option will only appear in the tool options if a vanishing point assistant is selected
  2. Added an ability to have assistant specific properties. No other assistant types have an angle slider for example

I also added the ability to load/save from a KRA....and load/save from a painting assistant file with this new custom property

These lines are part of the "preview", so turning off the assistant preview will hide these lines if people don't want to see them.

Test Plan

Tested quite a bit with different selections and making sure the state updated correctly with showing/hiding. This includes selecting other assistants, deleting an assistant, adding an assistant.

Loaded and saved KRA file

Loaded and saved painting assistant file

Played around with the angle slider and made sure it updated on the canvas.

Diff Detail

R37 Krita
Lint Skipped
Unit Tests Skipped
scottpetrovic requested review of this revision.Apr 14 2018, 4:22 PM
scottpetrovic created this revision.
scottpetrovic edited the summary of this revision. (Show Details)Apr 14 2018, 4:27 PM

Hi Scott, good idea and nice work. I agree that this is a nice improvement to the way vanishing point assistants are visualized!

Having applied the patch I do have a few notes though:

1.) Right now changes in density don't show up until you mouse over the canvas, change the opacity, etc. Not a big deal, but it'd feel a bit nicer if the canvas refreshed immediately when the slider is changed (when the value is set, that is).

2.) Hiding/Deactivating the assistant (clicking on the little eye) doesn't seem to hide the reference lines for me.

3.) While small, in my opinion it'd be nice for user feedback if the currently selected assistant was visualized in some way while they're using the Assistants Tool. For example, maybe changing the color of the assistant itself, or maybe changing the color of the assistant's controller 'gizmo' - just some small way of showing the user "you've clicked on this thing and now it's selected". You know?

At any rate, it looks nice!

Great ideas @emmetoneill !

I was pretty happy to just get this working, so I think your suggestions helps the UX quite a bit with selections and realtime updates.

I made all of your updates. Let me know if it is better

Working very nicely now! Density updates responsively, hiding works, and the selection highlighting is clear and looks great.

Good job again, Scott. =]

On my system I should apply this patch to make it build:

1diff --git a/libs/ui/kis_painting_assistants_decoration.cpp b/libs/ui/kis_painting_assistants_decoration.cpp
2index 08f827a..f3ef3e8 100644
3--- a/libs/ui/kis_painting_assistants_decoration.cpp
4+++ b/libs/ui/kis_painting_assistants_decoration.cpp
5@@ -305,7 +305,7 @@ void KisPaintingAssistantsDecoration::setSelectedAssistant(KisPaintingAssistantS
7 void KisPaintingAssistantsDecoration::deselectAssistant()
8 {
9- d->selectedAssistant = 0;
10+ d->selectedAssistant.clear();
11 }

I have built it, but didn't test yet :)

dkazakov requested changes to this revision.Apr 18 2018, 8:39 AM

Hi, @scottpetrovic!

I have tested the patch! It works fine, saves data to .kra fine. There are two minor issues with the code, please fix them and then we can push that to master then :)


This code is a bit confusing architecture-wise. getCustomPropertyFloat() hints the reader that the framework for the properties is uniform and can be reused by other assistants, but the tool still manually check for the assistant name and property name. I guess the code should either be simplified to dynamic_cast'ing to VanishingPointAssistant and just requesting this property directly via explicit function call, or a complicated system for "abstract properties" should be done, like in KisSliderBasedPaintOpProperty.

I guess, for this specific case, it would be easier to just use dynamic_cast. In contrast to these manual checks it delegates compiler to check stuff for us :)


Please use KisDomUtils for reading and writing property data. Using QString::number() and QString::toFloat() will not work in languages that have comma as a decimal separator (e.g. German or Russian).

You can either use KisDomUtils::toString()/toDouble() or use more hi-level routines like KisDomUtils::saveValue()/loadValue(). Since the rest of the code doesn't use save/loadValue(), I guess you should just use toString()/toDouble().

This revision now requires changes to proceed.Apr 18 2018, 8:39 AM

Thanks for the review @dkazakov

I am hitting a snag trying to refactor it. All of the XML loading and saving for the assistant is done in the base class of the painting assistant, so I am not sure if I can use derived classes in a base class when saving a property that only exists in one derived class. See this situation in the code when saving the XML...

QByteArray KisPaintingAssistant::saveXml(QMap<KisPaintingAssistantHandleSP, int> &handleMap)

QByteArray data;
QXmlStreamWriter xml(&data);
xml.writeAttribute("active", QString::number(d->isSnappingActive));

if (d->id == "vanishingpoint") {
    VanishingPoingAssistant vpAssistant;  // having a hard time doing this since Vanishing point derives from this class. I would have to re-implemet the saving and loading XML stuff elsewhere

I am having a hard time typecasting or referencing the vanishing point assistant type to get its special property.

Hi, @scottpetrovic!

To overcome the issue of saving custom properties you can use "method extraction" refactoring pattern. Just define a virtual function that saves/loads some custom assistant information,and call it from the main KisPaintingAssistant::saveXml(). LIke that:

// **********************************************
// in KisPaintingAssistant
// **********************************************
QByteArray KisPaintingAssistant::saveXml(QMap<KisPaintingAssistantHandleSP, int> &handleMap)
    QByteArray data;
    QXmlStreamWriter xml(&data);
    xml.writeAttribute("active", QString::number(d->isSnappingActive));
    Q_FOREACH (const KisPaintingAssistantHandleSP handle, d->handles) {
        // ...skipped...


    return data;

// **********************************************
// in VanishingPointAssistant
// **********************************************
virtual void VanishingPointAssistant::saveCustomXml(QXmlStreamWriter &writer) 

    xml.writeAttribute("value", KisDomUtils::toString(realAngleDensityValue));


virtual bool VanishingPointAssistant::loadCustomXml(QXmlStreamReader &reader) 
    // read stuff
    return true;

thanks @dkazakov for the tips/suggestions

I made all those changes and the code is a lot cleaner with this new "extraction method pattern". It seems to still work well with the loading and saving after I made these changes.

scottpetrovic marked 2 inline comments as done.Apr 26 2018, 8:15 PM

seeing if I can mark those two comments as "done"

dkazakov accepted this revision.Apr 28 2018, 3:24 PM

Hi, @scottpetrovic!

Now the patch works and look fine! Thank you for your work! Please push! :)

This revision is now accepted and ready to land.Apr 28 2018, 3:24 PM
scottpetrovic closed this revision.Apr 29 2018, 6:07 PM

Great! I just pushed it out. Closing this patch