Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core Graphics back-end #176

Merged
merged 30 commits into from May 11, 2020
Merged

Core Graphics back-end #176

merged 30 commits into from May 11, 2020

Conversation

raphlinus
Copy link
Contributor

This is still work in progress. It's based off #4, updated for API changes and so on. In its current state, paths work and there is code for images, but nothing on gradients or text yet.

Copy link
Member

@xStrom xStrom left a comment

Choose a reason for hiding this comment

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

The CI changes look fine apart from some naming.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@cmyr cmyr marked this pull request as ready for review May 7, 2020 00:28
@cmyr cmyr force-pushed the cg branch 5 times, most recently from 222dd59 to 681332f Compare May 8, 2020 18:44
Copy link
Contributor Author

@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.

Generally looks good to me. It says I can't approve my own pull request, but feel free to approve and merge after addressing my concerns (also happy to discuss any open questions either here or in Zulip chat).

The situation around wrapped macOS types is deeply unsatisfying, what I propose here feels like an ok solution, but I would hope for improvement in the foundational libraries (difficult, though, without breaking existing clients).

writer.write_image_data(cg_ctx.data()).unwrap();
}

fn unpremultiply(data: &mut [u8]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wondering about the duplication, whether it's possible to get this built with the png option. But also willing to accept that the Cargo feature plumbing would be too much of a pain :)

Copy link
Member

Choose a reason for hiding this comment

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

Just not sure where it would live? I guess we could have some common-utils module or something in piet...

CoreGraphicsContext::new_y_up(&mut cg_ctx, HEIGHT as f64 * SCALE.recip());
piet::draw_test_picture(&mut piet_context, test_picture_number).unwrap();
piet_context.finish().unwrap();
std::mem::drop(piet_context);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is unpremultiply not needed here as well?

Copy link
Member

Choose a reason for hiding this comment

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

good point. What's a good way to rest the premultiplication? I don't really understand the tradeoffs here, or whether we even want to be using premultiplied alpha?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general, premultiplied is faster for rendering, but PNG stores only "separated." What I generally do for testing is render a large object with semi-transparency. For example, a white rectangle at 50% opacity will appear as gray if rendered as premultiplied and saved as png without correction.

piet-coregraphics/src/ct_helpers.rs Show resolved Hide resolved
piet-coregraphics/src/ct_helpers.rs Show resolved Hide resolved
piet-coregraphics/src/ct_helpers.rs Show resolved Hide resolved
pub(crate) line_y_positions: Vec<f64>,
/// offsets in utf8 of lines
line_offsets: Vec<usize>,
pub(crate) frame_size: Size,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to self while reviewing: should this be a Rect rather than a Size, or can we always assume the origin is at zero?

Copy link
Member

Choose a reason for hiding this comment

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

this is a size. It's hard to even say what origin means here, but in some sense it is at zero. (the drawing origin is the baseline of the first line, but we aren't given it explicitly)

piet-coregraphics/src/text.rs Show resolved Hide resolved
piet-coregraphics/src/text.rs Outdated Show resolved Hide resolved
piet-coregraphics/src/text.rs Outdated Show resolved Hide resolved
piet-coregraphics/src/text.rs Outdated Show resolved Hide resolved
piet-coregraphics/src/lib.rs Outdated Show resolved Hide resolved
piet-common/src/cg_back.rs Outdated Show resolved Hide resolved
jrmuizel and others added 16 commits May 11, 2020 11:29
Still work in progress, but it draws and saves a PNG.
Still work in progress as images are not tested, but should be a start.
I actually consider the "unused" warnings to be useful, as they signal
where implementation is needed, but grepping for "unimplemented" also
works :)
This doesn't do hit testing, but it does paint multiline text.
This just makes our API cleaner? Ideally?
This involves stashing our source string, and generating a map to get
utf8 offsets from utf16 offsets.

This also adds stashing of line origins, which we will need for hit
testing.
This is not currently extensively tested, and there may be some funny
little things, but it gets us started.
cmyr added 12 commits May 11, 2020 11:29
This isn't well supported in the core-graphics crate, so we have
to do a bit of our own FFI; there's a weird combination of traits
and types used for FFI between the different core-foundation and
core-graphics crates, and it was hard to get them to play nicely
together, so this ends up throwing various aliases out the window
for FFI.
And 'implement' status, which is apparently always peachy.

This is the last bit of feature work; after this we'll just have
fixups and getting geometry right etc.
This can execute the examples defined in piet-test.
These are small changes required to work with piet-common and druid.
Also includes other small manifest changes to get that all working.
This involves explicitly handling the different coordinate spaces
between piet and coregraphics. I've chosen to be explicit; when
creating a CoreGraphicsContext, you must either ask for y-up and pass
in a height (so that we can apply a transform under the hood) or you
can ask for y-down and handle conversion yourself (which plays nicely
with Cocoa, and NSView.isFlipped.
This was just broken and not well enough tested.
Relying on CoreGraphics here causes problems from druid; because
druid-shell for mac flips the coordinate system, there is a transform
applied to the context before we see it that we want to ignore.
A bunch of small fixes. Larger things include:

- reusing unpremultiply_rgba function
- allowing explicit CGGradientDrawingOptions
- better signatures in FFI
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

great stuff, wonderful, amazing really 🤩

@cmyr cmyr merged commit 723e95f into master May 11, 2020
@cmyr cmyr deleted the cg branch May 11, 2020 18:39
@cmyr cmyr mentioned this pull request May 11, 2020
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.

None yet

4 participants