Honor max_vertices in RenderGeometryRaw

[kbolino]

Currently, the signature for Renderer.RenderGeometryRaw looks like this:

func (renderer *Renderer) RenderGeometryRaw(
	texture *Texture,
	xy []float32, xy_stride int,
	color []Color, color_stride int,
	uv []float32, uv_stride int,
	num_vertices int,
	indices interface{},
) (err error)

However, the parameter num_vertices is ignored and the actual number of vertices passed to SDL_RenderGeometryRaw is taken from the xy slice length instead:

	_num_vertices := C.int(len(xy))

This sort of deduction is fine for RenderGeometry which takes a slice of sdl.Vertex. But here, for RenderGeometryRaw, the slices being supplied may contain interleaved positions (XYs), colors, and texcoords (UVs). This means that the number of vertices may not be obvious from any of the slice lengths: it could be len(xy)/3 if all 3 values are interleaved, it could be len(xy)/2 if only XYs and UVs are interleaved, it could be len(xy) if there's no interleaving, or it could be something else entirely if there's padding involved. SDL can handle all of these cases on its own but right now go-sdl2 can't.

[kbolino]

Further thoughts: given the low-level nature of RenderGeometryRaw, it may be best to discard the slice parameter types in favor of something more generic. I know the C signature uses float * and SDL_Color * types, which usually map nicely to []float32 and []sdl.Color, but I think those types are actually misleading. For example, what if the stride is not a multiple of 4? I don't know if a stride of say 5 would actually work across all renderers but the API doesn't say not to do it.

Two options come to mind: replace the slice types with the equivalent pointers, i.e. *float32 and *sdl.Color, which remains misleading IMO but matches the underlying C API signature better, or use unsafe.Pointer to make it totally apparent that there be dragons here. Either would be a breaking change to the public API, but RenderGeometryRaw has sharp edges, so I don't think anyone would be surprised if they got cut.

[veeableful]

Hi @kbolino, thank you for pointing this out! I was thinking to use *float32 and *sdl.Color but I see that you think it's misleading. Could you elaborate more on that to explain why you think it's misleading?

[kbolino]

Sure. The biggest reason I think it's misleading is because the strides are specified in bytes. Here's an example of how uv and uv_stride are used inside SDL_RenderGeometryRaw:

for (i = 0; i < num_vertices; ++i) {
    const float *uv_ = (const float *)((const char*)uv + i * uv_stride);
    /* ... */
}

If we look at the renderer implementations as well, e.g. for Metal in METAL_QueueGeometry, we can see similar pointer arithmetic based on xy_stride and color_stride:

for (int i = 0; i < count; i++) {
    int j = indices[i]; /* effectively */
    float *xy_ = (float *)((char*)xy + j * xy_stride);
    *((SDL_Color *)verts++) = *(SDL_Color *)((char*)color + j * color_stride);
    /* ... */
}

So the apparent type is thrown away in favor of char * briefly to do some pointer arithmetic. Under the hood, the real array type is one of bytes.

Moreover, between these two functions, there was no check I saw that uv_stride, xy_stride, nor color_stride had to be multiples of 4 (by comparison, num_indices does have to be a multiple of 3, since a triangle is always composed of 3 vertices). So it's misleading IMO to suggest you're passing an array of float32s or Colors when those types are both 4 bytes wide and yet you can tell SDL to skip over a number of bytes which isn't a multiple of the type's size. A 4-byte type with 4-byte alignment would be densely packed in an array, but a stride of 5 implies there's 1 byte of padding for some reason. And even if you do specify a multiple of 4, you might still be skipping over a field. If you stored 4 bytes of flags at the end of each vertex, for whatever reason, your stride would be 24, but one of those 4-byte fields would be neither float32 nor Color.

The second reason is because of possible interleaving. The three pointers can point into the same region of memory (plus small offsets). For example, I can arrange an output buffer from Nuklear's nk_convert function so that the buffer is filled with a sequence of XY, color, UV in that order. Then I can pass the zeroth byte of the buffer, the eighth byte of the buffer, and the twelfth byte of the buffer as xy, color, and uv respectively to RenderGeometryRaw with all the strides set to 20. Each component of a vertex is going to be obtained by leapfrogging over the other two. So this region of memory is filled with what the program is interpreting as heterogenous data. It's not properly an array of float nor is it an array of Color, it's actually an array of struct{x, y float32;c Color;u, v float32}.

Having put to digital paper all of that, I think you should go with *float32 and *sdl.Color ultimately. There's two reasons that are probably better than my reasons above:

1. It matches, as closely as possible, the upstream library's function signature, which helps minimize confusion for programmers who "speak" SDL2 first and foremost
2. If we flip our semantic understanding from float * meaning "an array of floats" to simply "a pointer to a value which happens to be float" then it's a lot less misleading -- as I noted already, RenderGeometryRaw has sharp edges

Building on point 2, I could also argue that, in C, the float pointers should have been SDL_FPoint pointers, since you're really pointing to an 8-byte-wide XY or UV vector and not just a single float component thereof, but I think I have more than exhausted the need for pedantry already.

[veeableful]

Thank you for writing such comprehensive reasons! I agree that it might be better to match closer to SDL2 function and I did think of the *float32 and *sdl.Color types to be pointers to the values instead of arrays and they could also be clues of what you would normally put into the address.

I have made the change in fed6d258 where I fix the unused num_vertices parameter and change the parameter types to use pointers. If it all looks good to you, I will tag it as the next patch version!

UPDATE: I tagged v0.4.16 including this change due to a separate issue that needs fixing but let me know if there's a problem with the change and I can make another tag.

[kbolino]

I tested it out and it works fine. Definitely more ergonomic to pass pointers instead of slices, since I had to make fake slice headers for them before.