Change path_data's type to store values as f32

[PoignardAzur]

This is useful on its own for simplicity, but it's also useful to more easily validate data.

Add validate_path_data as an example, though down the line we'd probably want a more holistic validation method that includes unbalanced tags, NaNs in transforms, infinite values, etc.

[waywardmonkeys]

This feels strange coming in at roughly the same time as #817 which handles both the path_data and the draw_data (although it changes them to u32 which matches the shaders).

I'd be more inclined to see #817 land first, but I see there's also some minor differences in the changes here and the corresponding changes in #817. If those are meaningfully different, perhaps they should be a code review on #817.

After that, perhaps changing the u32 to an f32 might make sense, but it seems like both the CPU side buffers and the GPU side buffers should agree on the type and interpretation of the data.

[waywardmonkeys]

(Also, if I read the GPU side of this correctly, it isn't all f32 data anyway ...)

[PoignardAzur]

This feels strange coming in at roughly the same time as #817 which handles both the path_data and the draw_data (although it changes them to u32 which matches the shaders).

Huh, I didn't notice that PR.

(Also, if I read the GPU side of this correctly, it isn't all f32 data anyway ...)

Multiple data streams are merged before they reach the GPU. Some of them aren't f32, but path_data is always f32.

[DJMcNab]

Multiple data streams are merged before they reach the GPU. Some of them aren't f32, but path_data is always f32.

I'm unsure how you reached that conclusion. The prior discussion of this idea clarified that this wasn't the case (at least on the GPU side). And indeed, the code referred to does still exist on main at the time of writing.

vello/vello_shaders/shader/flatten.wgsl

Lines 622 to 627 in 782fb5a

	fn read_i16_point(ix: u32) -> vec2f {
	let raw = scene[pathdata_base + ix];
	let x = f32(i32(raw << 16u) >> 16u);
	let y = f32(i32(raw) >> 16u);
	return vec2(x, y);
	}

and

vello/vello_shaders/shader/flatten.wgsl

Lines 713 to 722 in 782fb5a

	} else {
	p0 = read_i16_point(pathseg_offset);
	p1 = read_i16_point(pathseg_offset + 1u);
	if seg_type >= PATH_TAG_QUADTO {
	p2 = read_i16_point(pathseg_offset + 2u);
	if seg_type == PATH_TAG_CUBICTO {
	p3 = read_i16_point(pathseg_offset + 3u);
	}
	}
	}

Certainly, one could make arguments for removing the i16 point support, since it is not really used. But as it stands, your validation method would have false positives for some correct uses of the API, and so cannot reasonably be added as-is.

[DJMcNab]

This also interacts with #820, incidentally. On a big-endian host, we'd need to encode these as little endian bytes, which would not be the same f32 value.

[tomcur]

This also interacts with #820, incidentally. On a big-endian host, we'd need to encode these as little endian bytes, which would not be the same f32 value.

(This and your PR together is what made me wonder whether the current encoding is strictly correct.)

[PoignardAzur]

I'm unsure how you reached that conclusion.

I'll confess I didn't read the shaders while writing this PR. I guess I should have re-read that zulip conversation first.

I reached that conclusion because none of the Rust code that writes to path_data ever encodes anything other than f32s in it. Though since the Encoding fields are public, someone could always manually add those i16 points to the bitstream.

Honestly, I'm not sure we should even keep the i16 code, given that we don't ever exercise those paths? Maybe some external Vello user does (I'd be shocked), maybe we'll add it back later, but in the meantime it's basically an undocumented API which I'm pretty sure we aren't testing.

If we do keep the i16 code, we could always store the i16 points in a different range of the buffer, with its own offsets and such. That way we should still store f32s as such.

This also interacts with #820, incidentally. On a big-endian host, we'd need to encode these as little endian bytes, which would not be the same f32 value.

Fair enough. I'll note that you always have to encode f32s to bytes somewhere, this PR just changes what the somewhere is.

[PoignardAzur]

Making it a draft for now, since the concerns raised make it unmergeable.