Added support for user defined UV type.

[joeriedel]

Hi Ocornut,

I added the ability for the ImGui user config to specify a custom UV type so we could render images that are in texture arrays. By default with nothing defined everything is the same as before (ImVec2 is used).

In the ImGui user config you can now #define IMUV MyType along with several other helper macros that allow the code to work in the relatively few places that UVs are manipulated in ImGui.

Example code to use ImVec4 as UVs (which is what we do):

#define IMUV ImVec4 #define IMUV_00 ImVec4(0,0,0,0) #define IMUV_11 ImVec4(1,1,0,0) #define IMUV_01 ImVec4(0,1,0,0) #define IMUV_10 ImVec4(1,0,0,0) #define IMVEC2_TO_UV(_x) ImVec4((_x).x, (_x).y, 0, 0) #define IMUV_XYXY(_xy) ImVec4((_xy).x, (_xy).y, (_xy).x, (_xy).y) #define IMUV_XY(_a, _b) (_a).x, (_b).y, (_a).z, (_a).w

[ocornut]

Hello @joeriedel and thanks for this.

This looks unnecessarily complicated.

so we could render images that are in texture arrays.

1. I don't understand why this has to do with texture array. Supposedly your imgui rendering function could use any shaders of your choice. Couldn't your shaders emit the additional 0,0 while sampling from the texture array?

2. Or, why not just using the IMGUI_OVERRIDE_DRAWVERT_STRUCT_LAYOUT facility and adding a dummy 2 floats after the UV value?

(1) is preferred. With (2) or your solution you are adding unnecessary burden/cost CPU side, increase memory cost. With your solution the code also gets more complicated.

[joeriedel]

Hi @ocornut,

The use case is for rendering images that aren't just on layer 0 of a texture array. Textures can be on any layer. Consider doing ImageButton() using UVs on layer 5 of a texture array? You are either forcing me to allocate memory for every combination of texture array and layer and passing that in as the texture ID, and then on the backend I have to patch the vertex data before it's uploaded to fill in the extra UVs, or set a shader constant, or something like that, and then of course ImGui is going to unnecessarily break every Image into its own draw command at layer boundaries which is unnecessary and inefficient. All of that is much worse than passing through a couple extra floats into the vertex data. It's a pure win from fewer draw calls alone.

1. This doesn't allow you to render from any layer in the texture array.

2. IMGUI_OVERRIDE_DRAWVERT_STRUCT_LAYOUT only gets us a couple extra coordinates but ImGUI doesn't provide any mechanism for filling in those extra fields. As I said before I'd have to piggy back the information in the TextureId parameter, patch the vertex command data on the backend, and ImGui is going to be breaking up all my Image calls into individual draw commands because the TextureId is changing.

With my change here, there is no cost if you don't use it, and if you do want to use an extra UV the cost is far far less than what you'd actually have to do to make this actually support texture arrays. This is the most efficient way of support texture arrays in ImGui.

[ocornut]

OK I understand your problem better.

Although the PR is mostly right (*), my issue is that this adds cognitive and maintenance cost for what is a very rarely used feature. My role is to avoid both.

I'd like to challenge the importance of that:

is going to unnecessarily break every Image into its own draw command at layer boundaries which is unnecessary and inefficient.

Yes, this is absolutely correct, but it is a meaningful bottleneck for your use of dear imgui ?

