Closed
Bug 298880
Opened 19 years ago
Closed 17 years ago
thebes API updates/suggestions
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: vlad, Unassigned)
Details
Attachments
(4 obsolete files)
I've attached a patch that has some suggestions for the public Thebes APIs; the main changes/points are: * Consistent ref/ptr usage. I think we should always use pointers to contexts, surfaces, patterns, and other similar "heavy" classes; converseley, we should always use refs for lightweight types (points, sizes, rects, etc.). Const refs should be used where appropriate instead of passing points/etc. by value. * gfxMatrix suggestion (unimplemented): Should use 'double' directly and call it gfxDoubleMatrix, and then typedef it to gfxMatrix in the same place we typedef double to gfxFloat. That way if someone wants to use a different gfxFloat, they can implement a new matrix class which may be more natural for that other type. * changed gfxRect to contain a gfxPoint and a gfxSize instead of 4 gfxFloats; this will lead to more natural expressions involving rects and their components (e.g. someRect.pos += somePoint; instead of someRect.x += somePoint.x; someRect.y += somePoint.y;). * Added some more Context APIs: - Suggestion of a SaveMatrix()/RestoreMatrix() API that does the same as Save()/Restre(), but just with the CTM. The stack can be owned by the context. This can fix the XXX for needing a fast way to save/restore the current transformation as the frame hierarchy is traversed mentioned above Save(). - Defined Stroke()/Fill()/Clip() to not consume the current path (cairo's stroke_preserve()/fill_preserve()/clip_preserve() methods). Calling NewPath() is a lot clearer than ending up with an implicit new path. - Changed SetColor and SetPattern to a single SetSource taking a color, pattern, or surface. This more closely indicates that you can only have one source set; e.g. calling SetColor and then calling SetPattern cancels the color. - Added Paint() method for painting surfaces - Added PixelSnappedRectangle() method - Added DeviceToUser/UserToDevice for points and sizes Thoughts?
Reporter | ||
Comment 1•19 years ago
|
||
Reporter | ||
Comment 2•19 years ago
|
||
Updated after discussion with pav; got rid of my SaveMatrix/RestoreMatrix idea, because cairo's Save/Restore is pretty fast. Changed overloaded SetSource() back to SetColor and SetPattern, added SetSource for surfaces.
Reporter | ||
Updated•19 years ago
|
Attachment #187378 -
Attachment is obsolete: true
> cairo's Save/Restore is pretty fast Are you sure? -private: cairo_surface_t* mSurface; Is this really wise? + void Paint(gfxASurface *surface, gfxFloat alpha = 1.0); Painting is not a path operation (right?) so it should be moved. Why is the surface parameter here? + void Polygon(const gfxPoint *points, unsigned long numPoints); Ditch Polygon, I reckon. + void DeviceToUser(gfxPoint& point) const; + void DeviceToUser(gfxSize& size) const; + void UserToDevice(gfxPoint& point) const; + void UserToDevice(gfxSize& size) const; Why not make these functions that return a result? + void SetSource(gfxASurface* surface, const gfxPoint& origin); What does the origin parameter do? (In reply to comment #0) > * Consistent ref/ptr usage. I think we should always use pointers to > contexts, surfaces, patterns, and other similar "heavy" classes; converseley, > we should always use refs for lightweight types (points, sizes, rects, etc.). Good > Const refs should be used where appropriate instead of passing points/etc. by > value. It may be more efficient to pass points by value, especially on 64-bit machines using SSE2. But it doesn't matter too much because we can change that without changing callers. > * gfxMatrix suggestion (unimplemented): Should use 'double' directly and call > it gfxDoubleMatrix, and then typedef it to gfxMatrix in the same place we > typedef double to gfxFloat. That way if someone wants to use a different > gfxFloat, they can implement a new matrix class which may be more natural for > that other type. I'm not sure why you'd want to do that... > * changed gfxRect to contain a gfxPoint and a gfxSize instead of 4 gfxFloats; > this will lead to more natural expressions involving rects and their > components (e.g. someRect.pos += somePoint; instead of someRect.x += > somePoint.x; someRect.y += somePoint.y;). I'm not sold on this but OK. > - Defined Stroke()/Fill()/Clip() to not consume the current path (cairo's > stroke_preserve()/fill_preserve()/clip_preserve() methods). Calling NewPath() > is a lot clearer than ending up with an implicit new path. > > - Added Paint() method for painting surfaces All good > - Added PixelSnappedRectangle() method It's been replaced by a parameter now, I guess, which is fine. BUT you need to define what it does when the transformed rectangle is not aligned with the axes. In my current snapping code, I then decline to snap the rectangle at all. Does that sound like the right thing?
Reporter | ||
Comment 4•19 years ago
|
||
(In reply to comment #3) > > cairo's Save/Restore is pretty fast > > Are you sure? From looking at the code, it should be; I was thinking of a lightweight matrix stack that we could use instead, but pav had me look at the code. It just allocates a constant-size gstate, does a memcpy, and then refs the existing surfaces/patterns/etc. If for some reason it's a bottleneck, we can have it cache unused gstates, but I can't imagine that it will be. > -private: > cairo_surface_t* mSurface; > > Is this really wise? I think so; it makes it protected instead of private, so that concrete surface classes can get at mSurface directly instead of needing to go through CairoSurface(). > + void Paint(gfxASurface *surface, gfxFloat alpha = 1.0); > > Painting is not a path operation (right?) so it should be moved. Why is the > surface parameter here? Yeah, I wasn't sure where to stick it; I guess it's really its own beast. Ignore the surface parameter, not sure what I was thinking. > + void DeviceToUser(gfxPoint& point) const; > + void DeviceToUser(gfxSize& size) const; > + void UserToDevice(gfxPoint& point) const; > + void UserToDevice(gfxSize& size) const; > > Why not make these functions that return a result? Will do. > + void SetSource(gfxASurface* surface, const gfxPoint& origin); > > What does the origin parameter do? It sets the origin inside the surface which should be drawn at the current origin. (In cairo's set_source_surface, it sets up a transform of -origin on the pattern matrix.) > > Const refs should be used where appropriate instead of passing points/etc. by > > value. > > It may be more efficient to pass points by value, especially on 64-bit machines > using SSE2. But it doesn't matter too much because we can change that without > changing callers. Good point; I picked refs for consistency with other usage, but as you say we can just change everything over to passing by value. > > * gfxMatrix suggestion (unimplemented): Should use 'double' directly and call > > it gfxDoubleMatrix, and then typedef it to gfxMatrix in the same place we > > typedef double to gfxFloat. That way if someone wants to use a different > > gfxFloat, they can implement a new matrix class which may be more natural for > > that other type. > > I'm not sure why you'd want to do that... Well, it's a bit of a moot point since we're not likely to do much with matrices other than interact with cairo. My thinking was that if someone wanted to use 'float' or even some fixed point type for gfxFloat, they would want to reimplement gfxMatrix to be the most efficient for their particular type (instead of using the cairo matrix internally, which is doubles). > > > - Added PixelSnappedRectangle() method > > It's been replaced by a parameter now, I guess, which is fine. BUT you need to > define what it does when the transformed rectangle is not aligned with the axes. > In my current snapping code, I then decline to snap the rectangle at all. Does > that sound like the right thing? I think that sounds right; I'm a little unsure right now about when we want to use a snapped rectangle and when not... I think with most rendering right now we want to, but that may give us strange results when there's a scaling factor in the transformation matrix. Maybe we also only want to snap only if we have an x/y scale of 1.0? New patch coming shortly.
Reporter | ||
Comment 5•19 years ago
|
||
Updated with comments
Attachment #187452 -
Attachment is obsolete: true
Comment 6•19 years ago
|
||
This patch merges roc's changes from 298637 with vlad's patches from this bug along with my own patches. I've added refcounting to gfxContext and gfxASurface as well as gone back to passing by value for gfxPoint,gfxSize,gfxRect. I've removed some comments that no longer apply and added a few new ones. Fixed a few problems that didn't build on windows. I'm going to go ahead and check this in so that we aren't all passing around megapatches anymore.
Attachment #187488 -
Attachment is obsolete: true
Comment 7•19 years ago
|
||
Comment on attachment 187495 [details] [diff] [review] All thebes changes. I checked this in along with a followup patch to make refcounting a little less silly than whats in this patch.
Attachment #187495 -
Attachment is obsolete: true
> It sets the origin inside the surface which should be drawn at the current
> origin. (In cairo's set_source_surface, it sets up a transform of -origin on
> the pattern matrix.)
Why not provide a general way to set the pattern matrix? We should allow the
pattern matrix to be non-affine.
All the other comments sound good.
Reporter | ||
Comment 9•19 years ago
|
||
(In reply to comment #8) > > It sets the origin inside the surface which should be drawn at the current > > origin. (In cairo's set_source_surface, it sets up a transform of -origin on > > the pattern matrix.) > > Why not provide a general way to set the pattern matrix? We should allow the > pattern matrix to be non-affine. We do, through gfxPattern::SetMatrix. SetSource() is really just a convenience wrapper around cairo_set_source_surface(), which is just a wrapper for pattern_create_for_surface(). I think the most common use will be SetSource with an origin of 0,0; the next common use will probably be one with an offset; and the for the fully general case, you'd have to go through SetPattern. Does that sound sane? We can rename SetSource() to make it more clear that it's just a convenience wrapper around SetPattern/set_source_pattern..
Comment 10•17 years ago
|
||
closing out old bug
Status: NEW → RESOLVED
Closed: 17 years ago
Component: GFX → GFX: Thebes
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•