Page MenuHomePhabricator

edje_calc: make INTP use TO_INT_ROUND
ClosedPublic

Authored by kimcinoo on Jan 31 2019, 2:30 AM.

Details

Summary

The edje_part_recalc calculates next postion(p3).
Please refer to following line.

p3->final.y = INTP(p1->final.y, p2->final.y, pos);

If the condition is as blow, then p3->final.y becomes -50 only if pos is 1.0.
Because INP uses TO_INT not TO_INT_ROUND.

p1->final.y == -32
p2->final.y == -50

So we had nonsmooth ending of transition.

Test Plan

Sample application to check this issue. Please look carefully when the rect moves from bottom to top.

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kimcinoo created this revision.Jan 31 2019, 2:30 AM
kimcinoo requested review of this revision.Jan 31 2019, 2:30 AM
kimcinoo edited the summary of this revision. (Show Details)Jan 31 2019, 2:31 AM
kimcinoo edited the test plan for this revision. (Show Details)

using round there looks fine but the modification of round function doesn't look right to me.
That is only valid for decrease case in negative values. increment in negative will be incorrect.
IMO, just leave round up as it does would be fine...

akanad added a subscriber: akanad.Jan 31 2019, 9:32 PM

I just found that rounding rules has already defined on IEEE standard.
I think the round implementation as before this patch looks wrong as the standard rule.
(please have a glance at https://en.wikipedia.org/wiki/IEEE_754#Rounding_rules)

I mean, the way that cinoo wrote is right, isn't it?

kimcinoo updated this revision to Diff 19147.Feb 1 2019, 3:52 AM

Fix one more incorrect calculation.
The _edje_part_pixel_adjust could change final.w(h) value to -1.
This value is using in the _edje_part_recalc_single_rel, and has caused
incorrect position.

kimcinoo updated this revision to Diff 19327.Feb 12 2019, 4:06 AM

If final value is calculated by TO_INT_ROUND, then make _edje_part_pixel_adjust
use TO_INT_ROUND for eval value as well.

changing rounding stuff is ok to me but wondering why introduce additional condition (params->final.w/h > 0) there...

src/lib/edje/edje_calc.c
2433

Is this (params->final.w > 0) condition necessary to come here?

kimcinoo added inline comments.Feb 24 2019, 11:48 PM
src/lib/edje/edje_calc.c
2433

It is because that negative value could come.

cedric added a comment.Mar 1 2019, 3:15 PM

I so wish we had a tests suite for edje...

zmike added a subscriber: zmike.Mar 1 2019, 3:18 PM

I so wish we had a tests suite for edje...

We do though?

cedric added a comment.Mar 1 2019, 3:18 PM

Something that would build more confidence in this not breaking the world somewhere... and silently.

zmike added a comment.Mar 1 2019, 3:19 PM

Something that would build more confidence in this not breaking the world somewhere... and silently.

Isn't that just the fault of people who change the internals of edje but don't add new unit tests?

cedric added a comment.Mar 1 2019, 3:21 PM
In D7842#144967, @zmike wrote:

Something that would build more confidence in this not breaking the world somewhere... and silently.

Isn't that just the fault of people who change the internals of edje but don't add new unit tests?

History too. Edje has been in before there was a make check. Nobody bothered writing a serious tests suite for it that covered what was existing. Considering the amount of work it took me with just eina and eet...

kimcinoo updated this revision to Diff 19936.Mar 6 2019, 2:28 AM

Remove change not related to use of TO_INT_ROUND.

Hermet accepted this revision.Mar 7 2019, 8:28 PM
This revision is now accepted and ready to land.Mar 7 2019, 8:28 PM
This revision was automatically updated to reflect the committed changes.