Skip to content

Conversation

@longmathemagician
Copy link
Contributor

CoreGraphicsContext::draw_image_area() doesn't account for coregraphics' reversed y-axis, 086e8e6 fixes that by passing the cropped image to CoreGraphicsContext::draw_image() which does draw correctly.

@xStrom
Copy link
Member

xStrom commented Aug 16, 2022

Please rebase on master to fix clippy errors and then also run cargo fmt.

Calls CoreGraphicsContext::draw_image() from CoreGraphicsContext::draw_image_area() after cropping the image.
@xStrom xStrom added the piet-coregraphics issue in the CoreGraphics backend label Aug 17, 2022
@longmathemagician longmathemagician marked this pull request as ready for review August 17, 2022 19:12
@longmathemagician
Copy link
Contributor Author

Added one last (hopefully) commit to this branch: 5df4eec fixes capture_image_area for y-up contexts by tracking the y-direction in CoreGraphicsContext and translating the context's origin if necessary.

@xStrom
Copy link
Member

xStrom commented Aug 18, 2022

I haven't looked into it deeper yet, but the test image is definitely broken now.

coregraphics-test-16-1 00
.

@longmathemagician
Copy link
Contributor Author

Dang, this actually broke a lot of stuff that wasn't in my test case. Getting à la cart piet snapshot testing set up locally and then harmonizing y-direction handling across druid with and without bitmap caching (linebender/druid#2246) may take me a day or two.

If you want I can just shelve the commit, it's not likely to help anyone who isn't also using bitmap render caching of some form in druid.

@xStrom
Copy link
Member

xStrom commented Aug 18, 2022

No rush, we can see what else comes up.

@longmathemagician
Copy link
Contributor Author

Not thrilled about adding overhead in 6bbf3a1 but this is just about ready to go. Piet tests, standard druid windows, and bitmap-cached druid windows all draw properly. Still fighting a couple of coregraphics functions (mostly CGImageCreateWithImageInRect, which does not actually strip excess data when cropping so capture_image_area is still bugged if you want to access raw image data). Mixing y-up and y-down context content ``works'' but is approximately as inadvisable now as before.

I made a handful of changes to test pattern 16 in be1da57 to troubleshoot y-flip issues. A bunch of the test images fail out of the box on my system (macOS 12.5, perhaps coregraphics itself has changed) so I hesitate to make a PR to the snapshot repo. That image now looks like this:
test_image

@longmathemagician
Copy link
Contributor Author

Oops, I was using as_ref in another branch and didn't catch that with clippy. Some form of unwrapper is needed for sharing assets across contexts with differing y-directions, so I just made as_ref public and axed pub fn copy_image since it didn't actually copy yet (a weirdly difficult thing for coregraphics it seems).

For the snapshots, I don't know if the standard here is to have more comprehensive tests or faster/simpler ones. If simpler is better I can just drop be1da57.

@longmathemagician
Copy link
Contributor Author

longmathemagician commented Aug 19, 2022

Pulled the changes to test image 16, I'll throw that in a separate PR. Took care of the last issue I had with coregraphics in f5abbb1, as long as I didn't break anything else in the process this branch is ready for review / merge.

…as to not conflict with as_ref trait

Edit: Fix typo in commit message
@jneem
Copy link
Member

jneem commented Nov 17, 2022

@longmathemagician can I go ahead and merge this? Or is it waiting on the updated test image 16?

@longmathemagician
Copy link
Contributor Author

@jneem you're good to merge it, thank you. There are still a lot of issues with piet-coregraphics I never got around to tackling but this is at least an improvement over the current state.

@jneem jneem merged commit ad55c40 into linebender:master Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

piet-coregraphics issue in the CoreGraphics backend

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants