Consistently use qreal in libs/pigment
ClosedPublic

Authored by dcaliste on Aug 24 2018, 9:51 AM.

Details

Summary

On my ARM environment, qreal is interpreted as float, not double. Currently in libs/pigment, qreal is implicitly treated like a double, while writing something like qmax(v, 0.), where v is a qreal. 0. is a double and the compiler complains because there is no function using (float, double).

There are also many conversions that are done between floats and doubles in KoColorSpace.cpp. So I've updated all necessary functions to accept qreal instead of double or float. As a result, the API has been changed. I've checked that it's still compiling on amd64 in addition to arm for instance but I'm not sure that I didn't break something in the plugins for instance.

I would like to hear your review on this patch. If I'm creating some issues, I'm eager to try to solve them.

Diff Detail

Repository
R8 Calligra
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
dcaliste created this revision.Aug 24 2018, 9:51 AM
Restricted Application added a subscriber: Calligra-Devel-list. · View Herald TranscriptAug 24 2018, 9:51 AM
dcaliste requested review of this revision.Aug 24 2018, 9:51 AM

You use it on Pinebook or similar ARM based laptop?

I'm crosscompiling in SailfishOS SDK. I'm in the process of upgrading Calligra there to latest version. @leinir have done it earlier with version 2.9.x. At that moment, pigment was not containing that kind of function to change luminosity... where the conversions are used. I don't know if it's the best way to handle the issue. Don't hesitate to comment !

I'm OK with the patch, can you try Karbon as well?

Ok, I'll try in the afternoon or during the week-end and comment here.

After test (PRODUCTSET="KARBON"), it's not breaking karbon build on amd64.

I cannot test easily on arm because karbon depends on KF5kdelibs4support, which is not available. But at least, proposed patch makes PART_WORDS and PART_STAGE compilable when qreal <=> float.

I cannot test easily on arm because karbon depends on KF5kdelibs4support, which is not available.

I will investigate to remove it as depend.

dcaliste updated this revision to Diff 40892.Sep 3 2018, 8:14 AM

Update the patch not to use the form `qBound(qreal(0.0), a, qreal(1.0))` but `qBound<qreal>(0.0, a, 1.0)` instead. For reference, original form was `qBound(0.0, a, 1.0)` which was causing issues when qreal <-> float, because of mixed doubles and floats.

This is looking pretty good to me, really... i know there was talk about deprecating qreal at some point, but turns out that never happened?

leinir added a comment.Oct 9 2018, 9:12 AM

@dcaliste @anthonyfieroni Any updates here, pro/con? Would be a terrible shame to let this fall through :)

I'm OK with it +1, @danders or @boemann should accept it.

danders accepted this revision as: danders.Oct 9 2018, 10:38 AM

Fine with me.

This revision is now accepted and ready to land.Oct 9 2018, 10:38 AM
This revision was automatically updated to reflect the committed changes.

Thank you all for the review.