Ruler is not actively drawn when zoom is large about xournalpp HOT 40 CLOSED

Thanks! The only improvement I can see on this is to reduce the redraw lag as much as possible.

from xournalpp.

This may have reappeared when using the highlighter on the Saucy Deb.

from xournalpp.

It happens with the highlighter no matter if the zoom is big or small --- only in freehand. If drawing straight lines all is ok. Saucy deb 1.0.0 i386.

from xournalpp.

Strange. In the 64bit version of the packages, the problem does NOT show up.

from xournalpp.

Well, I can't reproduce the error. Everything works under x86_64 / i386 as far as I am concerned. What do you mean by freehand? The ruler is only ever drawn if the straight light options is enabled...

from xournalpp.

I think he's talking about the highlighter tool. Because this doesn't appear when the straight line option is selected, I'm thinking this is not the same bug. Perhaps related to bug #42.

from xournalpp.

I have the error only on the 32bit. Choose the highlighter, freehand (no ruler selected); start drawing. The highlight mark appears only when you end the trace. Easier too see than to explain:

As you can see, the pencil track works ok; the highlighter track appears only at the end, so you have to draw it blindly.

from xournalpp.

ok, now I see the problem, thanks :)

from xournalpp.

you're welcome. BTW, the screencast is registered directly in GIF with byzanz --- great piece of software, highly recommended for bug reporting ;-)

from xournalpp.

Well, the problem is definitely here: https://github.com/xournalpp/xournalpp/blob/master/src/view/DocumentView.cpp#L72

If you change CAIRO_OPERATOR_OVER to CAIRO_OPERATOR_SOURCE everything works fine, minus the transparency of course...

from xournalpp.

I have not set-up the system for compiling (is it complex? I will do it otherwise), but if the transparency is off just during the tracing it's a minor problem (if I remember well, the same happens in the original xournal --- the "temporary" track of the highlighter has no transparency while drawing):

from xournalpp.

Thanks for finding the root cause! I'll fiddle with it when I get some time, but for now - if we change it to SOURCE, it's basically not a highlighter anymore, it's a thick pen tip. Perhaps ADD or SATURATE have the correct effect?

It looks like it's something Cairo changed in new versions as on all my installs the highlighter works normally (it first seems to place a SOURCE type stroke and then when you let go of the button it performs the composition OVER). There might be a workaround for this...

from xournalpp.

Well, I am still not quite satisfied... Rerendering the entire rectangle is just too time-consuming...

from xournalpp.

Do you notice that it's sluggish with your commit?

from xournalpp.

Ok, next try :) Let me know if it is still sluggish^^

from xournalpp.

Unfortunately, very sluggish. On my well powered desktop (Debian Squeeze x86_64) it takes almost a minute to render if I quickly draw a complicated pattern quickly.

from xournalpp.

As a summary of this issue - I have only seen it on the Saucy 32 bit release. On 32 bit systems that I have, the highlighter is transparent all through the stroke, while on 64 bit systems the highlighter is transparent only after the pen is lifted for the stroke. I am suspicious that this was at one time a bug in Cairo that appears on Saucy 32 bit, but was fixed later on.

I'll open a new issue for "shapes look different on canvas" since I hadn't noticed that problem before, and @x2b mentioned that in a previous commit. Maybe someone can provide details there - and if there ends up being a redraw fix for that bug with an efficient redraw, perhaps it will serendipitously fix this bug :)

from xournalpp.

Well, granted, with the previous solution (rerendering the rectangle) it might have been a little sluggish. For one thing the rendering (i.e. drawing on the PageViews frame buffer is expensive itself). On the other hand querying the elements intersecting the rectangle is expensive when there are many elements.

The current solution (x2b@d1e1306) only repaints the stroke and otherwise only copies buffers. This is pretty much as fast as it gets. One minute to render a pattern seems pretty impossible to me. Could you check your setup? Maybe you are out of RAM and have to use the hard disk, maybe there is some other process eating up CPU time. Could you just clone the addpoppler branch of my repository into a clean directory and check again?

from xournalpp.

Unfortunately it's not that simple - there are a number of changes in the addpoppler branch that are incompatible with older installs. For example, cloning your branch and compiling gives

../control/../model/../pdf/popplerdirect/poppler/XojPopplerPage.h:53: error: field 'renderMutex' has incomplete type

What I have had to do is push your patches through master and then merge them upwards to addpoppler, glib_new, etc. I was able to do this for x2b/xournalpp@d1e1306, and I ran the patched Xournalpp alongside the master branch. When drawing a stroke with your patch, there is a flicker throughout the entire stroke as it is getting redrawn, whereas in the master branch this isn't noticeable.

Here is the log for the patching I performed:

commit 609060d4deacfc459b35f51115112bf580686f1d
Author: x2b <[email protected]>
Date:   Sun Apr 6 00:06:52 2014 +0200

    Taking another whack at xournalpp/xournalpp#41

    Signed-off-by: Wilson Brenna <[email protected]>

