Integrate ninepatch into theming

[desttinghim]

This is my first attempt at integrating the ninepatches into theming. I don't think this implementation is the best way to handle it, but I don't have a better idea (yet).

[RedPhoenixQ]

For size reasons and to avoid copies, would it work to pass ?*NinePatch in Options. I would think it's likely that you would reuse the same ninepatch texture for multiple elements, perhaps even to the point were you have a global variable you can pass a pointer to wherever you need it.

[desttinghim]

I agree that ninepatches will probably be referenced a lot. I think the final system will be like the font database + font ids, but what to use for the key it is not as clear as with fonts.

[RedPhoenixQ]

but what to use for the key it is not as clear as with fonts.

It's probably fine to have string names like FontId with some named enum fields for the builtin ones, like the win98 one.

For simplicity just passing pointers around and defining a global like you have done in the examples seems plenty good. You can even modify the default options for something like the ButtonWidget and not even need to pass anything around.

[david-vanderson]

This is a great start! Love the aesthetic you get from that theme.

For size reasons and to avoid copies, would it work to pass ?*NinePatch in Options.

I also think this is a decent thing to try. My guess is that most of the time a ninepatch will either be in the theme or be a global variable - hard to imagine copying them around.

I'm in favor of switching to pointers in Options, testing this theme some more, and then merging. We can then get some experience using it to know if more infrastructure is necessary.

[desttinghim]

Instead of making the Options.ninepatch_* fields into pointers, I changed the Ninepatch struct to be more of a "fat pointer" type, with the uv field now being a pointer. Let me know if that seems ok.

[david-vanderson]

Instead of making the Options.ninepatch_* fields into pointers, I changed the Ninepatch struct to be more of a "fat pointer" type, with the uv field now being a pointer. Let me know if that seems ok.

With 3 optional pointers to ninepatch, Options is 392 bytes big. With the fat pointers, Options is 464. That makes me worried, but I don't have any hard data to say how big is too big. Right now the pointers seems better - what was the reasoning behind making them fat pointers? Also I suppose we could have both - optional pointers in Options to the fat pointers.

Unrelated, for that theme you might want to set the new Theme.max_default_corner_radius to zero to remove all the rounded corners?

[david-vanderson]

As I've been playing more with this, there are a lot of situations where the widget is too small for the normal ninepatch rendering (animations, resizing windows). So now I think we should remove error.NinepatchBelowMin. And if the space is too small for normal, we should not log anything and draw as much (starting from the top left) as we can.

[desttinghim]

I think that makes sense. Thinking out loud, we'll need to recalculate the UVs to fit. If one of the dimensions is zero we can skip rendering, otherwise we'll render the top edge, left edge, or top left corner cropped.

I should note that the ninepatches I am using are way larger than necessary, and I am planning to reduce them. I think 5px by 5px should be sufficient for the windows 98 look.

[desttinghim]

Right now the pointers seems better - what was the reasoning behind making them fat pointers? Also I suppose we could have both - optional pointers in Options to the fat pointers.

Sorry for the long wait, but I initially held off on the pointer approach because it made integration into theming more difficult. Specifically, using a pointer in the options complicates the lifetime of the Ninepatch struct significantly. It requires there to be a stable memory location to point to, and it can't be initialized at compile time because it contains a texture. I've given the simplest approach a try but I'm running into memory protection errors. Using a pointer will need a caching mechanism like fonts.

[david-vanderson]

Sorry for the long wait, but I initially held off on the pointer approach because it made integration into theming more difficult. Specifically, using a pointer in the options complicates the lifetime of the Ninepatch struct significantly. It requires there to be a stable memory location to point to, and it can't be initialized at compile time because it contains a texture. I've given the simplest approach a try but I'm running into memory protection errors. Using a pointer will need a caching mechanism like fonts.

Ah I understand. In that case we probably want Ninepatch to use ImageSource and have dvui handle texture creation/deletion automatically. Does that make sense?

(Sorry I didn't mention it before, I really should have)

[david-vanderson]

themeEditor is currently broken here.

@phatchman I'm running into a problem with struct_ui that I can't figure out. We added ?dvui.Ninepatch to Theme. Ninepatch doesn't make sense to display the internals of, so we are trying to prevent struct_ui from displaying anything inside with:

    const ninepatch_options: dvui.struct_ui.StructOptions(dvui.Ninepatch) = .init(.{}, dvui.Theme.builtin.win98.control.ninepatch_fill.?);

And passing that into displayStruct as one of the options. Should we be able to stop struct_ui from looking into Ninepatch this way?

The specific error I'm getting is:

src/struct_ui.zig:993:25: error: field bytes for struct Texture.ImageSource.ImageFile does not support default initialization
                        @compileError(std.fmt.comptimePrint("field {s} for struct {s} does not support default initialization", .{ field.name, @typeName(T) }));

[RedPhoenixQ]

ImageSource doesn't make much sense to expose through struct_ui. You have to provide pointers to data which isn't reasonable to do via a text entry. Just opting out of showing that field is probably ok for now.

I think we should instead provide a dropdown or similar swapping between the buildins (or loading an image from a file path). Would be a good showcase for how to do custom rendering for a subset of fields (or reason to make this possible in struct_ui, like a display option of custom that holds a custom display function that takes a pointer to the field as an argument maybe)

[phatchman]

themeEditor is currently broken here.

@phatchman I'm running into a problem with struct_ui that I can't figure out. We added ?dvui.Ninepatch to Theme. Ninepatch doesn't make sense to display the internals of, so we are trying to prevent struct_ui from displaying anything inside

One of the changes I made was to make the field options processed at runtime, rather than comptime. This means even if a field is not displayed, the code to enable the display of that field is still generated. So you need to provide default values for any optional structs / unions that cannot be default intialized.

Hmm. This should work by adding default values for all of the Structs / Unions it errors on e.g.

 const texture_options: dvui.struct_ui.StructOptions(dvui.Texture) = .init(.{}, .{ .ptr = &.{}, .width = 0, .height = 0 });

But this isn't working for ImageSource and I'm not sure why. i.e.

    const image_source_options: dvui.struct_ui.StructOptions(dvui.Texture.ImageSource) = .init(.{}, .{ .texture = .{ .ptr = &.{}, .width = 0, .height = 0 } });

Still results in an error message displayed. I'll need to investigate further.

You want to just comment it out for now? I'll work out how to fix it. And I'll think about how we could do @RedPhoenixQ suggestions as an enhancement.

I did some experimentation previously about how to exclude fields fully at comptime, but wasn't sure if the complexity of adding it was worth it without seeing some real-world need.

Edit: Probably the issue is that you will need to provide StructOptions with default values for all members in the ImageSource union. Because the user could potentially initialize any value in the union from the UI (if it was shown), I think it will need default values provided.

[david-vanderson]

You want to just comment it out for now? I'll work out how to fix it. And I'll think about how we could do @RedPhoenixQ suggestions as an enhancement.

Good plan, will do.

I did some experimentation previously about how to exclude fields fully at comptime, but wasn't sure if the complexity of adding it was worth it without seeing some real-world need.

Edit: Probably the issue is that you will need to provide StructOptions with default values for all members in the ImageSource union. Because the user could potentially initialize any value in the union from the UI (if it was shown), I think it will need default values provided.

That makes sense for the union, but I was trying to get Ninepatch, a normal struct, to not display any of its fields (one field is ImageSource). Maybe there is some short-circuiting we need to do.

Thanks!

[phatchman]

It is what I thought. Each member of the ImageSource union needs to be supplied with default values, even though they would never be shown. The below code fixes it.

   const color_options: dvui.struct_ui.StructOptions(dvui.Color) = .init(.{
        .r = .{ .number = .{ .min = 0, .max = 255, .widget_type = .slider } },
        .g = .{ .number = .{ .min = 0, .max = 255, .widget_type = .slider } },
        .b = .{ .number = .{ .min = 0, .max = 255, .widget_type = .slider } },
    }, .{ .r = 127, .g = 127, .b = 127, .a = 255 });
    const ninepatch_options: dvui.struct_ui.StructOptions(dvui.Ninepatch) = .init(.{}, dvui.Theme.builtin.win98.control.ninepatch_fill.?);
    const texture_options: dvui.struct_ui.StructOptions(dvui.Texture) = .init(.{}, .{ .ptr = &.{}, .width = 0, .height = 0 });
    const image_source_opts1: dvui.struct_ui.StructOptions(@FieldType(dvui.Texture.ImageSource, "pixelsPMA")) = .init(.{}, .{ .rgba = &.{}, .width = 0, .height = 0 });
    const image_source_opts2: dvui.struct_ui.StructOptions(@FieldType(dvui.Texture.ImageSource, "pixels")) = .init(.{}, .{ .rgba = &.{}, .width = 0, .height = 0 });
    const image_source_opts3: dvui.struct_ui.StructOptions(dvui.ImageSource.ImageFile) = .init(.{}, .{ .bytes = &.{} });
    const theme: *dvui.Theme = &dvui.currentWindow().theme; // Want a pointer to the actual theme, not a copy.
    if (dvui.struct_ui.displayStruct(
        @src(),
        "Theme",
        theme,
        2,
        .default,
        .{ color_options, texture_options, ninepatch_options, image_source_opts1, image_source_opts2, image_source_opts3 },
        null,
    )) |box| {
        box.deinit();
    }

I reminded myself of all the reasons why excluding fields at comptime is difficult. Basically, how do you specify which field in a series of nested structs should be excluded. Ideally, we'd be able to exclude fields through the field options, making them comptime was previously explored in the original solution and I discarded that due to complexity and difficulty to debug.

I think the best I've come up with is to introduce a new displayStructWithExclusions function that takes a comptime slice of field names to be excluded. The limitation here is that fields would be excluded from every struct recursively displayed by that function, regardless of nesting level. The advantage is it is it's simple and I'm a big fan of keeping comptime simple.

Edit: I'm cleaning up a nicer patch and will push later. Edit2: That is pushed now. It should now display any ninepatch options as read-only. Default values for NinePatch is set to null as it will never be created by struct_ui. Edit3: I could make the lack of default value a runtime debug logging error. You would then only see the error if you use struct_ui to create that struct/union, otherwise you won't see an error. The downside is that you lose comptime checking that all default values are provided.

[david-vanderson]

@phatchman Thank you!

[david-vanderson]

@desttinghim I'm unsure about how ninepatch rendering should work with the colormods and transparent parts of the source. Can you give me some usecases for how I should be thinking about these?

[david-vanderson]

@desttinghim I'm unsure about how ninepatch rendering should work with the colormods and transparent parts of the source. Can you give me some usecases for how I should be thinking about these?

I now understand why you might want transparent parts. The background and ninepatch have been decoupled, so you could draw any color background (or none), and then a ninepatch on top of it with transparent parts.

Couldn't think of anything for the colormods, so left them out for now.

Thank you!