FrameSvgItem: fix textureRect for tiled subitems to not shrink to 0
ClosedPublic

Authored by kossebau on Mar 15 2019, 2:37 PM.

Details

Summary

The old logic results in broken rendering if the sample was bigger than the
area to render, due to normalized texture rect being 0 in the respective
dimansion. As well as stretched mapping of the sample for fractional
relationships between the area to render and the sample size.

Test Plan

Borders of backgrounds of FluffyBunny theme render properly, no other
regressions seen (but unsure what else might rely on that artifact).

Diff Detail

Repository
R242 Plasma Framework (Library)
Branch
fixtilingtexture
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 9626
Build 9644: arc lint + arc unit
kossebau created this revision.Mar 15 2019, 2:37 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 15 2019, 2:37 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
kossebau requested review of this revision.Mar 15 2019, 2:37 PM

I have to admit I am not exactly sure what this code does (being a noob to openGL), so my assumptions about the effects I saw by try & error might be wrong. Thus please check properly someone who understands this code.

Broken rendering before:

Find old copy of Fluffy Bunny here to test yourself:
https://share.kde.org/s/RZenX3xAxsoapMM (thanks to @sitter)
broken for panels, but otherwise works at least to test this.

An alternative approach might be to do instead

//if tiling horizontally
if (m_border == FrameSvg::TopBorder || m_border == FrameSvg::BottomBorder || m_border == FrameSvg::NoBorder) {
    textureRect.setWidth(qMax(1, nodeRect.width() / m_elementNativeSize.width()));
}
//if tiling vertically
if (m_border == FrameSvg::LeftBorder || m_border == FrameSvg::RightBorder || m_border == FrameSvg::NoBorder) {
    textureRect.setHeight(qMax(1, nodeRect.height() / m_elementNativeSize.height()));
}

i.e. to use 1 as minimum, and continue to use full multiples. That might help potential themes where the subparts should properly align on the subpart borders (think something like sine wave), at the cost of having things being shrunk to fit.

Both solutions are fine for the Fluffy Bunny theme :)

Perhaps something to control via another flag? :P

apol added a subscriber: apol.Mar 15 2019, 6:40 PM

Does this solve the fluffy bunny issue?
Otherwise it could be that the theme needs adapting to tell plasma to repeat the pattern instead of stretching it.

src/declarativeimports/core/framesvgitem.cpp
148

Makes sense to me. Should we also use qRound?

In D19787#431712, @apol wrote:

Does this solve the fluffy bunny issue?

Yes, both approaches solve the broken rendering for Fluffy Bunny, as they both prevent width/height to be set to 0. Given the fuzzy nature of the border samples it is not that important whether full sample copies are used or if the last is only shown partially. Also if stretched/squashed a bit they look still okay. But that is due to the nature of this theme.

Otherwise it could be that the theme needs adapting to tell plasma to repeat the pattern instead of stretching it.

No, tiling already happens as it should (that's also why this code is reached).

src/declarativeimports/core/framesvgitem.cpp
148

qRound would mean that samples are also stretched beyond their original size if rounding up is done, right? In that case the pixel quality might be an issue perhaps.
Though the max variations then would be only half as much off from the original sample, which seems attractive indeed.

For the stretching needs I smell this could get input from the fractional resolution experts, to know if oversampling better also gets support for higher-resolution samples.

Seems this really depends on the nature of the look, and for the case of stretching with full sample copies one might provide a higher resolution sample, to keep quality.
I sense one could really would want another hint flag. Time to draft another theme where full copies are interesting. Where are pages with some Greek/Roman meander patterns to get inspired? :)

Makes me wonder how complex this should get and if theme authors actually really need all such options :)

Possibly the best for now is to just keep the existing behaviour, but limit to the minimum 1.

kossebau updated this revision to Diff 53986.Mar 16 2019, 6:18 AM

Only prevent width/height of textureRect becoming 0, otherwise keep old
behaviour. Not changing to qround to prevent bad image quality due to
up-sampling.

I learned that CSS3 also has a concept of doing frames from images. And
there are quite some options for doing the tiling of the border image, seems
Plasma currently implements the "stretch" and "round" options:
https://www.w3.org/TR/css-backgrounds-3/#the-border-image-repeat

Perhaps some direction to extend the Plasma theme spec to in the future.
HiDpi might allow theme authors to do more fancy themes where then more
control about might be good to have -> setting a thought seed into whoever will work
on that :P

kossebau updated this revision to Diff 53987.Mar 16 2019, 6:24 AM

add documentation about wanted behaviour, so the next person does not change
it again

After a first coffee, I realize this very logic actually already does upsampling all the time, given it takes the floor of the nodesize/samplesize expression, not the ceiling. And with eyes made sensitive, I see this now all the time.

Rereading the CSS3 spec, it seems they actually want the proposed qRound (+ excluding 0 as nearest integer), so the actual proper algorithm to meet the "round" value of "border-image-repeat" would be:

textureRect.setWidth(qMax(1, qRound((qreal)nodeRect.width() / m_elementNativeSize.width())));

So we still have upsampling, at least with the pixmap material provided by Fluffy Bunny, which seems 1:1 pixels, so not a happy camper yet here for my mission to actually bring Fluffy Bunny back:

Then, looking closer at screenshots of Fluffy Bunny from Plasma4 times, I cannot see any scaling/stretching at all:
https://frinring.files.wordpress.com/2009/01/x-bar.png
https://liquidat.files.wordpress.com/2008/06/fluffybunny.png

Other tiling based themes indicate that indeed no stretching was done:
https://liquidat.files.wordpress.com/2008/06/slickback.png

So... with all that in mind and yet another coffee, IMHO we should restore the logic of Plasma4 then. And if somebody wants the "round" repeat style, we should introduce another hint for them.
Returning to the initial logic, things look like this, which I favour over the other:


Updating patch next.

kossebau updated this revision to Diff 53988.Mar 16 2019, 7:55 AM

Update patch to restore initial logic without stretching, which also resembles Plasma4 one

So far I have not yet seen another tiled-border theme on store.kde.org, so restoring the Plasma4 behaviour should be fine.

Both tiled-border themes I know are now up at stores.kde.org/GHNS also to allow testing:

Anyone who I can/should poke to get this patch okayed?

mart accepted this revision.Mar 19 2019, 10:22 PM
This revision is now accepted and ready to land.Mar 19 2019, 10:22 PM
This revision was automatically updated to reflect the committed changes.