How many simultaneous images may be visible and what is the effective cost of drawing them? Consider we already have two draw calls per window (which itself is technically an inneficiency I can and would like to address, but it never has been a top of priority and hasn't really been requested by anyone).

I presume your system in place doesn't facilitate the possibility that those different images could be in a texture texture (aka address them with a pair of UV) instead?

(*) I would suggest renaming IMUV to ImUV to match other structures (we don't put an emphase on this being a define. If anything this could even be a typedef. Then split those changes into 2 commits where the first commit only changes ImVec2 to ImUV in the right places, then second commit actually add the actual feature.

[joeriedel]

My role is to avoid both.

I totally agree and support your decision to not incorporate this if you don't think it's worth the trouble.

Yes, this is absolutely correct, but it is a meaningful bottleneck for your use of dear imgui ?

No it's not really that important (but I think it has a much bigger effect than passing the extra floats around). I think it could get significant if you have a list of hundreds of images.

I presume your system in place doesn't facilitate the possibility that those different images could be in a texture texture (aka address them with a pair of UV) instead?

It works because the vertex stride doesn't change, you don't need a separate vertex decl or special shader code for the regular texture case, as long as your vertex decl does a sizeof(ImDrawVert). In our engine we do switch shaders if necessary to go from an array to a regular texture but that's it.

(*) I would suggest renaming IMUV to ImUV to match other structures (we don't put an emphase on this being a define.

I'm more than happy to go ahead and make the changes you suggest and resubmit the PR. ImGui is really a great tool thank you for your hard work.

[joeriedel]

One issue with your suggestion there with the typedef @ocornut is (I had done this initially) but I can't typedef using any ImGui types in the config header since they aren't defined, so most of this does have to be done with macros. Thoughts?

[ocornut]

I'm more than happy to go ahead and make the changes you suggest and resubmit the PR. ImGui is really a great tool thank you for your hard work.

FYI you can force-push to that same branch you used for the PR so it all stays within the same PR number. However given the discussion above my intuition right now would be to not merge the PR at the moment. Unmerged PR are sort of useful for others and as reference of desirable things to support (e.g. if we were to redesign some of that code it may be more amenable to merge in that idea), however it is probably not in your best interest to keep running with a patch which is likely to often conflicts.

I'm tempted to just suggest dropping it (mostly because I have a thousand other fishes to fry) and encourage you to try running with the extra draw calls. If at some point you feel the extra cost become actually a problem, we'll always have this PR+patch are a reference and we can reconsider.

I think it could get significant if you have a list of hundreds of images.

My intuition is, actual inline images would be capped to whatever can be reasonably made visible in a single UI screen. So perhaps if you have an image browser in place it could reach 100+ but it is not going to stray toward the thousands?

If you were to be able to merge all images into a single draw calls you would still be left in the awkward situation where any non-image ImGui elements would submit their own vertices and create a draw call. You can work-around this of course by carefully submitting only images separated from UI elements but that will hinder your agiliby/flexibility with that UI code.

(Then, there is the possibility you are using small "icon-ish" images very frequently, in which case if it is a finite set it is recommended to stick them in the main atlas (see fonts/README.ttt about font icons merging, and AddCustomRect api to submit rect for packing that you can map into Unicode point and render into.))

either forcing me to allocate memory for every combination of texture array and layer and passing that in as the texture ID, and then on the backend I have to patch the vertex data before it's uploaded to fill in the extra UVs,

For the sake of exhausting esoteric possibilities :)

• You may be able to use callbacks to submit the layer (in order to avoid allocating structures for every layer).
• Since indices are consumed in order, your renderer could indeed decide to recreate enlarged vertices and merge draw calls, which is what you suggested above. It's a bit of cruft but assuming images consume very few vertices would actually be cheap.

can't typedef using any ImGui types in the config header since they aren't defined, so most of this does have to be done with macros. Thoughts?

Hmm that's right. I'd suggest adding in that section of imgui.h: (May be wrong, I haven't tested the whole thing)

#ifndef ImUV
typedef ImVec2 ImUV;
// other stuff
#endif

And in the case the user wants the change it they can #define it in imconfig.h. This seems analoguous to what we do with ImDrawIdx. (And it is a small annoyance that IMGUI_OVERRIDE_DRAWVERT_STRUCT_LAYOUT can't use the same method!).

[joeriedel]

Thanks for your detailed thoughts. For now I think we can just leave the PR here. I don't mind lugging around the patch (it's not that bad) and applying it when we pull from upstream occasionally. Really tbh the docking branch works great we probably won't pull it again for a long time unless there's something else there.

We use this for a terrain painting palette so I'd do my best not to try and merge these into the main texture atlas, since there could be hundreds of textures, it's also just kind of a pain to use instead of the terrain atlas which is already right there so having the ability to draw with the array's is great.

If you were to be able to merge all images into a single draw calls you would still be left in the awkward situation where any non-image ImGui elements would submit their own vertices and create a draw call.

This is absolutely true and I hadn't though of that. We would have to merge the atlas into the imgui one in order to share everything in one batch. We'd also have to maintain a UV lookup for each terrain texture -> imgui atlas which is another housekeeping task (if we ever ended up needing to do this).

Thanks again!

[ocornut]

IMGUI_OVERRIDE_DRAWVERT_STRUCT_LAYOUT only gets us a couple extra coordinates but ImGUI doesn't provide any mechanism for filling in those extra fields

How about using this to replace ImVec2 uv with a custom type which can be constructed from ImVec2 and can implicitely convert to ImVec2?

[NukeBird]

How about using this to replace ImVec2 uv with a custom type which can be constructed from ImVec2 and can implicitely convert to ImVec2?

Actually it sounds like a great idea, such a ctor hacks will reduce code complexity (we won't be forced to pass vec4 if we need just a vec2)