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

Make fine.wgsl integer literals explicitly signed #418

Merged
merged 1 commit into from
Dec 20, 2023

Conversation

pengiie
Copy link
Contributor

@pengiie pengiie commented Dec 20, 2023

The previous literals were being interpreted as an AbstractInt, which according to the u32 constructor:

If T is i32, this is a reinterpretation of bits (i.e. the result is the unique value in u32 that has the same bit pattern as e).

If T is AbstractInt, this is an identity operation if the e can be represented in u32, otherwise it produces a shader-creation error.

I made the literals explicit signed integers so the correct constructor overload is used.

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'm not going to merge, as I haven't run this change.

But if someone else can confirm this still executes as expected, that would be helpful.

Is there a corresponding Naga issue? i.e. tracking that the abstractint doesn't get maintained through unary minus (?)

@@ -311,7 +311,7 @@ fn fill_path_ms(fill: CmdFill, local_id: vec2<u32>, result: ptr<function, array<
let mask1_exp = (mask_b >> 4u) & 0x1010101u;
var mask1_signed = select(mask1_exp, u32(-i32(mask1_exp)), is_down);
if is_bump {
let bump_delta = select(u32(-0x1010101), 0x1010101u, is_down);
let bump_delta = select(u32(-0x1010101i), 0x1010101u, is_down);
Copy link
Member

Choose a reason for hiding this comment

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

Good catch, thanks!

@@ -400,7 +400,7 @@ fn fill_path_ms(fill: CmdFill, local_id: vec2<u32>, result: ptr<function, array<
}
// packed_w now contains the winding numbers for a slice of 4 pixels,
// each relative to the top left of the row.
for (var i = 0u; i < local_id.y >> 2u; i++) {
for (var i = 0u; i < (local_id.y >> 2u); i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this consistent with elsewhere in our source?

I think the relational expression binds more tightly than the shift operation

@simbleau
Copy link
Member

Related: linebender/bevy_vello#17

@DJMcNab DJMcNab merged commit 0dd9ed2 into linebender:main Dec 20, 2023
4 checks passed
@DJMcNab
Copy link
Member

DJMcNab commented Dec 20, 2023

Thanks for the PR!

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