Closed Bug 929723 Opened 6 years ago Closed 6 years ago

Add support for playing paths into pathsinks

Categories

(Core :: Graphics, enhancement)

enhancement
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: cabanier, Assigned: cabanier)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Assignee: nobody → cabanier
Blocks: 830734
I'm unsure how I can write patches for this code.
Sorry, I meant that I don't know how to write tests for this code since it's not used yet.
Attachment #821207 - Attachment is obsolete: true
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?
Attachment #821213 - Flags: review?(roc)
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? :-)
Attachment #821213 - Flags: review?(bas)
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!
Attached patch Replay paths into a pathsink (obsolete) — Splinter Review
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.
Attachment #821213 - Flags: review?(bas)
Attachment #821213 - Flags: review-
Attachment #821213 - Flags: feedback+
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.
Attachment #821213 - Flags: review?(roc)
Attachment #821213 - Attachment is obsolete: true
Attachment #822434 - Attachment is obsolete: true
Attachment #8366199 - Flags: feedback?(bas)
Attachment #8366199 - Flags: feedback?(bas)
(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
Flags: needinfo?(bas)
Attachment #8366199 - Flags: review?(bas)
Isn't there code for this already on Central?
Flags: needinfo?(bas)
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.
Attachment #8366199 - Flags: review?(bas) → review-
(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.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.