Images

[Celtoys]

Have you thought much about adding image widgets? Along with that: buttons as widgets, checkboxes, radio buttons, etc.

I'm adding it to my fork at the moment but it would help you had any thoughts on how you'd approach it.

One option of course is to just add fonts to the glyph.

[dwilliamson]

It was pretty simple to add with the minor exception of how to control the active image index. My commit is 2d7192ffc4c7b6e3b2d25c31447cde1f4e0bd1b8

Let me know if you would be interested in pulling something into the main repo and if so, what changes you'd want to see.

[ocornut]

Excellent stuff ! Yes I'd like to pull the feature. Some quick comments:

• Image() and ImageButton() are lots of use so they're cool, but out of curiosity what are you using ImageRadioButton() for? I am concerned about an explosion of functions if people expect to use images to "reskin" ImGui. If that is the intent maybe the system needs to be designed another way (Also see discussion here https://github.com/ocornut/imgui/issues/17 ).
• The index should probably be changed to a void* to let the user store what they want. Maybe typedef ImTextureID void* and by specification say that 0/null refer to the font texture to preserve compatibility.
• Image buttons could uses a border on hover/click instead of tinting perhaps, something to try?
• uv+uv_size could be passed as uv0/uv1 it seems like a more standard way of specifyign uvs?
• The declaration of Image() in ImGui.h is a oddly positioned as the first function in the list. I'd suggest putting Image/ImageButton next to each other after SmallButton().

[dwilliamson]

OK, cool... I've added some changes here: c9363d3ef64b7b26eb95e724cefdada24cdc8a87

what are you using ImageRadioButton() for?

It's a big grid of images where each image represents a material/texture you can select.

I am concerned about an explosion of functions if people expect to use images to "reskin" ImGui. If that is the intent maybe the system needs to be designed another way

Re-skinning was out of the remit of the changes I'm making but there is definitely cross-over. My instinct would be to include it and change/remove it later once the scope of re-skinning becomes more obvious.

The index should probably be changed to a void* to let the user store what they want

I'm happy to do this but it presents two minor issues with the selection of the font page. This is my inner loop at the moment:

    u32 last_image_index = (u32)-1;
    for (u32 i = 0; i < m_DrawCommands.size(); i++)
    {
        const ImGuiDraw& cmd = m_DrawCommands[i];
        if (cmd.image_index != last_image_index)
        {
            rapi::Device_SetPSResources(device, 0, 1, &m_TextureResources[cmd.image_index]);
            last_image_index = cmd.image_index;
        }
        rapi::Device_SetScissorRects(device, 1, &cmd.scissor);
        rapi::Device_Draw(device, rapi::PRIM_TriangleList, cmd.nb_vertices, cmd.vertex_offset);
    }

m_TextureResources stores pointers to private types in my engine. It maps directly to D3D11: so I expose an opaque type for ID3D11Texture2D that users pass around and don't require them to create/get the equivalent ID3D11ShaderResourceView object in order to use it as a texture. For the client this would imply something along the lines of:

ImGui::Image(texture->ShaderResourceView(), ....

Of course the user to cache their own array of SRVs like the engine does internally but this all just makes the client's work too verbose.

I expect other engines to have a similar issue but you may have a different idea about this.

The second issue is that without the ability to tell ImGui what a null texture ID actually points to, the engine inner loop needs to check for this special case on each pass:

    ImTextureID last_texture_id = ImTextureID_Null;
    for (u32 i = 0; i < m_DrawCommands.size(); i++)
    {
        const ImGuiDraw& cmd = m_DrawCommands[i];
        if (cmd.texture_id != last_texture_id)
        {
            rapi::ShaderResource* srv = (rapi::ShaderResource*)cmd.texture_id;
            if (srv == ImTextureID_Null)
                srv = ... // srv for font page
            rapi::Device_SetPSResources(device, 0, 1, &srv);
            last_texture_id = cmd.texture_id;
        }
        rapi::Device_SetScissorRects(device, 1, &cmd.scissor);
        rapi::Device_Draw(device, rapi::PRIM_TriangleList, cmd.nb_vertices, cmd.vertex_offset);
    }

To circumvent that you could of course had a field in `ImGuiIO that points to your null texture pointer (the font page) but it all seems a little complicated to me.

I'm currently a bit feverish so much of this may not make any sense ;)

[dwilliamson]

Oh, a quick thought but it's probably a bit too late for something like this. To limit API explosion in a system such as this I've used method chaining before: http://www.parashift.com/c++-faq-lite/named-parameter-idiom.html

So you'd do:

Image(index,x,y);
Button(name).Image(index,x,y);
RadioButton(name).Image(index,x,y);

Of course it has its own fair share of issues, like: it's not immediately obvious what combination of modifiers will work unless you try it out!

[dwilliamson]

Actually, I can get around the first issue by calling ShaderResourceView() on-demand in the engine code. That would mean adding a single ImGuiIO field that points to the font texture would solve everything else. Does that sound like a good idea?

[ocornut]

Yes that sounds good since it would be 0/null and then can be ignored by most users. So the field would be something like,

// after FontFallbackGlyph ImTextureId FontTextureId; // = NULL // Texture id ImGui will use in ImDrawCmd when it wants to use the font texture. Ignore if not using image textures

And we may want an explanatory paragraph on using Image/Textures at the top of the file. I'm not sure if the sample code should handle that. It would make sense but I don't want reduce clutter of the sample because they create a mental barrier against new users integrating the library and lowering this mental is a key idea behind ImGui engineering principles.

It's a big grid of images where each image represents a material/texture you can select.

On the ImageButton* colors I find unlikely that using color multiplier will be a satisfying-enough solution in the long run. Especially a 0.67 multiplier as the default may be unacceptable to some use cases? So as we may want something different for the "selected/active" visualization of images.

Adding a wire-frame border or Button() style framing may be a more acceptable general solution for ImageButton and ImageRadioButton ? But I can see color multiplying also being useful.

ImGuiCol_ImageButtonActive is currently used by both ImageButton and ImageRadioButton but they aren't necessarily the same thing. There's isn't a parallel with non-image button/radio button because the normal radio uses ImGuiCol_CheckActive

My suggestion would be along those lines:

• Uses bordering to visualize hovering/active, rename ImGuiCol_ImageButton\ to ImGuiCol_ImageButtonBorder**
• Add ImGuiCol_ImageButtonBorderSelected and ImGuiCol_ImageButtonBorderUnselected
• Add a 'int selected = -1' parameter to ImageButton(). 0: unselected, 1: selected, Find a way that makes sense to visualize both "selected" and "hovered/active" state.
• Add a ImGuiCol col_multiplier parameter to ImageButton() for user controlled color multiply

It makes the function a little heavier/intimidating but then ImageButton can act as ImageRadioButton, the later can be removed and we've got lots of flexibility.

Extra

• ImageButton() doesn't have uv, ImageButtonUV() two extra parameters. default to (0.0) and (1.1) - so ImageButton just calls ImageButtonUV as a convenience.

Extra

• Add ImGuiCol_ImageButtonColMultiply* so the function can handle both color multiplication and borders to visualize the state, but that is set in the theme. The default setting can use a combination or bordering and color multiplying? That would add a total of 10 color entries.. might feel too complicated?

What do you think? I haven't actually tried to "use" that so I may be completely off with real world needs here.

[ocornut]

This should be in now.

We have two functions which should cover most cases. ImageButton() by default pad the image like a normal button. You can make the padding invisible with frame_padding=0 and a different size e.g. frame_padding=1 one pixel.

void   Image(ImTextureID user_texture_id, const ImVec2& size, const ImVec2& uv0 = ImVec2(0,0), const ImVec2& uv1 = ImVec2(1,1), const ImVec4& tint_col = ImVec4(1,1,1,1), const ImVec4& border_col = ImVec4(0,0,0,0));
bool   ImageButton(ImTextureID user_texture_id, const ImVec2& size, const ImVec2& uv0 = ImVec2(0,0),  const ImVec2& uv1 = ImVec2(1,1), int frame_padding = -1, const ImVec4& bg_col = ImVec4(0,0,0,1));    // <0 frame_padding uses default frame padding settings. 0 for no paddnig.

I have NOT added ImageRadioButton() yet. The reason is that I want to introduce a proper concept of what is "selected" and this will affect things in a more general way.

Right now you can mimic a radio button by poking into the button color, e.g.

PushStyleCol(ImGuiCol_Button, ImColor(255,255,255))

Which should be bearable. Also note I added a (totally optional) helper called ImColor() which serves to automatically convert to 4 float to 1 bytes.

[ocornut]

@dwilliamson Hope it works well for you. A fair amount has changed since November since I added the font atlas / truetype so your patch for texture handling will conflict with the new version.

[ocornut]

The demo window shows examples. It is a bit awkward because the example app don't have access to textures other than the font :)

Also showing color buttons easy to setup using ImColor::HSV I may add a global Hue-offset slider to the style editor.

[dwilliamson]

Great job! I'm so far away from being able to sync to latest that this won't become an issue for me until around March or so. I'll probably just clobber my changes completely and update the broken user code.