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

Confusing child window with ### in name with top level window #713

Open
nem0 opened this issue Jun 25, 2016 · 12 comments
Open

Confusing child window with ### in name with top level window #713

nem0 opened this issue Jun 25, 2016 · 12 comments
Labels
label/id and id stack implicit identifiers, pushid(), id stack

Comments

@nem0
Copy link
Contributor

nem0 commented Jun 25, 2016

I have a top level window named DockPanel###DockPanel, its child Shader Editor abc###Shader editor and other top level window Shader Editor abc###Shader editor. The way ImHash works causes imgui to confuse the child ShaderEditor with the top level Shader Editor

@ocornut ocornut changed the title Confusing child window with ## in name with top level window Confusing child window with ### in name with top level window Jun 25, 2016
@ocornut
Copy link
Owner

ocornut commented Jun 25, 2016

I am not sure what solution you would suggest here?

Problem stem from the fact that BeginChild construct a string to pass to Begin which will hash it.
If BeginChild applied both hashes in succession and provided an explicit ID to say, an hypothetical BeginEx() which would name both name and id, we'd have a fix. However I am not sure it is worth bothering so much and it would introduce some confusion and complication.

Since your ### string is an explicit string because you could including your own ID within it? It would be a trivial workaround.

@nem0
Copy link
Contributor Author

nem0 commented Jun 25, 2016

There is no easy fix on imgui's side I can think of. I agree that this is probably not worth fixing. I am going to fix it on my side by using different string ids. It just cost me some time to find what's wrong and I wanted to save time for other people who encounter this bug. Maybe this problem can be mentioned in the documentation. Feel free to close this.

@nem0
Copy link
Contributor Author

nem0 commented Jun 25, 2016

Just an idea, maybe we can put assert in ImHash when there is ## twice in the ID

@ocornut
Copy link
Owner

ocornut commented Jun 25, 2016

Note that you can also use BeginChild(ImU32 id) with our own id which would also ease fixing the problem on your side.

Just an idea, maybe we can put assert in ImHash when there is ## twice in the ID

We could add a strcmp() assert in the success path of FindWindowByName().

@nem0
Copy link
Contributor Author

nem0 commented Jun 25, 2016

We could add a strcmp() assert in the success path of FindWindowByName().

That would be even better.

@ocornut
Copy link
Owner

ocornut commented Jun 25, 2016

We could add a strcmp() assert in the success path of FindWindowByName().

Ah no, that's actually stupid :( it would defeat the whole purpose of ### when changing the title of a window.

Just an idea, maybe we can put assert in ImHash when there is ## twice in the ID

That's also incorrect unfortunately. Here the problem isn't that you have two `###' but merely that the ID after the final ### of both window names are matching.

An unrelated assert that may have helped but completely by coincidence and not covering all cases, would be to check that e.g. the ImGuiWindowFlags_Child doesn't change.

Note that you can also call BeginChild with an ID in your use case.

Any other suggestion?

@nem0
Copy link
Contributor Author

nem0 commented Jun 25, 2016

Note that you can also call BeginChild with an ID in your use case.

I am simply generating different string for the child nem0/LumixEngine@3b9c08f

Is it useful in any way to have ### in children? Maybe we can assert when there is ### in the child.

@nem0
Copy link
Contributor Author

nem0 commented Jun 25, 2016

Or
ImFormatString(title, IM_ARRAYSIZE(title), "%s.%s", window->Name, str_id); in ImGui::BeginChild can take only part of str_id which is after ###

@ocornut
Copy link
Owner

ocornut commented Jun 25, 2016

Is it useful in any way to have ### in children? Maybe we can assert when there is ### in the child.

Never required, since child name are never printed out to the user you can always get a unique id with the base name without ###, but I don't see why it should be disallowed.

What sort of end-result did you get with the bug? Perhaps we could catch the error in a different spot. For example, for multiple begins in a same frame we could check that the parent is the same?

ImGui::BeginChild can take only part of str_id which is after ###

I don't understand, that would be no-op and equivalent to leaving things to way they are.
Basically anything before the last ### will be ignored for the ID computation.

@nem0
Copy link
Contributor Author

nem0 commented Jun 25, 2016

but I don't see why it should be disallowed

This bug would be avoided, since something like parent.xyz###child could not happen

What sort of end-result did you get with the bug?

It triggers IM_ASSERT(g.MovedWindow->RootWindow->MoveID == g.MovedWindowMoveId); in NewFrame()

ImGui::BeginChild can take only part of str_id which is after ###

Example: I have parent ImGui::Begin("DockPanel"), it has a child ImGui::BeginChild("def###ShaderEditor"), the full str_id of the child is "DockPanel.def###ShaderEditor" but hash is computed only from "ShaderEditor" part. If the full str_id is instead "DockPanel.ShaderEditor", the hash is computed from "DockPanel.ShaderEditor", which does not collide with the top level "ShaderEditor"

@ocornut
Copy link
Owner

ocornut commented Nov 6, 2016

(I tagged this issue with commit only because the topics are similar but the issue discussed here is still valid.)

ocornut added a commit that referenced this issue Sep 26, 2018
…y not applying the ID stack to the provided string to uniquely identify the child window. This was undoing an intentional change introduced in 1.50 and broken in 1.60. (#1698, #894, #713) + reworked the Begin/BeginChild comments in imgui.h.
@ocornut ocornut added the label/id and id stack implicit identifiers, pushid(), id stack label Oct 20, 2020
@ocornut
Copy link
Owner

ocornut commented Aug 22, 2022

I've been looking at this problem today as part of another set of of issues.

Recap: ### is not meaningful in BeginChild() context.
but ideally should not result in that issue.

Or ImFormatString(title, IM_ARRAYSIZE(title), "%s.%s", window->Name, str_id); in ImGui::BeginChild can take only part of str_id which is after ###

I'm tending toward this, aka more generally changing current law from:

GetID("Hello###World)" == ImHashData("###World")
to
GetID("Hello###World") == ImHashData("World")

Aka not including the ### in the hash.
This has the benefit that it gives more flexibility in reconstructing strings.

Begin("Window1")
BeginChild("Child1###Hello")
--> child name "Window1/Child1###Hello" (omitting the fact we also include a 0x08X hash, but doesn't matter here)
Problem: We can't really transform it to "Window1/###Hello", still get same problem.
If we manually parse, we lose property that ImHash(window->Name) == window->ID, seems like a recipe for trouble.
If instead we remove the ### from the hash data, we can simply transform "Window1/Child1###Hello" to "Window1/Hello".

Nothing new here, those ideas transpired in your post above, I'm mostly recapping for myself :)

This seem decent but it also means we lose that "annotation" (the "Child1" part) from Debug Tools which may be undesirable, as I've seen people use "LocalizedName###SomeGUID".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
label/id and id stack implicit identifiers, pushid(), id stack
Projects
None yet
Development

No branches or pull requests

2 participants