Bug 12150 - The cases where no stops are defined, or when a stop offset is less than previous not handled correctly
Summary: The cases where no stops are defined, or when a stop offset is less than prev...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL: http://www.w3.org/Graphics/SVG/Test/2...
Keywords:
Depends on:
Blocks:
 
Reported: 2007-01-07 10:13 PST by Rémi Zara
Modified: 2007-01-09 21:00 PST (History)
3 users (show)

See Also:


Attachments
Handle stops corner cases better (15.93 KB, patch)
2007-01-07 10:20 PST, Rémi Zara
no flags Details | Formatted Diff | Diff
Addressed WildFox's comments (16.21 KB, patch)
2007-01-07 13:48 PST, Rémi Zara
no flags Details | Formatted Diff | Diff
updated patch, with new test case and handling of offset>1.0 (31.44 KB, patch)
2007-01-08 14:00 PST, Rémi Zara
no flags Details | Formatted Diff | Diff
Don't use memset to set a CGFloat to 0 (1.21 KB, patch)
2007-01-09 13:56 PST, Rémi Zara
ddkilzer: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rémi Zara 2007-01-07 10:13:20 PST
When there are no stops, ToT uses a black fill, when something equivalent to fill="none" should be used.
Tot also sorts stop offsets, but should not, and should take for each offset value the max between the offset value and all previous offset values.

This breaks the W3C test pservers-grad-16-b

Patch following.
Comment 1 Rémi Zara 2007-01-07 10:20:54 PST
Created attachment 12279 [details]
Handle stops corner cases better

Use a transparent fill when there are no stops.
Don't sort stop offsets, and always take the max between current offset value and previous offset values.
Comment 2 Nikolas Zimmermann 2007-01-07 12:01:17 PST
Hey Remi,

patch looks great! Two tiny comments:

     m_stops = stops;
-    std::sort(m_stops.begin(), m_stops.end(), compareStopOffset);

you can remove the static compareStopOffset helper completelty.

+        CGFloat currOffset = max(stops[i].first,previousOffset);

missing space between first and previousOffset.

Keep up the good work.
Niko
Comment 3 Rémi Zara 2007-01-07 13:48:52 PST
Created attachment 12287 [details]
Addressed WildFox's comments
Comment 4 Eric Seidel (no email) 2007-01-07 13:58:23 PST
This is covered in the spec, it seems:

Some notes on gradients:

Gradient offset values less than 0 (or less than 0%) are rounded up to 0%. Gradient offset values greater than 1 (or greater than 100%) are rounded down to 100%.
It is necessary that at least two stops defined to have a gradient effect. If no stops are defined, then painting shall occur as if 'none' were specified as the paint style. If one stop is defined, then paint with the solid color fill using the color defined for that gradient stop.
Each gradient offset value is required to be equal to or greater than the previous gradient stop's offset value. If a given gradient stop's offset value is not equal to or greater than all previous offset values, then the offset value is adjusted to be equal to the largest of all previous offset values.
If two gradient stops have the same offset value, then the latter gradient stop controls the color value at the overlap point. In particular:
<stop offset="0" stop-color="white"/>
<stop offset=".2" stop-color="red"/>
<stop offset=".2" stop-color="blue"/>
<stop offset="1" stop-color="black"/>
will have approximately the same effect as:
<stop offset="0" stop-color="white"/>
<stop offset=".1999999999" stop-color="red"/>
<stop offset=".2" stop-color="blue"/>
<stop offset="1" stop-color="black"/>
which is a gradient that goes smoothly from white to red, then abruptly shifts from red to blue, and then goes smoothly from blue to black.
Comment 5 Darin Adler 2007-01-07 21:12:46 PST
Comment on attachment 12287 [details]
Addressed WildFox's comments

Looks good. r=me

Is there also a maximum value for stops of 1.0 that should be enforced?
Comment 6 Rémi Zara 2007-01-08 14:00:47 PST
Created attachment 12309 [details]
updated patch, with new test case and handling of offset>1.0
Comment 7 Eric Seidel (no email) 2007-01-08 14:31:36 PST
Comment on attachment 12309 [details]
updated patch, with new test case and handling of offset>1.0

Looks good to me.  Thanks for the great patch!
Comment 8 Sam Weinig 2007-01-08 15:19:54 PST
Landed in r18681.
Comment 9 Darin Adler 2007-01-08 16:01:18 PST
Comment on attachment 12309 [details]
updated patch, with new test case and handling of offset>1.0

memset is not a correct way to set floating point values to 0 -- the memset part of this patch is incorrect
Comment 10 David Kilzer (:ddkilzer) 2007-01-08 16:21:16 PST
Reopened per Comment #9.

Comment 11 David Kilzer (:ddkilzer) 2007-01-09 10:01:14 PST
Comment on attachment 12287 [details]
Addressed WildFox's comments

Clearing darin's r+ as this patch was never landed.
Comment 12 David Kilzer (:ddkilzer) 2007-01-09 10:02:47 PST
Comment on attachment 12309 [details]
updated patch, with new test case and handling of offset>1.0

Clearing macdome's r+ so this bug doesn't show up in the commit queue.

We still need to address the issue in Comment #9.
Comment 13 David Kilzer (:ddkilzer) 2007-01-09 10:03:51 PST
Comment on attachment 12309 [details]
updated patch, with new test case and handling of offset>1.0

(In reply to comment #12)
> Clearing macdome's r+ so this bug doesn't show up in the commit queue.
> 
> We still need to address the issue in Comment #9.

REALLY clearing the flag this time.
Comment 14 Rémi Zara 2007-01-09 13:56:50 PST
Created attachment 12335 [details]
Don't use memset to set a CGFloat to 0

Do not use memset.
Patch in attachment #12309 [details] was committed as r18681
Comment 15 Rémi Zara 2007-01-09 14:02:24 PST
Comment on attachment 12335 [details]
Don't use memset to set a CGFloat to 0

I'm no reviewer; correct the flag
Comment 16 Eric Seidel (no email) 2007-01-09 14:05:04 PST
Comment on attachment 12335 [details]
Don't use memset to set a CGFloat to 0

r=me, sorry for leading you astray.
Comment 17 Darin Adler 2007-01-09 14:05:28 PST
Comment on attachment 12335 [details]
Don't use memset to set a CGFloat to 0

r=me
Comment 18 David Kilzer (:ddkilzer) 2007-01-09 14:05:39 PST
Comment on attachment 12335 [details]
Don't use memset to set a CGFloat to 0

>+        outColor[0] = 0;
>+        outColor[1] = 0;
>+        outColor[2] = 0;
>+        outColor[3] = 0;

I think "0" should be "0.0f" here.
Comment 19 Rémi Zara 2007-01-09 14:11:45 PST
(In reply to comment #18)
> >+        outColor[0] = 0;
> >+        outColor[1] = 0;
> >+        outColor[2] = 0;
> >+        outColor[3] = 0;
> 
> I think "0" should be "0.0f" here.
> 
The original code used "0"
Comment 20 David Kilzer (:ddkilzer) 2007-01-09 14:24:17 PST
Comment on attachment 12335 [details]
Don't use memset to set a CGFloat to 0

This version of Bugzilla apparently doesn't have collision detection for review flags.

Please see Comment #16 or #17 for a "real" reviewer to credit.
Comment 21 Darin Adler 2007-01-09 14:28:49 PST
Comment on attachment 12335 [details]
Don't use memset to set a CGFloat to 0

0.0 or 0.0f would be OK (CGFloat is double under 64-bit). But in general this does not matter, because the conversion works fine. In expressions (not simple constants) the type is more likely to matter because of the intermediate values.
Comment 22 David Kilzer (:ddkilzer) 2007-01-09 14:35:54 PST
(In reply to comment #20)
> This version of Bugzilla apparently doesn't have collision detection for review
> flags.

In case anyone is interested in collision detection for flags in Bugzilla:

https://bugzilla.mozilla.org/show_bug.cgi?id=99215
Comment 23 David Kilzer (:ddkilzer) 2007-01-09 21:00:23 PST
(In reply to comment #14)
> Created an attachment (id=12335) [edit]
> Don't use memset to set a CGFloat to 0

Committed revision 18731.