Skip to content

masonry: Use Kurbo via Vello.#223

Merged
waywardmonkeys merged 1 commit intolinebender:mainfrom
waywardmonkeys:use-kurbo-via-vello-or-peniko
Aug 16, 2024
Merged

masonry: Use Kurbo via Vello.#223
waywardmonkeys merged 1 commit intolinebender:mainfrom
waywardmonkeys:use-kurbo-via-vello-or-peniko

Conversation

@waywardmonkeys
Copy link
Contributor

@waywardmonkeys waywardmonkeys commented Apr 26, 2024

Rather than have to keep a dependency on kurbo at the correct version, always use it via vello.

This makes this match how peniko is already used within masonry.

@waywardmonkeys
Copy link
Contributor Author

This does leave some pub use of kurbo stuff in crates/masonry/src/lib.rs that I think a subsequent PR should follow up and remove so that the top-level exports of masonry are cleaner and more of just what's needed.

@DJMcNab
Copy link
Member

DJMcNab commented Apr 26, 2024

I think we need to re-export vello and maybe winit from Masonry? I'm not sure what the best API here looks like - there are some unclear abstraction boundaries. At the moment, xilem_masonry has to depend directly on vello and winit, which probably isn't correct. See also linebender/vello_svg#5

I would propose postponing this until after RustNL.

The phrasing via "vello or peniko" there is quite telling, imo, that this is still messy.

@waywardmonkeys
Copy link
Contributor Author

The vellol or peniko is only because I included changes for xilem_web which uses peniko but not vello.

@DJMcNab
Copy link
Member

DJMcNab commented Apr 26, 2024

Ah, apologies. That makes sense.

I still think this isn't straightforwardly a win, and needs more careful thought.
It's not really good DX to have multiple paths to the same item up and down the stack, but it's also not good DX to require a specific neighbour dependency - especially when it's only to refer to the types, as it is with Vello, here.
Maybe we re-export all of Vello except for the Kurbo re-export, and re-export Kurbo seperately at the root?

And so overall, would still prefer to defer this for 3 weeks or so

@djc
Copy link

djc commented May 8, 2024

It looks like a win to me. If most usage of Kurbo will happen either by crates that need to depend on Vello or (in the case of xilem_web) via Peniko it makes sense then re-exporting Kurbo in Vello makes sense (and so I would also say that linebender/vello_svg#5 does not make much sense to me). In fact I already noticed that xilem_web has a kurbo dependency that it doesn't end up using so removing that would definitely make sense.

@Philipp-M
Copy link
Member

In fact I already noticed that xilem_web has a kurbo dependency that it doesn't end up using so removing that would definitely make sense.

It's used though? In the svgtoy for example?

But I guess we can put kurbo and other things there behind a feature flag.

@djc
Copy link

djc commented May 8, 2024

If it's used (directly) only for the examples it should be a dev-dependency.

@waywardmonkeys
Copy link
Contributor Author

@djc @Philipp-M See #293, I just split that off.

@waywardmonkeys waywardmonkeys force-pushed the use-kurbo-via-vello-or-peniko branch from 984796c to ff29738 Compare May 8, 2024 14:18
@waywardmonkeys
Copy link
Contributor Author

I've rebased this PR forward, but left in the part that I did in #293 (I'll rebase again).

This should make things more clear that this patch isn't about how we re-export, but about how we use, and this just brings the usage of kurbo within masonry in line with how peniko is used: Not via a direct dep, but via the vello re-export.

This also cleans up a number of places where multiple paths to kurbo (which might have pointed at different crate versions) were used within a single file.

I'd previously done (and landed) a version of this in xilem_classic in PR #90.

I've edited the commit message and description on the commit in this PR to reflect that it isn't going to be involving anything but masonry (once I rebase it forward again after #293 lands hopefully).

@waywardmonkeys waywardmonkeys force-pushed the use-kurbo-via-vello-or-peniko branch from ff29738 to 180c661 Compare May 8, 2024 14:23
@waywardmonkeys waywardmonkeys changed the title Use Kurbo via Vello or Peniko. masonry: Use Kurbo via Vello. May 8, 2024
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

I find this frustrating, because it is not clear what the right rules are here.

I now realise that this is an improvement - apologies, I hadn't fully understood the actual changes being suggested here. I had assumed that this was only doing pub use vello::kurbo

use kurbo::{Affine, Stroke};
use smallvec::{smallvec, SmallVec};
use tracing::{trace, trace_span, Span};
use vello::kurbo::{Affine, BezPath, Cap, Join, Size, Stroke};
Copy link
Member

Choose a reason for hiding this comment

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

I think I'd prefer these all to use crate::{...}

If we need the types, we should export them directly, probably.

@waywardmonkeys waywardmonkeys force-pushed the use-kurbo-via-vello-or-peniko branch from 180c661 to 8b899a1 Compare May 8, 2024 14:57
@waywardmonkeys
Copy link
Contributor Author

It is frustrating, I agree! And what's worse is that just naively accepting suggestions from rust-analyzer or other tools to resolve imports may or may not do the "right" thing (whatever that even is).

We had a pub use kurbo before, so in my last rebase (moments ago, after #293), I added in the pub use vello::kurbo; to keep existing code seeing the same thing.

In that case, we could do:

use crate::turbo::Foopy;

or

pub use crate::vello::kurbo::Foopy;

Or the current:

pub use vello::kurbo::Foopy;

This brings up another thing which is that the import styles are also not identical across things, so some stuff will do:

use vello::kurbo::Line;
use vello::peniko::Image;
use vello::Scene;

while others might do:

use vello::{kurbo::Line, peniko::Image, Scene};

So ... I'm happy to make them all do the same thing, I just need to know what that thing should be.

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.

Based on discussion in Zulip we can move forward in principle.

Next step would be to rebase on main to resolve merge conflicts.

@waywardmonkeys
Copy link
Contributor Author

I'll do that this week.

@flosse flosse added the masonry Issues relating to the Masonry widget layer label Aug 13, 2024
@waywardmonkeys waywardmonkeys force-pushed the use-kurbo-via-vello-or-peniko branch from 8b899a1 to 1b0678f Compare August 16, 2024 12:20
Rather than have to keep a dependency on `kurbo` at the correct
version, use it consistently via `vello`.

This makes this match how `peniko` is already used within `masonry`.
@waywardmonkeys waywardmonkeys force-pushed the use-kurbo-via-vello-or-peniko branch from 1b0678f to 77a4e87 Compare August 16, 2024 12:22
@waywardmonkeys waywardmonkeys requested a review from DJMcNab August 16, 2024 12:23
@waywardmonkeys
Copy link
Contributor Author

I've resurrected this and rebased it.

Copy link
Member

@Philipp-M Philipp-M left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@waywardmonkeys
Copy link
Contributor Author

FYI @PoignardAzur

Copy link
Contributor

@PoignardAzur PoignardAzur left a comment

Choose a reason for hiding this comment

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

LGTM.

@waywardmonkeys waywardmonkeys added this pull request to the merge queue Aug 16, 2024
Merged via the queue into linebender:main with commit 052ac39 Aug 16, 2024
@waywardmonkeys waywardmonkeys deleted the use-kurbo-via-vello-or-peniko branch August 16, 2024 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

masonry Issues relating to the Masonry widget layer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants