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.
Details
- Reviewers
mart - Group Reviewers
Plasma - Commits
- R242:0ffe183d71fd: FrameSvgItem: fix textureRect for tiled subitems to not shrink to 0
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
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
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? |
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. 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. 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. |
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
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.
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:
- Fluffy Bunny: https://store.kde.org/p/1295138/
- Unicorn: https://store.kde.org/p/1295310/
Anyone who I can/should poke to get this patch okayed?