On Thu, 2013-03-28 at 16:59 +0100, Markus Engel wrote:

Okay, so here's my branch: https://code.launchpad.net/~engelmarkus/inkscape/cppify

Thanks Markus,

Looking at the changes, the scope and breadth of files changed indicates that we should deal with merging this branch quickly. I'm tempted to merge it and deal with the fallout, but I'd like to hear from other devs.

Martin,

My main concern about this kind of approach is that we're hovering between two different object systems. SPObject itself is actually derived from the GObject class structure. However, this patch switches to C++-style inheritance. As such, we should make sure that we don't break the expected behaviour of GObject objects.

Really, I guess it would be good to derive SPObject from Glib::ObjectBase (I.e. use the c++ bindings from upstream) so that we can lose the need for GObject-style inheritance.

AV On 28 Mar 2013 16:52, "Martin Owens" <doctormo@...400...> wrote:

On Thu, 2013-03-28 at 16:59 +0100, Markus Engel wrote:

...

Okay, so here's my branch:

https://code.launchpad.net/~engelmarkus/inkscape/cppify

Thanks Markus,

Looking at the changes, the scope and breadth of files changed indicates that we should deal with merging this branch quickly. I'm tempted to merge it and deal with the fallout, but I'd like to hear from other devs.

Martin,

---

Own the Future-Intel&reg; Level Up Game Demo Contest 2013 Rise to greatness in Intel's independent game demo contest. Compete for recognition, cash, and the chance to get your game on Steam. $5K grand prize plus 10 genre and skill prizes. Submit your demo by 6/6/13. http://p.sf.net/sfu/intel_levelupd2d _______________________________________________ Inkscape-devel mailing list Inkscape-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/inkscape-devel

On 28-3-2013 16:59, Markus Engel wrote:

Okay, so here's my branch: https://code.launchpad.net/~engelmarkus/inkscape/cppify

So great to see someone work on this!!! I'm excited about finally getting rid of the overly verbose and bugprone inheritance code.

I have an important comment about function naming. "sp_feTile_write" is changed to "::onWrite", which is not actually what the function does. It's more like a "::doWrite". For me this kind of naming is very confusing, so I'd rather have it changed simply to "::write". Maybe for some functions it makes sense ("sp_flowtext_child_added" -> "::onChildAdded"), but for most it is not an improvement to change the name of the function ("sp_flowtext_set" -> "::onSet" is particularly bad).

I hope you've automated a lot of those changes... So, at least for a first pass, please don't change the function names. Just remove the "sp_classname_" part, but keep the rest untouched.

Thanks! Johan

Hey Johan,

Well, we're not in a freeze, but the condition on us being mostly thawed was that things should be relatively feature complete when committed. I will make note that if people are adding/improving features incrementally, that has shown to be a nice way to gradually make an overall huge change. Also, we explicitly shouldn't commit things with known regressions unless the developer of that feature has the time and motivation to actively work on fixing them. The number one rule still stands... don't break trunk. If you won't have the time to check in again on a commit within 24 hours (to fix anything that may have inadvertently been broken), wait to commit it until you do have that additional time.

As for merging stuff like the grids/guides branch, if you feel okay with merging that as is and if it's complete enough to not break anything, please feel free to merge it. This is a refactoring cycle and we have 11 blockers for a release with some of those probably waiting until late summer for Krzysztof to work on them. I also have to throw out that I still don't believe the cairo folk have added the api hooks to utilize the new pixman bitmap scaling stuff... so we may still be waiting a bit on upstream.

Anyway, that's part of why I'm so open to committing these changes... I don't foresee a release for a while.

Cheers, Josh

On Thu, Mar 28, 2013 at 12:06 PM, Johan Engelen <jbc.engelen@...2592...> wrote:

On 28-3-2013 17:51, Martin Owens wrote:

...

On Thu, 2013-03-28 at 16:59 +0100, Markus Engel wrote:

...

Okay, so here's my branch: https://code.launchpad.net/~engelmarkus/inkscape/cppify

Thanks Markus,

Looking at the changes, the scope and breadth of files changed indicates that we should deal with merging this branch quickly. I'm tempted to merge it and deal with the fallout, but I'd like to hear from other devs.

Uhm, aren't we still in some kind of freeze state to get 0.49 out the door?

I agree such work is not suited at all for doing in a branch for more than a week, but it seems like a very dramatic change that could spawn a ton of crasher bugs... so it doesn't sound wise if we still intend to release 0.49 within the next months. If we plan on releasing 0.49 half a year or later from now, I'm fine with doing this work in trunk, *but* we really should first merge other branches that have been waiting for merge. (grids/guides branch is waiting for merge for example)

Ciao, Johan

---

Own the Future-Intel&reg; Level Up Game Demo Contest 2013 Rise to greatness in Intel's independent game demo contest. Compete for recognition, cash, and the chance to get your game on Steam. $5K grand prize plus 10 genre and skill prizes. Submit your demo by 6/6/13. http://p.sf.net/sfu/intel_levelupd2d _______________________________________________ Inkscape-devel mailing list Inkscape-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/inkscape-devel

2013/3/28 Josh Andler <scislac@...400...>:

As for merging stuff like the grids/guides branch, if you feel okay with merging that as is and if it's complete enough to not break anything, please feel free to merge it. This is a refactoring cycle and we have 11 blockers for a release with some of those probably waiting until late summer for Krzysztof to work on them. I also have to throw out that I still don't believe the cairo folk have added the api hooks to utilize the new pixman bitmap scaling stuff... so we may still be waiting a bit on upstream.

Jasper's recent temporary solution of scaling the image on our side, then using Cairo only to rotate / skew might be acceptable for the common case of embedding photos.

Regards, Krzysztof

Hey Krzysztof,

In speaking with ~suv, not unexpected, but of course there was an issue she observed and I think it's a problem. :) Basically it was with an unscaled QR code image that was imported and aligned to the pixel grid and then zoomed in on. Comparison screenshot http://dl.dropbox.com/u/65084033/irc/bitmap-sampling-screenshot-r12226-r1222... The left side is before Jasper's change.

I did a test here on my end and yeah, it's definitely unexpected that it's increasing the antialiasing as you increase the zoom. Possibly an unintended bug, but if not it's a regression that's not worth the trade off imho.

Either way... even if that one gets fixed, there's 10 more blockers left. Please note, that's just a statement, not a complaint. :)

Cheers, Josh

On Thu, Mar 28, 2013 at 2:14 PM, Krzysztof Kosiński <tweenk.pl@...400...> wrote:

2013/3/28 Josh Andler <scislac@...400...>:

...

As for merging stuff like the grids/guides branch, if you feel okay with merging that as is and if it's complete enough to not break anything, please feel free to merge it. This is a refactoring cycle and we have 11 blockers for a release with some of those probably waiting until late summer for Krzysztof to work on them. I also have to throw out that I still don't believe the cairo folk have added the api hooks to utilize the new pixman bitmap scaling stuff... so we may still be waiting a bit on upstream.

Jasper's recent temporary solution of scaling the image on our side, then using Cairo only to rotate / skew might be acceptable for the common case of embedding photos.

Regards, Krzysztof

On 03/28/2013 10:43 PM, Johan Engelen wrote:

(this week I told a colleague that maybe he should upgrade to 0.48.4 for some fixes, and he did so. Shortly after I received a mail telling me that he could no longer get grid-snapping to work (anybody working on fixing the very broken settings system and restoring the previous behavior?)

I'm working on it. Expect a fix over the weekend!

Diederik

On 2013-03-28 22:45, Josh Andler wrote:

Hey Krzysztof,

In speaking with ~suv, not unexpected, but of course there was an issue she observed and I think it's a problem. :) Basically it was with an unscaled QR code image that was imported and aligned to the pixel grid and then zoomed in on. Comparison screenshot http://dl.dropbox.com/u/65084033/irc/bitmap-sampling-screenshot-r12226-r1222... The left side is before Jasper's change.

I did a test here on my end and yeah, it's definitely unexpected that it's increasing the antialiasing as you increase the zoom. Possibly an unintended bug, but if not it's a regression that's not worth the trade off imho.

This is basically just a choice, and completely expected (and quite desirable in many cases). My code only deals with DOWNscaling, upscaling is done using Cairo. However, while implementing this work-around, I removed the check that set the interpolation to nearest when upscaling, as I figured it might have been some weird work-around. I guess it was probably put in to have "nicer" upscaling of pixelized bitmaps, but for photographs it does not really make sense. Also, the standard essentially suggests to use at least bilinear interpolation.

Ideally, we would allow users to set a rendering hint (like shape-rendering) to decide what they prefer. However, there does not appear to be a proper hint in the standard (image-rendering mostly stresses the speed, but I guess it could be hijacked). Also, there is no guarantee other renderers make the same choice we do. In general it is probably a bad idea to rely on pixels being upscaled as flat rectangles, as this is quite uncommon behaviour.

Still, since photographs and such are typically downscaled, I would be okay with bringing back the old code. Or just remove the scale clamping from my "hack". This will have a similar effect, with slight anti-aliasing if the upscaled pixels do not fully align with the target raster.

Well, first a bit of history:

2012-12-12: backport fix for 985623http://bazaar.launchpad.net/%7Einkscape.dev/inkscape/RELEASE_0_48_BRANCH/revision/9938-> causes a regression in the snapping behaviour (see this https://bugs.launchpad.net/inkscape/+bug/1093739/comments/10 report) 2012-12-17: release of v0.48.4 2012-12-30: Fix for bug #1093739 (snapping regression)http://bazaar.launchpad.net/%7Einkscape.dev/inkscape/RELEASE_0_48_BRANCH/revision/9941-> will be included in v0.48.5

So the question is: when will v0.48.5 be released?

What's still remaining is that in 0.48.4 also some changes were backported from trunk which affect the snapping of very large selections (in which case some kind of a convex hull is being calculated), see thishttps://bugs.launchpad.net/inkscape/+bug/1093739/comments/10report. This should have been improved too in rev. 9947, which I just committed to the v0.48.x branch.

I'm not aware of any other snapping regression in v0.48.x branch.

Diederik

On Thu, Mar 28, 2013 at 10:58 PM, Diederik en Rezi <mail@...1689...>wrote:

On 03/28/2013 10:43 PM, Johan Engelen wrote:

(this week I told a colleague that maybe he should upgrade to 0.48.4 for some fixes, and he did so. Shortly after I received a mail telling me that he could no longer get grid-snapping to work (anybody working on fixing the very broken settings system and restoring the previous behavior?)

I'm working on it. Expect a fix over the weekend!

Diederik

On Thu, Mar 28, 2013 at 10:43 PM, Johan Engelen < jbc.engelen@...2592...> wrote:

...

On 28-3-2013 22:14, Krzysztof Kosiński wrote:

...

2013/3/28 Josh Andler <scislac@...400...>:

...

As for merging stuff like the grids/guides branch, if you feel okay with merging that as is and if it's complete enough to not break anything, please feel free to merge it. This is a refactoring cycle and we have 11 blockers for a release with some of those probably waiting until late summer for Krzysztof to work on them. I also have to throw out that I still don't believe the cairo folk have added the api hooks to utilize the new pixman bitmap scaling stuff... so we may still be waiting a bit on upstream.

Jasper's recent temporary solution of scaling the image on our side, then using Cairo only to rotate / skew might be acceptable for the common case of embedding photos.

We really shouldn't release if we already know it will be a downgrade from the previous release.

-Johan

(this week I told a colleague that maybe he should upgrade to 0.48.4 for some fixes, and he did so. Shortly after I received a mail telling me that he could no longer get grid-snapping to work (anybody working on fixing the very broken settings system and restoring the previous behavior?) and that memory usage had exploded, so he went back to 0.48.2, and I guess he'll never think about upgrading Inkscape again)

---

Own the Future-Intel(R) Level Up Game Demo Contest 2013 Rise to greatness in Intel's independent game demo contest. Compete for recognition, cash, and the chance to get your game on Steam. $5K grand prize plus 10 genre and skill prizes. Submit your demo by 6/6/13. http://altfarm.mediaplex.com/ad/ck/12124-176961-30367-2 _______________________________________________ Inkscape-devel mailing list Inkscape-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/inkscape-devel

On 04/01/2013 01:05 PM, Johan Engelen wrote:

trunk does not remember the bbox snapping settings for me. It's always turned off every time I start Inkscape. Very annoying.

That's strange, and should obviously be fixed. I haven't seen this behaviour, and can't reproduce it either with trunk. Do you have more details for me? Have you seen this happening with others too? On specific documents or systems maybe?

Diederik

I can appreciate that many of you find this sort of code reorganization to be quite attractive, and will concede that it might at some point minimize errors. However, it is perhaps worth pointing out that when an Inkscape binary built from current trunk was run today under valgrind, and given the trivial task of opening an SVG file consisting of just 5 lines of text, rotating them a little, and then quitting without saving, the resulting log file contained 34432 "lost in" records and 6 "uninitialized" records.

In brief, Inkscape at present seems to be exceedingly sloppy in how it handles memory, and I would rate the squishing of the vast majority of these memory issues as a far higher priority than the reorganization of the code. Unfortunately finding these is not going to be trivial since many of them show up in messages like this one:

==2926== 12 bytes in 1 blocks are possibly lost in loss record 10,043 of 54,283 ==2926== at 0x402B9B4: operator new(unsigned int) (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so) ==2926== by 0x8111EB0: Geom::Path::Path(Geom::Point) (path.h:183) ==2926== by 0x3FE851E3: ???

How one would fix these I have no idea, since "???" is not terribly informative, and the memory locations are likely to vary from run to run (and certainly from link to link.) The string "???" appears 91134 times in the valgrind log file.

This is all pretty ironic since eliminating exactly these sorts of memory issues was one of the reasons C++ was created in the first place. Yet here we have an example of C++ code with far more memory issues than I have ever encountered in any C project, and unfortunately with the majority (I think) of them of the type indicated above, making them exceedingly hard to find. Now it may well be that many of these are not actually memory issues per se, but are instances where g++/Inkscape's memory handling confuses valgrind. Even that would be a problem though, because the extra "noise" makes it hard to find the real memory issues.

Regards,

David Mathog mathog@...1176... Manager, Sequence Analysis Facility, Biology Division, Caltech

On Apr 1, 2013, at 5:35 PM, Krzysztof Kosiński wrote:

2013/4/1 mathog <mathog@...1176...>:

...

In brief, Inkscape at present seems to be exceedingly sloppy in how it handles memory, and I would rate the squishing of the vast majority of these memory issues as a far higher priority than the reorganization of the code.

I am guessing 95% of those are false positives caused by the use of libgc. Use the suppression file libgc.supp present in the root source control directory to remove them.

Yes, Krzysztof has hit on the main issue you're tripping up against.

Unfortunately even with the suppressions we'll not get any useful memory leak info. We can get things like uninitialized use, double frees, etc.

In general libgc will hold onto memory it grabs and reuse it later as things progress. If you want to disable the garbage collection in order to get valigrind to work better, try adding this environment variable tweak to the beginning of the line when you run it

_INKSCAPE_GC=disable valgrind...

Of course, to correct inefficient use of garbage collection, the general trick is to try to follow the chain of what is keeping objects pinned into memory. Often there will be perhaps only a single reference pinning a whole tree in place and blocking it from being collected. The other main option is to remove use of libgc altogethe, but that is probably a little more long-term.

On 2013-04-02 09:33 +0100, Jasper van de Gronde wrote:

On 31-03-13 15:24, Ivan Louette wrote:

...

Don't forget that pixellizing may be a very wanted feature when upscaling bitmaps or when upscaling filtered objects with FilterRes applied.

See also https://bugs.launchpad.net/inkscape/+bug/710639

But as a default? If you really think it will be more common to want pixelized bitmaps than smooth bitmaps, I'll gladly switch back to the old behaviour. Just say the word.

This report was just filed today for current trunk:

- Bug #1163449 “Imported bitmap appear blurry when zoomed in” https://bugs.launchpad.net/inkscape/+bug/1163449

and describes a use case where it might be helpful to allow an option for non-smoothed bitmap images when zooming in (aka the old behavior).

Personally, I'd appreciate such an option too, since when rarely working with (down-scaled) photographic images in Inkscape drawings, the new smoothing of unscaled, pixel-grid-aligned bitmap images (e.g. clipped screenshots of GUI elements for mockups mixed with vector objects) when zooming in closer can be rather irritating and has other side effects too (e.g. reducing the precision of the paint bucket tool).

On Wed, 2013-04-03 at 10:54 +0200, Jasper van de Gronde wrote:

On 02-04-13 20:10, ~suv wrote:

...

On 2013-04-02 09:33 +0100, Jasper van de Gronde wrote:

...

On 31-03-13 15:24, Ivan Louette wrote:

...

Don't forget that pixellizing may be a very wanted feature when upscaling bitmaps or when upscaling filtered objects with FilterRes applied.

See also https://bugs.launchpad.net/inkscape/+bug/710639

But as a default? If you really think it will be more common to want pixelized bitmaps than smooth bitmaps, I'll gladly switch back to the old behaviour. Just say the word.

This report was just filed today for current trunk:

• Bug #1163449 “Imported bitmap appear blurry when zoomed in” https://bugs.launchpad.net/inkscape/+bug/1163449

and describes a use case where it might be helpful to allow an option for non-smoothed bitmap images when zooming in (aka the old behavior).

Personally, I'd appreciate such an option too, since when rarely working with (down-scaled) photographic images in Inkscape drawings, the new smoothing of unscaled, pixel-grid-aligned bitmap images (e.g. clipped screenshots of GUI elements for mockups mixed with vector objects) when zooming in closer can be rather irritating and has other side effects too (e.g. reducing the precision of the paint bucket tool).

I've been discussing this with Ivan Louette as well, and I'll see if I can make it a (display) option. If someone has a /good/ suggestion for making it something that can be selected per-image, I'm all ears.

I've just had a quick look at how the web browsers and Batik handle bitmap scaling. When scaling up they all smooth the bitmaps. This seems to be what the SVG 1.1 spec requires. There is no provision for turning this off, the closest is to set the 'image-rendering' value to 'optimizeSpeed'.

It has been proposed in CSS to take the SVG 'image-rendering' property and add new values. Currently Firefox and Opera support the value 'crisp-edges'. See:

https://developer.mozilla.org/en-US/docs/CSS/image-rendering

It doesn't appear that anybody supports in inside SVG (only in HTML).

Tav

On Wed, 2013-04-03 at 14:38 +0200, Jasper van de Gronde wrote:

On 03-04-13 13:25, Tavmjong Bah wrote:

...

... I've just had a quick look at how the web browsers and Batik handle bitmap scaling. When scaling up they all smooth the bitmaps.I This seems to be what the SVG 1.1 spec requires. There is no provision for turning this off, the closest is to set the 'image-rendering' value to 'optimizeSpeed'.

It has been proposed in CSS to take the SVG 'image-rendering' property and add new values. Currently Firefox and Opera support the value 'crisp-edges'. See:

https://developer.mozilla.org/en-US/docs/CSS/image-rendering

It doesn't appear that anybody supports in inside SVG (only in HTML).

Would it make sense to start supporting this in Inkscape? Obviously support in other renderers would be absent (at least for now), but as it is mostly a hint anyway... Put differently, if our choice is between adding a global preference for Inkscape that toggles between resampling methods and adding support for image-rendering with crisp-edges and/or pixelated, what would be preferable?

Adding a global toggle is not a good idea. Our SVG would then never be rendered the same as by others. Using image-rendering is a much better solution.

BTW, just to be clear, I'm assuming that these values live purely in the CSS domain (and even there are not standardized, yet), and would thus have to live in the style attribute.

I have asked that this topic be added to the agenda for tomorrow's SVG working group meeting. If it receives a favorable response, we can then add it with the "-inkscape" prefix. Once the standard is approved, we can then remove the prefix. This avoids half the problem we face with flowed text.

This brings up the question of how to handle SVG2/CSS3/etc. features in general. I have a proposal but don't have time at the moment to write it all out.

Tav