> On July 17, 2014, 4:54 p.m., Eike Hein wrote: > > Unless I'm missing something, this approach seems to defeat the purpose of > > the exercise: The goal of the original change is to use actually use > > smaller textures that fit the QSG's "this can use the atlas" heuristic on > > most platforms. You have one node per patch element here, but they're all > > calling createTextureFromImage() on a big honking QImage that's not going > > to be atlased for most frames. > > > > Can you explain your pixel-perfect quality concerns better - you've said > > the competing approach breaks things there but not mentioned what or why? > > Marco Martin wrote: > That's a button that uses composeoverborders: see the corners/sides? > http://wstaw.org/m/2014/07/17/plasma-desktopvc1725.png > (overlays would give a similar problem, not only the internal is painted > over the borders, but it's also cutted with an alpha mask to match) > That's why i'm using framesvg to paint with the local qpainter instead of > just taking the side image. > > As for defeating the purpose yeah, it does (what actually does is just > being faster duting animated resizes), but in simple cases (no > composeoverborders, no overlays) the timer may be disabled altogether. > > Eike Hein wrote: > > composeoverborders > > Right, but doesn't have David's patch a fallback codepath for that? > Obviously I'd heavily -1 everything causing visual regressions too. > > About the symbol exports: I don't think they're a major issue. We're not > installing the header, things aren't exposed to developers above the lib, and > it's certainly not an uncommon pattern in Qt (there's data() methods all over > the place, even in public APIs). > > On the whole I think David's patch has some compelling advantages in the > ideal case that may warrant the complexity (I don't think complexity in > low-level components is a big deal if it's Worth It[TM] and just needs to be > debugged once; this isn't code that changes often). > > How do we progress here? > > David Edmundson wrote: > To credit Marco, I've only just added that to the fallback path; thanks > for pointing it out. > We can then recommend themers avoid it; am I right in thinking it was > just for QPainter optimisations? > > Marco Martin wrote: > the screenshot i posted is with david's patch, and you can see the button > background one pixel outside its corners. > the fallback here is to yes rerender the scaled svg, but with > Svg::image(), that only returns the rendered element id, not the composition > with alpha masks and all that jazz. Also since it immediately rerenders the > svg in that case it loses the advantage when the resize it's animated. > > Marco Martin wrote: > "We can then recommend themers avoid it" > No, it's the only way to have rounded borders *and* a gradient that is > correct, because otherwise resizing only the internal part of the gradient > but not the mini gradients at the edges, they have a different gradient > length, giving a final gradient not visually continuous
Makes sense. Thanks. - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119336/#review62607 ----------------------------------------------------------- On July 17, 2014, 12:47 p.m., Marco Martin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/119336/ > ----------------------------------------------------------- > > (Updated July 17, 2014, 12:47 p.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: plasma-framework > > > Description > ------- > > This is a derivative of https://git.reviewboard.kde.org/r/119330/ > > It has a much simpler codebase and it doesn't touch framesvg in the library > (and doesn't make things public) > > the important differences are two, that are imo 100% necessary to maintain a > pixel perfect rendering (sacrificing that is a regression simply not > acceptable in any case, even for non default themes, ever). At the same time > avoids duplication of framesvg code in framesvgitem. > > potential issues: > * e037203748 support for tiling still need porting > * yes, it uses a qpainter, that means another copy: but again is necessary as > framesvg knows how to render the end result pixel perfect, since for many > themes the end piece is *not* the simple rendered element id. > * yes, the final scaled texture is still uploaded as a whole, it only avoids > to do it too often (like in animations) with event compression, again, only > way to be sure it's correctly rendered all the time. > > The latter two points can have the following optimizations: > Iff the frame does not have an overlay and does not have composeoverborders > set, the following can be done: > * use Svg::image() to fetch the pixmap of the piece, since in that case would > be valid > * resize the framesvg one single time, at a fixed size , like something > > 256x256 (256 is not random thing, since we have 8 bits per channel, usually > gradients will have no more than 256 stops) and disable the resize timer, > this way we are even sure that only one image per prefix will be stored in > cache > > I should add regarding the last two optimization points: i would like to see > some real benchmarks about them, or i would not consider then necessary until > then. > > > Diffs > ----- > > examples/applets/widgetgallery/contents/ui/Buttons.qml 379585f > examples/applets/widgetgallery/contents/ui/Menu.qml 1336c42 > examples/applets/widgetgallery/contents/ui/standalonemain.qml PRE-CREATION > src/declarativeimports/core/framesvgitem.h e155f6a > src/declarativeimports/core/framesvgitem.cpp 8320212 > src/declarativeimports/core/svgitem.cpp 1ed0631 > tests/dialog.qml PRE-CREATION > tests/testborders.qml PRE-CREATION > > Diff: https://git.reviewboard.kde.org/r/119336/diff/ > > > Testing > ------- > > > Thanks, > > Marco Martin > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
