Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upEnable raqote as 2D canvas rendering backend by default #24470
Conversation
highfive
commented
Oct 17, 2019
|
Heads up! This PR modifies the following files:
|
|
I will proceed working on this as described in #24422 (comment). |
jrmuizel
commented
Oct 18, 2019
|
If you post test results I may be able to help with some of the problems. |
jrmuizel
commented
Oct 20, 2019
|
It looks like there are some panics inside of path building. This is presumably happening because the PathBuilder is being used after finish() is called. |
|
@pylbrecht Currently get_current_point does: let path = self.finish();If we do: let path = self.finish();
self.0 = Some(path.clone().into());Then we should be able to avoid the current problems without needing to add a new API to raqote. |
|
Thanks for the alternative solution! WPT tests are currently running. I'll post a log as soon as the process finishes. // edit: |
|
If you push the code change we can run a test run on CI. |
|
Some common failures:
All of /2dcontext/shadows/ is expected because we don't implement shadows yet. Failures like this one make me suspect that raqote might be using antialiasing, but we turn that off in tests in the azure backend: We should modify the DrawOptions to disable it when appropriate. This failure looks like an issue that's exposed because we're now comparing floating point values when we did not used to, so it's actually a missing part of our DOM canvas implementation (filed #24540): |
|
I think is_point_in_path may be still be incorrect and causing failures like: It's possible that we are checking the points in the untransformed path, when we should be ensuring that the path has had any transformations applied to it before checking for the presence of a point in it. |
|
There are various failures related to arc drawing that suggest that we're not doing it right: |
|
This failure shows that we're ignoring the Filter argument for draw_surface, and we should be including it with https://github.com/jrmuizel/raqote/blob/58cf1a43c02555cfa74571a6c12ed896d39d2dc9/src/draw_target.rs#L203 somehow. |
jrmuizel
commented
Oct 24, 2019
|
It would be good to know the parameters to SrcAtop::blend that cause the assertion to fail. |
jrmuizel
commented
Oct 24, 2019
|
Yeah, |
|
Thanks for the detailed feedback! I'll dig through it on the weekend. Maybe even by tomorrow, but I can't say for sure. |
|
After applying the transformation in pub fn contains_point(&self, x: f64, y: f64, path_transform: &Transform2D<f32>) -> bool {
self.as_raqote().clone().transform(path_transform).contains_point(0.1, x as f32, y as f32)
}Some of the tests' assertions succeeded, but not all. Especially this last one is still failing: I tried changing the tolerance to |
Return false from CanvasState::is_point_in_path for NaN/infinite values Servo doesn't pass WPT test `/2dcontext/path-objects/2d.path.isPointInPath.nonfinite.html` when built with raqote (see [here](#24470 (comment))). This change adds a missing check for NaN/infinite values in `CanvasState::is_point_in_path` and fixes this. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #24540 - [X] These changes do not require tests because WPT tests cover it
|
Regarding antialiasing: setting it to About the ignored
Neither of them is implemented yet, correct? Furthermore regarding |
|
With respect to the antialiasing question, there are other uses of DrawOptions::new() in that file, so try changing the antialiasing member for all of them. As for the filter question, I linked earlier to the Image variant of the Source enum, which contains a FilterMode enum. We don't have the filter information available when the source surface is created (although we do have the repeat x and y info, so we should fix the TODO there), but we can steal the implementation of DrawTarget::draw_image_with_size_at for our DrawTarget::draw_surface and pass appropriate arguments there. |
|
@jrmuizel All of the azure backends have code to invert the transform and apply it to the point for their ContainsPoint implementations. Should the raqote backend do something like that as well? |
jrmuizel
commented
Oct 29, 2019
Yeah, that's a better thing to do than transforming the path. |
|
Reading 2d.path.clip.basic.1.html, I translated it into a unit test for raqote and filed jrmuizel/raqote#91. It looks like my antialiasing guess was incorrect and it's a bug related to compositing inside raqote when there is clipping present. |
What parameters exactly? I looked through the involved operations and the only thing I've found in that context was DrawOptions::blend_mode. |
|
@bors-servo retry |
Enable raqote as 2D canvas rendering backend by default <!-- Please describe your changes on the following line: --> This will enable raqote by default and make all WPT tests pass. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #23431 <!-- Either: --> - [X] There are tests for these changes <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
|
|
|
@bors-servo retry |
Enable raqote as 2D canvas rendering backend by default <!-- Please describe your changes on the following line: --> This will enable raqote by default and make all WPT tests pass. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #23431 <!-- Either: --> - [X] There are tests for these changes <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
|
|
|
@bors-servo retry |
Enable raqote as 2D canvas rendering backend by default <!-- Please describe your changes on the following line: --> This will enable raqote by default and make all WPT tests pass. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #23431 <!-- Either: --> - [X] There are tests for these changes <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
|
|
Add pylbrecht to try users. @pylbrecht is working on replacing azure with raqote in servo/servo#24470 and running the full WPT suite on CI is often useful.
pylbrecht commentedOct 17, 2019
•
edited
This will enable raqote by default and make all WPT tests pass.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors