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

chore: update to vello 0.4 #49

Merged
merged 14 commits into from
Jan 30, 2025
Merged

chore: update to vello 0.4 #49

merged 14 commits into from
Jan 30, 2025

Conversation

simbleau
Copy link
Member

No description provided.

@@ -152,13 +151,13 @@ jobs:
os: [windows-latest, macos-latest, ubuntu-latest]
include:
- os: ubuntu-latest
gpu: 'yes'
gpu: "yes"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change all of these? Having them differ from all of the other Linebender CI tasks would be annoying. We can change them everywhere, but would want to know why it is needed here.

Copy link
Member Author

@simbleau simbleau Jan 27, 2025

Choose a reason for hiding this comment

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

These got autoformatted since the top of the file uses " instead of '
e.g. RUST_MIN_VER: "1.82"

I think whatever vscode extension I have wanted to make it consistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just don't stage those changes in the PR then, please.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

values[2] as f32,
values[3] as f32,
values[4] as f32,
]),
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the range on these values before? Was it 0.0 to 1.0 or 0 to 255? If it is 0 to 255, then we'll have to divide them here as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Alternatively, use Color::from_rgba()?)

Copy link
Member Author

Choose a reason for hiding this comment

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

The old Color::rgba method took normalized colors

https://docs.rs/peniko/0.2.0/peniko/struct.Color.html#method.rgba

Logic is correct

(Alternatively, use Color::from_rgba()?)

No such method exists in the new color crate

Some([x0, x1, x2]) => (*x0, *x1, *x2),
_ => (0.0, 0.0, 0.0),
};
Color::new([x0 as f32, x1 as f32, x2 as f32, 1.0])
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question about the range of these values here as elsewhere in the PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

These are normalized, 0.0 to 1.0: https://docs.rs/peniko/0.2.0/peniko/struct.Color.html#method.rgb

The new logic is correct

let stop = peniko::ColorStop::from((offset as f32, peniko::Color::rgba(r, g, b, a)));
let stop = peniko::ColorStop::from((
offset as f32,
peniko::Color::new([r as f32, g as f32, b as f32, a as f32]),
Copy link
Contributor

Choose a reason for hiding this comment

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

And same question here...

Copy link
Contributor

Choose a reason for hiding this comment

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

(But I do think this one is fine...)

Copy link
Member Author

Choose a reason for hiding this comment

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

CHANGELOG.md Outdated
@@ -66,6 +70,8 @@ This release has an [MSRV][] of 1.75.

- Initial release

[#49][https://github.com/linebender/velato/pull/49]
Copy link
Contributor

Choose a reason for hiding this comment

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

That's not the right syntax. There is a list of them further below, so it should just at the end of that list, with the right syntax.

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops, good catch. Fixed (actually this time).

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.

Approving to unblock. I haven't carefully reviewed this code

@@ -139,8 +149,8 @@ pub struct Animated<T: Tween> {

impl<T: Tween> Animated<T> {
/// Returns the value at the specified frame.
Copy link
Member

Choose a reason for hiding this comment

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

What does a "failure" here (where the or would be used) mean? Should we document that?

I'd naively expect it to e.g. return the first value of the t is before the timeline, and the last if after, instead.

The action for this comment would be documentation, not changing behaviour. And this needn't block this pr.

Copy link
Member Author

Choose a reason for hiding this comment

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

The new Color doesn't have Default, so I had to add a new method, _or which passes in a provided default.

@simbleau simbleau added this pull request to the merge queue Jan 30, 2025
Merged via the queue into main with commit db7617a Jan 30, 2025
16 checks passed
@simbleau simbleau deleted the simbleau/vello-0.4 branch January 30, 2025 16:31
not(target_arch = "wasm32"),
expect(
clippy::large_enum_variant,
reason = "Deferred. Furthermore, for some reason, only on wasm32, this isn't triggering clippy."
Copy link
Member

Choose a reason for hiding this comment

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

Probably because of an internal isize.

@@ -42,7 +42,7 @@ where
}

// Function signature must match Serde's Serialize trait, so need to suppress Clippy.
#[allow(clippy::trivially_copy_pass_by_ref)]
#[expect(clippy::trivially_copy_pass_by_ref, reason = "Deferred")]
Copy link
Member

Choose a reason for hiding this comment

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

The reason is present in the comment above.

Suggested change
#[expect(clippy::trivially_copy_pass_by_ref, reason = "Deferred")]
#[expect(clippy::trivially_copy_pass_by_ref, reason = "Function signature must match Serde's Serialize trait")]

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.

4 participants