Closed Bug 520710 Opened 10 years ago Closed 10 years ago

SVG SMIL: <set> uses calcMode="discrete"

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: birtles, Assigned: dholbert)

Details

Attachments

(3 files, 2 obsolete files)

I think our implementation of <set> needs review. With the attached test case it seems like we're applying calcMode="discrete" for <set> when we should just set the value. So at t=1s, the fill should be set at to green. Instead it seems like we're using two values: red (the base value, i.e. implied 'from' value), green and then applying discrete animation. Hence, it doesn't go green until t=2s. I added the 'x' animation to show when t=1s.

I could be wrong about this, but I don't have time to look into it at the moment. I had a quick look at nsSVGSetElement.cpp and I can't quite see what's going on. I suspect there are other issues there. e.g. Is it possible to set calcMode=linear on a <set> element and have it interpolate? Like I said, I haven't had time to look into it but I'm pretty sure the attached test case is wrong.
Sorry, I had a bit more of a look into it. Seems like we'll probably handle attempts to set the calcMode. But we are forcing calcMode="discrete" and I'm not sure that that's correct.

It's in content/smil/nsSMILSetAnimationFunction.cpp
You're right -- currently, <set to="val"> behaves just like <animate to="val" calcMode="discrete"> -- it sets the base value for the first half of the duration, when it shouldn't.
This fixes the problem by overriding InterpolateResult for <set> elements, such that it just always returns the 'to' value.

When we arrive in InterpolateResult, we've already verified that our animation is enabled and the current time is in its duration.  Given that those things are true, <set> should always just apply its "to" value.

I've included some basic sanity-checking at the top of nsSMILSetAnimationFunction::InterpolateResult (and cleaned up the corresponding sanity-checking in the nsSMILAnimationFunction::InterpolateResult).  But aside from that sanity-checking, all we really need to do is 
  { aResult = aValues[0]; // apply 'to' value }
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #407847 - Flags: superreview?(roc)
Attachment #407847 - Flags: review?(birtles)
Attachment #407847 - Flags: superreview?(roc) → superreview+
Attachment #407847 - Flags: review?(birtles) → review+
Same patch, plus some simple reftests for <set>.  Carrying forward sr+
Attachment #407847 - Attachment is obsolete: true
Attachment #407861 - Flags: superreview+
Attachment #407861 - Flags: review?(birtles)
Comment on attachment 407861 [details] [diff] [review]
same fix, with tests

Carrying forward r+
Attachment #407861 - Flags: review?(birtles) → review+
http://hg.mozilla.org/mozilla-central/rev/1a60b95e6537
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
I don't how how or why, but this broke my debug build on OSX
Undefined symbols:
  "nsSMILAnimationFunction::IsToAnimation() const", referenced from:
      nsSMILSetAnimationFunction::InterpolateResult(nsTArray<nsSMILValue> const&, nsSMILValue&, nsSMILValue&)in nsSMILSetAnimationFunction.o
ld: symbol(s) not found
Weird! I'll see if I can reproduce on my mac machine at the office... given that this worked fine on the tinderboxen, though, I wonder if this is a quirk in the particular version of your compiler?

[Talked to smaug on IRC -- he's away from this OS X box until Sunday. If we don't see this failure anywhere else, maybe we can find out more from smaug's box after Sunday]
(In reply to comment #8)
> Weird! I'll see if I can reproduce on my mac machine at the office

FWIW, I can't reproduce smaug's linker issue on my mac, with a fresh mozilla-central checkout. (running OS X 10.5.7, with gcc / g++ version 4.0.1 build 5465)
I think I have the very latest XCode on my 10.5 Mac.
(whatever the gcc version is)

Anyway, I'll retest on Sunday.
I did a clobber build and the result is still
Undefined symbols:
  "nsSMILAnimationFunction::IsToAnimation() const", referenced from:
      nsSMILSetAnimationFunction::InterpolateResult(nsTArray<nsSMILValue> const&, nsSMILValue&, nsSMILValue&)in nsSMILSetAnimationFunction.o
ld: symbol(s) not found
collect2: ld returned 1 exit status

gcc version 4.0.1 (Apple Inc. build 5493)
If you remove the inline keyword from

inline PRBool
nsSMILAnimationFunction::IsToAnimation() const

in content/smil/nsSMILAnimationFunction.cpp

does it link then? inline seems a strange thing to have in a cpp file. Maybe the two inline methods should move to the header file.
Removing 'inline' from the methods helps.
Followup patch to fix smaug's build issue.  This just moves two inline methods' definitions from the CPP file to the header file, as Robert Longson suggested above.
Attachment #408282 - Flags: review?(roc)
Comment on attachment 408282 [details] [diff] [review]
followup: move inline methods IsToAnimation / IsAdditive to header

/me redirects r= to smaug, since he's on IRC and can review this as well.
Attachment #408282 - Flags: review?(roc) → review?(Olli.Pettay)
Comment on attachment 408282 [details] [diff] [review]
followup: move inline methods IsToAnimation / IsAdditive to header

This seems to work here.

Nit, && should not start a line.
Attachment #408282 - Flags: review?(Olli.Pettay) → review+
Thanks smaug! Here's the patch with that nit fixed -- carrying forward r+.
Attachment #408282 - Attachment is obsolete: true
Attachment #408285 - Flags: review+
You need to log in before you can comment on or make changes to this bug.