commit eb157fd3ccad606f77665812d27799a7816c9e7b
Author: Wilson Brenna <[email protected]>
Date:   Sat Apr 5 22:51:18 2014 -0400

    Updates inputhandler to apply x2b's patch.

commit d7b4af53067337e5a6c11e11e40832c45a5ebef6
Author: Wilson Brenna <[email protected]>
Date:   Sat Apr 5 22:48:03 2014 -0400

    Manual merge of x2b/xournalpp:bb2cda5 with PageView.cpp manual patch.
    This should draw strokes more quickly.

commit 4fef9893869b93b5dcfaa057ed544745653be721
Author: Wilson Brenna <[email protected]>
Date:   Sat Apr 5 22:38:54 2014 -0400

    Revert "Another modification of the drawing code"
    Conflicts with future commits by x2b.
    This reverts commit ccaca41c9a9ba11ee139152aa9a15727195730c1.

commit ccaca41c9a9ba11ee139152aa9a15727195730c1
Author: x2b <[email protected]>
Date:   Sun Mar 16 18:23:19 2014 +0100

    Another modification of the drawing code

    This hopefully fixes both the reopened
    xournalpp/xournalpp#41 and the long standing
    'strokes look differently when being drawn
    that later on the canvas' issue.

    Signed-off-by: Wilson Brenna <[email protected]>

from xournalpp.

from xournalpp.

Well, I do the following:

git clone https://github.com/x2b/xournalpp.git
cd xournalpp/
git checkout -b addpoppler origin/addpoppler
mkdir build
cd build/
../configure
make
./src/xournalpp

In that case everything compiles fine and I can launch xournalpp without problems (on a current Arch Linux x86_64). I am using gcc 4.8.2-8, maybe the compile errors are due to using clang?

Edit: Also works with clang-3.4

from xournalpp.

It's not a compiler issue, I don't think - I think it's a dependency issue. Debian squeeze is using much older gtk, glib, glade, etc. compared to Arch.

from xournalpp.

Well, as far as I am concerned my changes fix the issue: The drawing is performed correctly and instantaneously on my machine. Perhaps the drawing code is just implemented less efficiently in the older libraries... Can you try it with debian-testing enabled?

from xournalpp.

Is it a complex thing compiling it? If you can point me to a step-by-step instruction (I got lost with poppler, branches, prerrequisites) I would be happy to check it on a couple of Ubuntu installations, 13.10-64bit and 14.04-beta-32 bit.

from xournalpp.

@Rmano It might still be slightly involved because I don't have any physical Ubuntu systems, so there are some dependency hacks. Still, it should be possible with 13.10 as per the instructions in the wiki here:
https://github.com/xournalpp/xournalpp/wiki/Ubuntu-13.10-Saucy-Salamander

@x2b I'm not sure what you mean about debian testing enabled. But at some point I will try a virtual 32-bit Ubuntu and I can test that then. I would certainly believe you that it could be a difference in the way the drawing code works with older libraries since there appear to be plenty of nuances between gtk versions. In order to fully implement the patch, I have to be convinced that it doesn't break compatibility, though (meaning in the best case scenario it won't make master until I make the current master legacy), and that it has a strong benefit (if the highlighter thing is only a bug on 13.10 32-bit and is fixed in 14.04, for example, it might not be worth the tradeoff if future releases for other systems will also render more slowly, even if they only render more slowly under high load).

from xournalpp.

Ok, I am trying to compile now. I have not found any line with --enable-libopenjpeg in the mentioned script, only
enable_libopenjpeg="@enable_libopenjpeg@"
which I did not touch. By the way, I have the libopenjpeg-dev package installed. It is churning away compiling now. Doubtful about it, because in the configure list of poppler seems this thing was not substituted...

Ok, does not work. Something is fishy in the configuration step; libpoppler get built but then I have this error:

g++: error: @DEPENDENCIES_CFLAGS@: No such file or directory

which is clearly something that di not run correctly. I have m4 installed... any idea?

from xournalpp.

The build system has changed since I wrote that wiki, I guess! The first bit should work if you just run

autoreconf
./configure --enable-mathtex --disable-libopenjpeg
make

and the second bit was something I was having troubles with as well. There are two things I needed to change to compile - see bug #56. You'll basically have to change the two @DEPENDENCIES_ lines in src/Makefile.am back to @PACKAGE_. It looks like maybe that was something added in the more modern autotools that Arch uses but most other systems don't - is that right, @x2b?

from xournalpp.

Ok, now it compiled. At the second make; the first one stopped with

ln: failed to create symbolic link ‘/home/romano/tmp/xournalpp/src/pdf/popplerdirect/libpoppler.a’: File exists
ln: failed to create symbolic link ‘/home/romano/tmp/xournalpp/src/pdf/popplerdirect/libpoppler-glib.a’: File exists
ln: failed to create symbolic link ‘/home/romano/tmp/xournalpp/src/pdf/popplerdirect/poppler-config.h’: Filie exists

...then I re-issued make and it went through. The good news is that the highlighter problem is gone and the app is snappy (32bit Ubuntu 13.10 here).

Two things:

1. I think it is important to iron out the build system so that a ./configure; make works if we want more testing/developers. It is one of the big problem with xournal (you never know what you have to download, which branch, what to do to compile) and we should avoid that.

2. There are a lot of warning, but some of them are really scary:

control/XournalMain.cpp: In member function ‘void XournalMain::checkForErrorlog()’:
control/XournalMain.cpp:77:58: warning: too many arguments for format [-Wformat-extra-args]
                                                  filename);
                                                          ^

... there is a filename completely ignored there...

from xournalpp.

Agreed - we've made leaps with the build system over the past year, and I think it will continue to get better. The step forward in that direction is to fix up dependencies regarding libopenjpeg, libjpeg-turbo, etc.

The warnings are fairly innocuous, though - there isn't really any way to have a complicated piece of software that doesn't show compile warnings on any machines, it's like whack-a-mole.

from xournalpp.

I've tested the highlighter patch on Saucy and yes it's very quick. I'm going to test it on a moderately old system that compiles the addpoppler branch and check, and if that has no downsides I'll push it into addpoppler. Thanks!

from xournalpp.

Unfortunately the patch does cause problems on another system - 32-bit 3.10.9-200 Fedora Core 19. The input is rendered much more quickly than my Debian Squeeze test, but it slows down after a while, and from the start there is a really irritating flicker during the redraw. FC19 is fairly recent (and uses the glib_new branch) so I'm not sure I can patch this anywhere quite yet. Comments? Once I figure out whether all recent systems will eventually behave like Ubuntu (i.e. if it's a new GTK version that behaves like this, etc.) I think I'll have a better idea of what to do.

from xournalpp.

@Rmano How is this x2b@2b94e5a improvement to the build script working?

from xournalpp.

Well I've had some time to think about the drawing code, so here is the TL; DR version:

Firstly, just grabbing the first cairo_t* that is available and drawing the temporary stroke is just wrong. That is also the reason why the CAIRO_OPERATOR_OVER is not working. The right way to draw the stroke would be to invalidate a suitable Rectangle by calling repaintRect() and taking care of the painting in the paintRect() method of the PageView. However, the issue #82 remains for the following reason:

If we repeatedly draw parts of the stroke the result will look different compared to a single drawing call of the entire stroke: In the first case we essentially have a sequence of cairo_move_to, cairo_line_to, cairo_stroke calls, while in the latter case we only call cairo_stroke once after a sequence of cairo_line_to calls. For the reason the transparency is not calculated correctly: The single points of the stroke are very close together, so we end up drawing over pixels repeatedly which makes the stroke look opaque. I suppose that for the same reason the antialiasing does not look the way it does after being redrawn correctly. Redrawing the entire stroke every time the cursor moves is not an option, since the drawing process becomes slower and slower as the number of points in the stroke increases. I think I will ask the cairo people if there is anyway around this, otherwise we will just have to accept the strokes looking different

from xournalpp.

Thanks for looking into this - hopefully the Cairo devs can provide some help!

from xournalpp.

bump
I think that x2b@2ba8332 should be fast enough, though I think I could improve everything even further in the case where the drawing is performed when the zoom level is higher. Please let me know if the performance is acceptable. If it is I would clean everything up a little so we can finally close this issue.

from xournalpp.

Thanks! I'll try implementing it when I get the time on the systems that were previously affected by sluggishness with the previous patch. In the worst case where for whatever reason that still exists, probably the most helpful thing would be to post a patch for the affected Ubuntu systems that could be manually applied (on the Wiki) as well as compiling that patch into the relevant binaries, until we know whether these bugs will persist on those systems in future updates.

from xournalpp.

Almost done testing this one - just one machine left to check. So far this is performing really well! The one thing that should be probably taken out when cleaned up as a patch is the CAIRO_ANTIALIAS_GOOD command - that feature might be great to leave in there for the future but at the moment it's not compiling on my cairo2:1.10.2-6.1ubuntu3 install.

view/DocumentView.cpp: In member function ‘void DocumentView::drawStroke(cairo_t*, Stroke*, int, double, bool)’:
view/DocumentView.cpp:101:26: error: ‘CAIRO_ANTIALIAS_GOOD’ was not declared in this scope

from xournalpp.

Ok, should be fixed in d399b0b

Could you merge the commit?

from xournalpp.

bump

from xournalpp.

Sorry - I've been pretty busy! Is there a .patch file that will merge with the glib_new branch? I think a fair amount of code changed between now and when you forked, so it might be a messy merge.

from xournalpp.