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

Examples: Replace Color::from_* and Color::new with palette::css where possible #787

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

BloodStainedCrow
Copy link

As mentioned in #749, this replaces all uses of Color::from_* and Color::new with palette::css constants when possible.

This also allowed me to remove the import of peniko::Color, in all but one example project.
I do not know how to use the examples/scenes lib, so it is technically untested, but I tried to match the colors as best as possible, which should be good enough.

@BloodStainedCrow
Copy link
Author

I stumbled onto this by trying to copy the example code into my own project, but discovered it does not compile with the vello version on crates.io.

I assume that is a known problem? If so, this is an unfortunate barrier for newcomers :(

@BloodStainedCrow
Copy link
Author

BloodStainedCrow commented Jan 14, 2025

Clippy failed because the import is only used behind the #[cfg(feature = "wgpu-profiler")] flag, which seems to not be set. Should I put the import behind a #[cfg(feature = "wgpu-profiler")]?

@DJMcNab
Copy link
Member

DJMcNab commented Jan 14, 2025

Thanks for this! To use the scenes library, you should run one of the with_winit or headless examples, which both use scenes to provide their content.

I stumbled onto this by trying to copy the example code into my own project, but discovered it does not compile with the vello version on crates.io.

Hmm, I'm unsure how this change would have an impact there - the palettes used would also not be available on the release on crates.io.

main is used for development of future versions, and to see examples for a specific release, you should use that release's tag (https://github.com/linebender/vello/releases/tag/v0.3.0 for v0.3.0). Maybe it would help if we had a comment about this in our README?

Clippy failed because the import is only used behind the #[cfg(feature = "wgpu-profiler")] flag, which seems to not be set. Should I put the import behind a #[cfg(feature = "wgpu-profiler")]?

I think that's right. It's the solution we used to import Line.

@DJMcNab
Copy link
Member

DJMcNab commented Jan 14, 2025

How did you find these matching colours? Were they all identical to the CSS counterpart? (if the colours have changed, the snapshot tests will need to be updated)

@BloodStainedCrow
Copy link
Author

Oh, I did not realize the exact values of the example colours mattered for tests, so I just chose the ones from the css list, which matches most closely...

@BloodStainedCrow
Copy link
Author

BloodStainedCrow commented Jan 14, 2025

I stumbled onto this by trying to copy the example code into my own project, but discovered it does not compile with the vello version on crates.io.

Hmm, I'm unsure how this change would have an impact there - the palettes used would also not be available on the release on crates.io.

This change won´t help. I was just making sure, that the issue is known! Mentioning this in the README seems like a good idea, since many people (me included) learn a new crate by adapting it's examples.

@BloodStainedCrow
Copy link
Author

How are the snapshot tests generated? Since they will need to be updated.

@DJMcNab
Copy link
Member

DJMcNab commented Jan 15, 2025

You should use cargo nextest run to run the tests, and then follow the guidance to set the environment variable for blessing the tests which should be printed for you - you also need to have git lfs enabled.

@BloodStainedCrow
Copy link
Author

BloodStainedCrow commented Jan 15, 2025

Ah, that is what i feared. Some test pass on my machine with the modified colors (due to the leniency in the tests) and are therefore not updated, but since this introduces baseline error into each test, they now are much more sensitive, so are failing on i.e. macos.

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.

2 participants