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

Add serde derives #26

Merged
merged 5 commits into from
Mar 23, 2024
Merged

Add serde derives #26

merged 5 commits into from
Mar 23, 2024

Conversation

ratmice
Copy link
Contributor

@ratmice ratmice commented Mar 15, 2024

This splits off just the serde support from #16 which originally contained both serde and schemars features.
None of my projects need schemars support, I mostly added it for feature parity with kurbo, but supporting blobs nicely required some upstream patches.

This adds a dependency on serde_bytes for Blob which is also a dtolnay crate.

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.

Sorry I didn't see this sooner - I wasn't previously watching peniko

This looks good, but we need to change CI to be using cargo-hack before adding this feature. That is, implement linebender/vello#504 (and some follow-ups) for this repo.

That should mostly require copy-and-pasting from Vello's CI. I'd suggest doing that in a different PR.

CC @xStrom for awareness

@ratmice
Copy link
Contributor Author

ratmice commented Mar 22, 2024

I'll have to look through the usage of cargo hack there, I think it might require a little bit of modification for both kurbo and peniko. Particularly I believe we need an argument like --at-least-one-of std,libm. But that specifies:

Space or comma separated list of features. Skips sets of features that don't enable any of the features listed.

This flag can only be used together with --feature-powerset flag.

I don't believe we use the --feature-powerset flag in the mentioned patch though just --each-feature.

@DJMcNab
Copy link
Member

DJMcNab commented Mar 22, 2024

We can do it with two commands:

cargo hack check --features libm --each-feature

and

cargo hack check --features std --each-feature

@ratmice
Copy link
Contributor Author

ratmice commented Mar 22, 2024

Will try and look into CI things soon.

@xStrom
Copy link
Member

xStrom commented Mar 22, 2024

I'll port over all the CI changes from Vello, should have PR up in about an hour.

@xStrom
Copy link
Member

xStrom commented Mar 22, 2024

Alright #28 is ready for review.

@xStrom
Copy link
Member

xStrom commented Mar 22, 2024

Okay #28 is merged, so this PR should now be rebased and we'll see what the new CI thinks of it.

@ratmice
Copy link
Contributor Author

ratmice commented Mar 22, 2024

Seems to work great, already found one oversight.

@ratmice ratmice dismissed DJMcNab’s stale review March 22, 2024 21:21

Requested changes done in PR #28

@ratmice ratmice requested a review from DJMcNab March 22, 2024 21:24
Cargo.toml Outdated Show resolved Hide resolved
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.

I'm wondering about Font and why it didn't get the derives.

peniko/src/font.rs

Lines 6 to 13 in 6f4150f

/// Owned shareable font resource.
#[derive(Clone, PartialEq, Debug)]
pub struct Font {
/// Blob containing the content of the font file.
pub data: Blob<u8>,
/// Index of the font in a collection, or 0 for a single font.
pub index: u32,
}

Is it because the index makes serializing too unstable or something else?

src/brush.rs Outdated
Comment on lines 44 to 45
#[cfg_attr(feature = "serde", derive(serde::Serialize))]
pub enum BrushRef<'a> {
Copy link
Member

@xStrom xStrom Mar 22, 2024

Choose a reason for hiding this comment

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

You mentioned in #16 that maybe BrushRef shouldn't derive even Serialize and I'm wondering the same. If this can be only serialized and not deserialized, what is the use case for it? 🤔

However if we keep it, then perhaps StyleRef should get a Serialize derive as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, I had forgotten about that bit, so I'll go ahead and remove it. Then if someone figures out a use for it in the future it could be added then (like perhaps it can be deserialized into an owned Brush).

@ratmice
Copy link
Contributor Author

ratmice commented Mar 22, 2024

I'm wondering about Font and why it didn't get the derives.

Is it because the index makes serializing too unstable or something else?

I was just needing enough to Serialize/Deserialize strokes/fills and the necessary components to pass to those scene functions in vello. I did look at this but then this index is exactly what scared me off from adding it, I figured I would let someone who was going to use the feature/ and would be in a better place to understand what is going on with that index do so.

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.

Thanks for your efforts!

@ratmice ratmice added this pull request to the merge queue Mar 23, 2024
Merged via the queue into linebender:main with commit 9f392fc Mar 23, 2024
14 checks passed
@ratmice ratmice deleted the serde_only branch March 23, 2024 16:18
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