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

Update Peniko #769

Merged
merged 1 commit into from
Dec 18, 2024
Merged

Conversation

waywardmonkeys
Copy link
Contributor

No description provided.

@waywardmonkeys
Copy link
Contributor Author

This builds on linebender/peniko#85

entry.1 = self.epoch;
entry.0
} else if self.map.len() < RETAINED_COUNT {
let id = (self.data.len() / N_SAMPLES) as u32;
self.data.extend(make_ramp(stops));
self.map.insert(stops.into(), (id, self.epoch));
self.map
.insert(CacheKey(ColorStops(stops.into())), (id, self.epoch));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not thrilled about this change to ColorStops(stops.into()) rather than perhaps going and doing some better impls for From or something in Peniko.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

linebender/peniko#86 allows for CacheKey(ColorStops::from(stops)) / CacheKey(stops.into()).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But really this copying of the color stops just to serve as key seems suboptimal since what we could probably do this with the Range<usize> that we have available in the resolver code. But that's not for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't currently do that ... I just tried, but we can't rely on data in the Resources::color_stops being uniqued, so we can't use the range of it as a key.

@waywardmonkeys
Copy link
Contributor Author

@DJMcNab This is to help make sure that all is well before we go and publish the new version of Peniko so this doesn't have to land by itself yet ...

@waywardmonkeys waywardmonkeys marked this pull request as draft December 18, 2024 04:42
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 think this is fine. IWBN to avoid the need to allocate for get to allocate. But given that we probably should be using the entry API here anyway (which would allocate), it's probably fine.

@@ -35,13 +36,14 @@ impl RampCache {
}

pub(crate) fn add(&mut self, stops: &[ColorStop]) -> u32 {
if let Some(entry) = self.map.get_mut(stops) {
if let Some(entry) = self.map.get_mut(&CacheKey(ColorStops(stops.into()))) {
Copy link
Member

Choose a reason for hiding this comment

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

This is not ideal, as it allocates "unnecessarily". It really complicates the traits in the Color crate to make this work nicely, I think.

@waywardmonkeys waywardmonkeys force-pushed the update-peniko branch 2 times, most recently from 31e7634 to 4a3f672 Compare December 18, 2024 16:58
@waywardmonkeys waywardmonkeys marked this pull request as ready for review December 18, 2024 17:01
@waywardmonkeys waywardmonkeys added this pull request to the merge queue Dec 18, 2024
Merged via the queue into linebender:main with commit a71236c Dec 18, 2024
17 checks passed
@waywardmonkeys waywardmonkeys deleted the update-peniko branch December 18, 2024 17:22
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