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

Impl serde for DynamicColor #70

Merged

Conversation

waywardmonkeys
Copy link
Collaborator

Using DynamicColor for color stops in Peniko will require this (when Peniko's serde feature is enabled).

Using `DynamicColor` for color stops in Peniko will require this
(when Peniko's `serde` feature is enabled).
@waywardmonkeys waywardmonkeys added this to the 0.2.0 milestone Nov 26, 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.

This doesn't expose any new capabilities in terms of creating values, so it's probably fine.

I must admit, I'm not massively convinced that this is the serialisation format we'll want in the long-term. Can we document that the serialisation/deserialisation format isn't stable between color versions?

@waywardmonkeys
Copy link
Collaborator Author

This is here so that things that enable serde in peniko will continue to work.

I don't think anyone should rely on serde remaining stable across breaking releases.

I'm also not really sure how great using serde on peniko (or color) would be ... just trying to keep the same set of things working.

@waywardmonkeys waywardmonkeys added this pull request to the merge queue Nov 26, 2024
Merged via the queue into linebender:main with commit 8ce1814 Nov 26, 2024
15 checks passed
@waywardmonkeys waywardmonkeys deleted the serde-for-dynamic-color branch November 26, 2024 12:03
@DJMcNab
Copy link
Member

DJMcNab commented Nov 26, 2024

I'm also not really sure how great using serde on peniko (or color) would be ... just trying to keep the same set of things working.

This isn't really our vibe; that being said, I'm perfectly happy for us to be pragmatic. I think having this thread of discussion that we are making the tradeoff for efficiency of intentionally not thinking about it is perfectly reasonable.

@waywardmonkeys
Copy link
Collaborator Author

I mean that peniko currently has a serde feature (as does kurbo), but I don't really know how or if that's used out in the wild. I could ask @ratmice!

@ratmice
Copy link

ratmice commented Nov 26, 2024

I'm also not convinced that serde is the serialization format I want to use,
My usage of serialization for peniko, is for a project which has never been released in the wild,
I am flexible with regards to changing it's serialization.

Selvage does publicly turn the feature on, and it is used in the examples (serializing to a string buffer to show that it works), but that is about the extent of it. selvage example usage

Even then the usage of serialization is more for the purposes of code organization,
keeping random shapes and their paint materials outside of the main code base, which is complicated enough without them.

Some of the issues with serde for peniko are described in linebender/peniko#30
I would kind of like to try rkyv, but kurbo had support for serde already, and I wasn't sure if there was desire or appetite for experimenting with something else.

Edit: Added link to the example.
Edit2: I'm not aware of selvage having any users of it the wild beyond my own unreleased usage, the API is explicit in it's experimental status and lack of semver compatibility in the README, and this carries over to the serialization format IMO.

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