929723 - Add support for playing paths into pathsinks

Status: RESOLVED WONTFIX

(Core :: Graphics, enhancement)

Description

[Rik Cabanier]

To support the canvas path object, it is necessary to copy paths between different backend.

This patch will add support to the gfx layer so a pathbuilder can play into any pathsink.

Comment 1

[Rik Cabanier]

I'm unsure how I can write patches for this code.

Comment 2

[Rik Cabanier]

Sorry, I meant that I don't know how to write tests for this code since it's not used yet.

Comment 3

[Rik Cabanier]

Attachment: First proposal for replaying paths into pathsinks

Comment 4

[Rik Cabanier]

Attachment: First proposal for replaying paths into pathsinks

Comment 5

[Rik Cabanier]

Comment on attachment 821213 [details] [diff] [review]
First proposal for replaying paths into pathsinks

try build: https://tbpl.mozilla.org/?tree=Try&rev=48620c5a64fe

Any suggestions how I can test this?

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 821213 [details] [diff] [review]
First proposal for replaying paths into pathsinks

Review of attachment 821213 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/2D.h
@@ +467,5 @@
>    virtual TemporaryRef<Path> Finish() = 0;
> +
> +  /* This plays the path back into a PathSink
> +  */
> +  virtual void ReplayPath(PathSink& sink) = 0;

I'm a bit confused as to why this is a method on PathBuilder instead of Path. Can you explain why?

::: gfx/2d/PathCG.cpp
@@ +136,5 @@
> +
> +void
> +PathBuilderCG::ReplayPath(PathSink& sink)
> +{
> +  CGPathApply(mCGPath, this, PathApplierFunction);

Shouldn't you be passing "sink" somewhere in here? :-)

Comment 7

[Rik Cabanier]

Comment on attachment 821213 [details] [diff] [review]
First proposal for replaying paths into pathsinks

Review of attachment 821213 [details] [diff] [review]:
-----------------------------------------------------------------

Now that I'm further with the Path object, I'm unsure if this is needed.
It would still be nice to have though...

::: gfx/2d/2D.h
@@ +467,5 @@
>    virtual TemporaryRef<Path> Finish() = 0;
> +
> +  /* This plays the path back into a PathSink
> +  */
> +  virtual void ReplayPath(PathSink& sink) = 0;

In order to have a Path, you have to call "Finish" on PathBuilder which makes it immutable.

This seems like a big limitation since you might want to create a copy of a path while you are still drawing.

::: gfx/2d/PathCG.cpp
@@ +136,5 @@
> +
> +void
> +PathBuilderCG::ReplayPath(PathSink& sink)
> +{
> +  CGPathApply(mCGPath, this, PathApplierFunction);

Yes, good catch!

Comment 8

[Rik Cabanier]

Attachment: Replay paths into a pathsink

Comment 9

[Bas Schouten (:bas.schouten)]

Comment on attachment 821213 [details] [diff] [review]
First proposal for replaying paths into pathsinks

Review of attachment 821213 [details] [diff] [review]:
-----------------------------------------------------------------

The general idea here seems okay, but a couple of changes to the patch are required.

::: gfx/2d/2D.h
@@ +467,5 @@
>    virtual TemporaryRef<Path> Finish() = 0;
> +
> +  /* This plays the path back into a PathSink
> +  */
> +  virtual void ReplayPath(PathSink& sink) = 0;

You can always still do that. I believe the way we should implement this is by putting it on Path, and I'd prefer something along the lines of 'AppendPathToBuilder'.

I'm not sure how this would add any limitations.

::: gfx/2d/PathD2D.cpp
@@ +241,5 @@
>  
>    return new PathD2D(mGeometry, mFigureActive, mCurrentPoint, mFillRule);
>  }
>  
> +class SpecializedSink : public ID2D1GeometrySink

This should be implemented with a code style similar to OpeningGeometrySink for consistency.

Comment 10

[Rik Cabanier]

Comment on attachment 821213 [details] [diff] [review]
First proposal for replaying paths into pathsinks

Review of attachment 821213 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/2D.h
@@ +467,5 @@
>    virtual TemporaryRef<Path> Finish() = 0;
> +
> +  /* This plays the path back into a PathSink
> +  */
> +  virtual void ReplayPath(PathSink& sink) = 0;

After calling Finish, you would have to create another copy if you want to continue using the original path. ie
c = new path(); c.moveto()....
d = new path(c); 
c.moveto(); <= you would need to create a new pathbuilder here

I will rename to function.

::: gfx/2d/PathD2D.cpp
@@ +241,5 @@
>  
>    return new PathD2D(mGeometry, mFigureActive, mCurrentPoint, mFillRule);
>  }
>  
> +class SpecializedSink : public ID2D1GeometrySink

I saw that class.
Will do.

Comment 11

[Rik Cabanier]

Attachment: Replay paths into a pathsink

Comment 12

[Rik Cabanier]

(In reply to Bas Schouten (:bas.schouten) from comment #9)
> Comment on attachment 821213 [details] [diff] [review]
> First proposal for replaying paths into pathsinks
> 
> Review of attachment 821213 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The general idea here seems okay, but a couple of changes to the patch are
> required.
> 
> ::: gfx/2d/2D.h
> @@ +467,5 @@
> >    virtual TemporaryRef<Path> Finish() = 0;
> > +
> > +  /* This plays the path back into a PathSink
> > +  */
> > +  virtual void ReplayPath(PathSink& sink) = 0;
> 
> You can always still do that. I believe the way we should implement this is
> by putting it on Path, and I'd prefer something along the lines of
> 'AppendPathToBuilder'.
> 
> I'm not sure how this would add any limitations.

The limitation is that you need to call Finish to get a Path. After calling Finish, the pathBuilder becomes immutable so you can't add to it anymore.
So from my last example, 
c = new Path(); c.moveTo();...
d = new Path(c); 
c.lineTo();

By moving the method to Path, the code for line 2 becomes:
f = c.Finish; f.AppendPathToBuilder(d); c = new PathSink(); f.AppendPathToBuilder(c); 
By putting it on pathSink it is:
c.ReplayPath(d);

> 
> ::: gfx/2d/PathD2D.cpp
> @@ +241,5 @@
> >  
> >    return new PathD2D(mGeometry, mFigureActive, mCurrentPoint, mFillRule);
> >  }
> >  
> > +class SpecializedSink : public ID2D1GeometrySink
> 
> This should be implemented with a code style similar to OpeningGeometrySink
> for consistency.

I tried to copy the style from that class. Note that SpecializedSink implements a different interface than ID2D1GeometrySink

Comment 13

[Bas Schouten (:bas.schouten)]

Isn't there code for this already on Central?

Comment 14

[Bas Schouten (:bas.schouten)]

Comment on attachment 8366199 [details] [diff] [review]
Replay paths into a pathsink

Review of attachment 8366199 [details] [diff] [review]:
-----------------------------------------------------------------

Duplication of functionality provided by Path::StreamToSink.

Comment 15

[Rik Cabanier]

(In reply to Bas Schouten (:bas.schouten) from comment #14)
> Comment on attachment 8366199 [details] [diff] [review]
> Replay paths into a pathsink
> 
> Review of attachment 8366199 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Duplication of functionality provided by Path::StreamToSink.

Great to see that something similar has landed.
I will close this bug.