Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Is using zero as valid ImTextureID okay? #7090

Closed
kevyuu opened this issue Dec 4, 2023 · 13 comments
Closed

Is using zero as valid ImTextureID okay? #7090

kevyuu opened this issue Dec 4, 2023 · 13 comments

Comments

@kevyuu
Copy link

kevyuu commented Dec 4, 2023

Version/Branch of Dear ImGui:

Version: 1.83
Branch: docking

Back-end/Renderer/Compiler/OS

Back-ends: imgui_impl_glfw.cpp + Custom graphics backend
Compiler: Visual Studio
Operating System: Windows

My Issue/Question:

What is the default value of ImTextureID in ImDrawCmd. I use UINT_MAX instead of 0 as empty value for my TextureID. It is an index to a vector. Is it okay to use this. Will Imgui use nullptr as non value ? All the example I see has 0 as null value. How do I check If a DrawCmd has a valid TextureId?

@ocornut
Copy link
Owner

ocornut commented Dec 4, 2023

This is technically undefined, there’s no concept of a null value but afaik we never check the contents of this value apart from when comparing it to another. So i think UINT_MAX would be fine as long as your backend knows how to handle it. The default value in structure is indeed 0 but that gets usually overwritten.

@kevyuu
Copy link
Author

kevyuu commented Dec 4, 2023

In the backend rendering logic, when we pass the TextureID to the shader, how do we check whether the ImTextureID in ImDrawCmd is valid? Is it always use io.Fonts->TexID with zero area texcoord when there is no image to render?

@ocornut
Copy link
Owner

ocornut commented Dec 4, 2023

how do we check whether the ImTextureID in ImDrawCmd is valid?

I don't understand this question in the first place. The ImTextureID in a non-callback ImDrawCmd is always valid.

For most dear imgui commands (shapes/fonts) it is copied from ImFontAtlas's TexID.

@kevyuu
Copy link
Author

kevyuu commented Dec 12, 2023

I found this line on imgui_draw.cpp(618, 58)
_CmdHeader.TextureId = (_TextureIdStack.Size == 0) ? (ImTextureID)NULL : _TextureIdStack.Data[_TextureIdStack.Size - 1];

So it does use NULL as a special value.

Another usage that prevent me to use my custom ImTextureID is conversion to intptr_t, for example
C:\Users\kevin\Dev\soul\dependencies\imgui\imgui.cpp(20316,41): error: cannot convert 'const soul::gpu::TextureID' (aka 'const ID<impl::Texture, TexturePool::ID, TexturePool::NULLVAL>') to 'intptr_t' (aka 'long long') without a conversion operator
pcmd->ElemCount / 3, (void*)(intptr_t)pcmd->TextureId,

seems like currently I can only #define ImTextureID to integral or pointer type.

@PathogenDavid
Copy link
Contributor

So it does use NULL as a special value.

I think it's more that in the mostly-broken state of having an empty texture ID stack that the texture ID has to be something.

Texture IDs are only meaningful to the renderer backend. Fairly certain none of the official backends actually support a draw command with a null texture ID. (The D3D12 backend certainly doesn't.)

seems like currently I can only #define ImTextureID to integral or pointer type.

While this does seem like an unintended limitation, it seems easily solvable using conversion operators (perhaps with an intermediary type if you don't want to pollute your ID<...> type for the sake of Dear ImGui.)

@kevyuu
Copy link
Author

kevyuu commented Dec 12, 2023

I have check the source code. For the intptr_t, It is only used for printing. So having to be able to define custom to_string function would be better. Or just remove the texture from printing. It is only used on DebugNodeDrawList

I change it so that it doesn't print the ImTextureID

        ImFormatString(buf, IM_ARRAYSIZE(buf), "DrawCmd:%5d tris, ~~Tex 0x%p,~~ ClipRect (%4.0f,%4.0f)-(%4.0f,%4.0f)",
            pcmd->ElemCount / 3, ~~(void*)(intptr_t)pcmd->TextureId,~~
            pcmd->ClipRect.x, pcmd->ClipRect.y, pcmd->ClipRect.z, pcmd->ClipRect.w);

My type is not really a pointer(It is actually bigger than 64 bit), so casting to intptr_t without knowing how it will be used by imgui in the future seems to be dangerous. Same with null, being able to define custom null value is preferrable. Or just call the default constructor instead of using null when state being broken. Currently I will just modify, imgui source code temporarily.

@ocornut
Copy link
Owner

ocornut commented Dec 12, 2023

We only print for debugging, we don't understand what the value is so will not have other reason to read it other than for comparing for equality.

What is your actual problem? What are you trying to solve?
You shouldn't need to care about any of this and you shouldn't have to modify the source code if you use a custom type.

@ocornut
Copy link
Owner

ocornut commented Dec 12, 2023

Another usage that prevent me to use my custom ImTextureID is conversion to intptr_t, for example
C:\Users\kevin\Dev\soul\dependencies\imgui\imgui.cpp(20316,41): error: cannot convert 'const soul::gpu::TextureID' (aka 'const ID<impl::Texture, TexturePool::ID, TexturePool::NULLVAL>') to 'intptr_t' (aka 'long long') without a conversion operator
pcmd->ElemCount / 3, (void*)(intptr_t)pcmd->TextureId,

I will rework this using a better cast format for now so this doesn't fail (without having to add a cast operator on your side).

ocornut added a commit that referenced this issue Dec 12, 2023
…ompile-error when ImTextureID is defined to be larger than 64-bits. (#7090)
@ocornut
Copy link
Owner

ocornut commented Dec 12, 2023

Pushed 07dbd46 + f6836ff let me know if that works better for you.

@kevyuu
Copy link
Author

kevyuu commented Dec 13, 2023

Thanks omar. Is it possible to use nullptr as well instead of NULL. so I can provide a conversion constructor using nullptr_t that is more type safe than using NULL which is an integer.
To be concrete my texture id type is actually a Variant<gpu::TextureNodeID, gpu::TextureID>, I have a render graph system. TextureNodeID is for transient resource that only live for one frame. gpu::TextureID is for persistent resource.
I have check the solution, casting abritrary type to int seems like UB? I am not sure whether UB correctness is important for the project. It will probably compile just fine.

@ocornut
Copy link
Owner

ocornut commented Dec 13, 2023

Thanks omar. Is it possible to use nullptr as well instead of NULL. so I can provide a conversion constructor using nullptr_t that is more type safe than using NULL which is an integer.

It is not possible because ImTextureID may be integer or anything.

Please note we currently copy TexID around bypassing C++ constructor/destructor
You may be better off storing an integer in ImTextureID and using that to index an array of Variant<gpu::TextureNodeID, gpu::TextureID>.

@kevyuu
Copy link
Author

kevyuu commented Dec 13, 2023

Okay, thanks for your suggestion.

@ocornut
Copy link
Owner

ocornut commented Dec 18, 2023

Closing as answered (I think). Let me know if you have further issues.

I once attempted a patch to honor more of C++ shenanigans (constructor/destructors etc) for ImTextureId but it went untested and the team who asked for it didn't confirm it worked well. I'm not sure it works for every use case, I think it may not be ok if the object cannot be relocated. If it only requires constructor/destructor to alter a remotely stored reference-counter it may be ok.

imgui-12beb82-store unique ImTextureId honoring constructors destructors (untested).patch

But generally would advise using aforementioned suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants