Skip to content

Comments

imaging_model: introduce initial clip-focused model types#165

Open
waywardmonkeys wants to merge 1 commit intolinebender:mainfrom
waywardmonkeys:initial-imaging_model
Open

imaging_model: introduce initial clip-focused model types#165
waywardmonkeys wants to merge 1 commit intolinebender:mainfrom
waywardmonkeys:initial-imaging_model

Conversation

@waywardmonkeys
Copy link
Collaborator

Introduce a first draft of the imaging_model crate with clip-focused model types. The goal is to establish a small, explicit core model that we can evolve as we define the imaging model and move our renderers to using it.


/// Conservative axis-aligned bounds of this geometry.
///
/// Returns `None` if the geometry bounds are non-finite.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a reasonable guarantee to make?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I understand the point of making these return values options, it forces the client to consider the non-finite case, where in the current world there is substantial risk of nan values flowing through to consumers where they will more likely than not wreak havoc. I'm not sure I love it though, it feels like it adds nontrivial ceremony.

return None;
}

// Conservative outset to account for width, caps, and miter joins.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the fact that this isn't accurate a reason to not do it? Should we drop Clip::bounds because of this? But we do call it out in the doc comment. But maybe we should call the function conservative_bounds instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok, and having fast reasonably-accurate bounds is a useful feature. In practice, I think it will be very unlikely people will specify a miter limit greater than the default 4.0.

Fill {
/// Transform applied to the clip shape.
///
/// This does not affect subsequent draws; it only affects how the clip shape is interpreted.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At some point, we should identify the semantics of this transform: coordinate space, when it is applied, etc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. It might be worth looking at the 1982 Warnock and Wyatt paper, as they are quite precise about this, and in many ways, that work is closer to the modern imaging model work than the PS/Cairo/HTML5 evolution; in particular, geometry for clip/fill/stroke is an object passed to the drawing method rather than belonging to graphics state.

The "subsequent draws" language is not great as normative documentation, as it implies a temporal sequencing. I think a more precise way of saying this is that the transform is applied to the clip geometry but not to the content affected by the clip.

Introduce a first draft of the `imaging_model` crate with
clip-focused model types. The goal is to establish a small,
explicit core model that we can evolve as we define the
imaging model and move our renderers to using it.
@AustinMReppert
Copy link
Contributor

What was the selection criteria for these clip geometries? Are you planning to add other shapes like polygons, circles, ellipses, text, etc. in the future?

@waywardmonkeys
Copy link
Collaborator Author

What was the selection criteria for these clip geometries? Are you planning to add other shapes like polygons, circles, ellipses, text, etc. in the future?

These are ones that we've commonly talked about having fast paths for due to how common they are in actual usage.

There could be some others in the future, especially if we saw an opportunity to have a fast path for them.

(Clipping to text would be more likely to use clipping to a path or stroke, not something dedicated. That's already supported in this model.)

Copy link
Contributor

@raphlinus raphlinus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm inclined to approve this (modulo the comments) but I have some concerns about the direction.

I don't feel I yet 100% understand the scope of the imaging_model crate. It seems like it can potentially abstract over some of the rendering functionality. I'm wondering whether SkPicture like functionality is envisioned as ultimately being in scope.

From what I can see, one of the biggest potential benefits is the ability to abstract over imaging operations that are actually provisioned by a system compositor (for example, in CALayer by creating a CAShapeLayer for the Bézier path and then setting that as the mask of the layer to be clipped). From that perspective, I expect the scope to include operations provided by a rich compositor, which includes opacity and blending, filter effects, and perhaps a bit more (but probably not everything in Core Animation, the particle effects stuff).

I'm going to reserve judgment for now about taking a dependency on imaging_model from Vello; I can be convinced but I'm also seeing some risks.

/// Transform applied to the clip shape.
///
/// This does not affect subsequent draws; it only affects how the clip shape is interpreted.
transform: Affine,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not obvious to me why the transform belongs in the clip geometry object, it feels to me it belongs either in the graphics state or as an argument to the clip method.

Fill {
/// Transform applied to the clip shape.
///
/// This does not affect subsequent draws; it only affects how the clip shape is interpreted.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. It might be worth looking at the 1982 Warnock and Wyatt paper, as they are quite precise about this, and in many ways, that work is closer to the modern imaging model work than the PS/Cairo/HTML5 evolution; in particular, geometry for clip/fill/stroke is an object passed to the drawing method rather than belonging to graphics state.

The "subsequent draws" language is not great as normative documentation, as it implies a temporal sequencing. I think a more precise way of saying this is that the transform is applied to the clip geometry but not to the content affected by the clip.

}

// Conservative outset to account for width, caps, and miter joins.
let outset = 0.5 * stroke.width.abs() * stroke.miter_limit.abs().max(1.0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You only need to account for the miter limit when the the stroke's join is Join::Miter.

Also, I'm not sure whether the .abs().max(1.0) belongs here, or should be treated as validation. According to W3C spec, values less than 1 are invalid. We don't (currently) document that in kurbo, but arguably should.


/// Conservative axis-aligned bounds of this geometry.
///
/// Returns `None` if the geometry bounds are non-finite.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I understand the point of making these return values options, it forces the client to consider the non-finite case, where in the current world there is substantial risk of nan values flowing through to consumers where they will more likely than not wreak havoc. I'm not sure I love it though, it feels like it adds nontrivial ceremony.

Stroke {
/// Transform applied to the clip shape.
///
/// This does not affect subsequent draws; it only affects how the clip shape is interpreted.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a question for you: is the transform applied to the geometry before stroking, or after? I believe the right answer is the latter, but it needs to be clear.

fill_rule: Fill,
},
/// Clip to the filled outline of a stroked shape.
Stroke {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This raises some questions. In some use cases, for example drawing to HTML5 canvas or making SVG output, implementing this requires path-to-path stroke expansion, which increases binary size. And, as far as I can tell, the capability isn't required by any existing 2D content, so it's not 100% clear to me what the motivation is for including it.

return None;
}

// Conservative outset to account for width, caps, and miter joins.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok, and having fast reasonably-accurate bounds is a useful feature. In practice, I think it will be very unlikely people will specify a miter limit greater than the default 4.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants