Closed Bug 344206 Opened 14 years ago Closed 14 years ago

ScaleRoundOut gives bad results


(Core Graveyard :: GFX, defect)

Not set


(Not tracked)



(Reporter: roc, Assigned: roc)




(1 file, 2 obsolete files)

I was debugging a problem in bug 174397 and discovered that an nsRect(0,0,100,100), after ScaleRoundOut by 15 was (0,0,1501,1501). That is bad.
Attached patch fix (obsolete) — Splinter Review
This FLT_EPSILON thing is clearly not reliable. Let's just use math.h functions instead. Since I'm not changing *Round(), everything I'm changing is rarely used so there shouldn't be any perf impact.
Attachment #228779 - Flags: review?(vladimir)
Attached patch real patch (obsolete) — Splinter Review
Attachment #228779 - Attachment is obsolete: true
Attachment #228781 - Flags: review?(vladimir)
Attachment #228779 - Flags: review?(vladimir)
Comment on attachment 228781 [details] [diff] [review]
real patch

Looks fine to me, though I wonder what this will do to layout, if there's any code that depends on the bogus ScaleRoundOut results.
Attachment #228781 - Flags: review?(vladimir) → review+
Comment on attachment 228781 [details] [diff] [review]
real patch

this patch scares me given how much those functions are used.  Also, are floorf() and ceilf() on every platform?
Good point stuart. floorf is C99 so Microsoft probably doesn't have it :-(.

These functions actually aren't used all that much. We normally use the Round versions, which I'm not changing.

I'll submit a new patch using plain old floor and ceil.
floorf is on windows.  I was more concerned about old random Unix platforms.

We should use floorf/ceilf whenever we can though, since we are deailing with floats for those functions and not doubles.  Maybe add configure checks?
This is better.
Attachment #228781 - Attachment is obsolete: true
Attachment #228855 - Flags: review?(vladimir)
It would be nice to have configure checks for floorf and ceilf. However, I have no reason to believe they significantly help us here.
on platforms where there is no double precision fpu, floorf and ceilf are _way_ _way_ faster
True, but we're already stuffed on those platforms, and cairo is taking us in the wrong direction there. If someone wants us to seriously run there, this will be a very small part of their problem.
checked in.
Closed: 14 years ago
Resolution: --- → FIXED
I think we should take this on the 1.8.1 branch after some trunk baking.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.