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

Image x/y extend, alpha, nearest neighbor sampling #766

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

Conversation

dfrg
Copy link
Collaborator

@dfrg dfrg commented Dec 12, 2024

This adds support for image extend modes, alpha and nearest neighbor sampling to fine and plumbs support for all through the pipeline.

Extend modes with bilinear filtering (bottom right is x repeat and y reflect):
image_extend_bilinear

Extend modes with nearest neighbor filtering:
image_extend_nearest

Image alpha:
image_alpha

dfrg added 2 commits December 12, 2024 01:12
This adds support for image extend modes, alpha and nearest neighbor sampling to fine and plumbs support for all through the pipeline.
@dfrg dfrg mentioned this pull request Dec 12, 2024
Copy link
Contributor

@waywardmonkeys waywardmonkeys left a comment

Choose a reason for hiding this comment

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

LGTM. I don't know if we need to quantize the alpha down to 8 bits and store it with the other stuff, but okay for now.

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 should have a changelog entry.

I'd also like to see some new tests here; these should be very similar to the existing tests, and so should be straightforward. Indeed, one of the tests needs to be updated.

Otherwise, only relatively peripheral concerns.

Cargo.toml Show resolved Hide resolved
image_extend_modes_helper(scene, ImageQuality::Medium);
}

pub(super) fn image_extend_modes_nearest_neighbor(scene: &mut Scene, params: &mut SceneParams) {
Copy link
Member

Choose a reason for hiding this comment

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

We could impl this in a way similar to longpathdash, which is another parameterised test scene

vello_encoding/src/encoding.rs Show resolved Hide resolved
@@ -415,6 +415,10 @@ impl Encoding {
.extend_from_slice(bytemuck::bytes_of(&DrawImage {
xy: 0,
width_height: (image.width << 16) | (image.height & 0xFFFF),
sample_alpha: ((image.quality as u32) << 12)
| ((image.x_extend as u32) << 10)
| ((image.y_extend as u32) << 8)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe? It would be worth debug_asserting that these all fit. Since these enums are exhaustive, having an exhaustive match #[cfg(test)]ed somewhere would also be sufficient.

@@ -164,6 +164,8 @@ pub struct DrawImage {
pub xy: u32,
/// Packed image dimensions.
pub width_height: u32,
/// Packed quality, extend mode and 8-bit alpha (bits `qqxxyyaaaaaaaa`).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Packed quality, extend mode and 8-bit alpha (bits `qqxxyyaaaaaaaa`).
/// Packed quality, extend mode and 8-bit alpha (bits `qqxxyyaaaaaaaa`, 18 unused prefix bits).

Comment on lines +1172 to +1174
let r = extend_mode(atlas_uv.x * extents_inv.x, image.x_extend_mode);
let g = extend_mode(atlas_uv.y * extents_inv.y, image.y_extend_mode);
let fg_rgba2 = vec4(r, g, 0.0, 1.0);
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 this do?

}
}
}
case IMAGE_QUALITY_MEDIUM, default: {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
case IMAGE_QUALITY_MEDIUM, default: {
// We don't have an implementation for `IMAGE_QUALITY_HIGH` yet, just use the same as medium
case IMAGE_QUALITY_MEDIUM, default: {

atlas_uv = atlas_uv + image.atlas_offset;
// TODO: If the image couldn't be added to the atlas (i.e. was too big), this isn't robust
let atlas_uv_clamped = clamp(atlas_uv, image.atlas_offset, atlas_max);
let fg_rgba = premul_alpha(textureLoad(image_atlas, vec2<i32>(atlas_uv_clamped), 0));
Copy link
Member

@DJMcNab DJMcNab Dec 12, 2024

Choose a reason for hiding this comment

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

I think it would be useful to give a small comment as a hint here.

Suggested change
let fg_rgba = premul_alpha(textureLoad(image_atlas, vec2<i32>(atlas_uv_clamped), 0));
// Nearest Neighbor
let fg_rgba = premul_alpha(textureLoad(image_atlas, vec2<i32>(atlas_uv_clamped), 0));

let b = premul_alpha(textureLoad(image_atlas, vec2<i32>(uv_quad.xw), 0));
let c = premul_alpha(textureLoad(image_atlas, vec2<i32>(uv_quad.zy), 0));
let d = premul_alpha(textureLoad(image_atlas, vec2<i32>(uv_quad.zw), 0));
let fg_rgba = mix(mix(a, b, uv_frac.y), mix(c, d, uv_frac.y), uv_frac.x);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let fg_rgba = mix(mix(a, b, uv_frac.y), mix(c, d, uv_frac.y), uv_frac.x);
// Bilinear sampling
let fg_rgba = mix(mix(a, b, uv_frac.y), mix(c, d, uv_frac.y), uv_frac.x);

@nicoburns nicoburns added the enhancement New feature or request label Dec 12, 2024
@dfrg
Copy link
Collaborator Author

dfrg commented Dec 13, 2024

Thanks for the feedback, Daniel. I'm on vacation and out of town for the next week but I may have some time over the weekend to address all concerns and get this landed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants