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

Fix applying docking settings #3215

Open
wants to merge 2 commits into
base: docking
Choose a base branch
from

Conversation

kudaba
Copy link
Contributor

@kudaba kudaba commented May 9, 2020

As per my comments in #2573 removing the call to DockContextBuildAddWindowsToNodes when applying settings. From what I can tell it was pushing the previous frame's window state into the dock system which was redundant anyway.

@ocornut
Copy link
Owner

ocornut commented May 9, 2020

This tends to be needed to be able to reload ini data without flickering. Please submit a fuller explanation/repro about what you are trying to fix precisely, thank you.

@kudaba
Copy link
Contributor Author

kudaba commented May 9, 2020

Hrm, I see the flicker you're talking about after limiting to 30 fps. I'll dig deeper to see if I can find a different solution, without this fix I was hitting crashes instead as I mentioned in the referenced issue.

@kudaba
Copy link
Contributor Author

kudaba commented May 10, 2020

So, testing a bit more, if I do a full reset of imgui (clear context, reset renderers) I actually get no flicker.

@kudaba
Copy link
Contributor Author

kudaba commented May 10, 2020

Nevermind, after fixing a local flicker with my 3d scene, both ways seem to flicker about the same, It's an inconsistent 1 frame blank when changing layouts. I'd say it might be threading, but all of this is single threaded.

@ocornut
Copy link
Owner

ocornut commented May 11, 2020

About your researches, does this PR still fixes the flicker you're getting?
That function call is supposed to FIX a flicker by building the nodes ahead, so it seems to be having the opposite effect here.

Building a minimal test may be nice (in the form of 2 .ini files relying on demo windows) but I think I can probably repro this (window call order will probaby matter too)

@ocornut ocornut added docking settings .ini persistance labels May 11, 2020
@kudaba
Copy link
Contributor Author

kudaba commented May 12, 2020

After further testing including the function does seem to remove any flicker if I'm only reordering the same set of windows, when I switch to a completely new set of windows I'll get a blank for a frame regardless of whether I have that function or not.

Also, the plot thickens. If I transition from my initial layout to a more complex one I crash, but I found that I can transition to a different simple layout and then the complex one and it won't crash. It's very bizarre.

I attached the layouts and screen shots of the metrics windows for each state. I have no idea what the difference is between 1 and 2 where doing 1->3 crashes but 1->2->3 works fine. It's even reproducible, once I go 1->2->3 I'll still crash if I go 3->1->3.

LayoutBug.zip

@kudaba
Copy link
Contributor Author

kudaba commented May 13, 2020

Well it think i sort of found the root of the issue.

When I save my layout I simply ask imgui for whatever it's internal state is. Imgui will hold onto window settings basically forever, so when I saved my complex layout it actually stored the last known dock information about a window I wasn't actively using. So when I start my app in the initial layout (state 1) it is now actively using that window, when I switch to layout 3, it thinks that window should still be docked into the root dock-node shared by everyone since it was active last frame, but by doing that it's creates an inconsistent state with the tab bar and it's adding a node that will no longer have a host window.

The reason it worked when switching to layout 2 was that the layouts were similar so it didn't create and invalid situation in the docking system.

The two options I see at the moment are

  1. Fix the flicker without using that function (partially fixed by setting the node->VisibleWindow when setting ini settings and moving the grey empty background to end of frame, needs more work)
  2. Don't save dock settings from inactive windows. Flicker would remain but it would ensure I wouldn't get into an invalid state. This might be difficult though if we can't distinguish between an real in-active window and one that's simply in a hidden tab.

I'll see if I can put more time into option 1 and update he PR, since I'd prefer that, but it might take a while.

@kudaba kudaba force-pushed the docking_settings_fix branch from 5356543 to 472f0be Compare May 13, 2020 18:50
@kudaba
Copy link
Contributor Author

kudaba commented May 13, 2020

I managed to narrow down the original problem to simply adding a window to a split node which is invalid. I also disabled saving out the Selected property when a node is split as well to keep things a bit clean.

As a separate commit in this PR I deferred the drawing of the grey background when a node is empty, since it was checking for that before any windows have had a chance to register with the node. This reduces flicker a little more since you won't get grey for a frame, it will still be blank but at least it will be the normal window background color.

@ocornut
Copy link
Owner

ocornut commented May 13, 2020

Chris, as suggested in my messages above,
I am not going to be able to merge anything without repro of specific broken cases that the PR aim to fix. Please make repro so I can begin inspect and judge if the PR is right.

if (window->DockNode && window->DockNode->IsEmpty() && window->DockNode->IsVisible && !(window->DockNode->GetMergedFlags() & ImGuiDockNodeFlags_PassthruCentralNode))
window->DrawList->AddRectFilled(window->DockNode->Pos, window->DockNode->Pos + window->DockNode->Size, GetColorU32(ImGuiCol_DockingEmptyBg));
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why iterating on windows and not visible nodes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I couldn't find the list of nodes before, I'll fix that up and also make it a proper array not using auto.

@kudaba kudaba force-pushed the docking_settings_fix branch from 472f0be to 7f0d65c Compare May 13, 2020 23:15
@kudaba
Copy link
Contributor Author

kudaba commented May 14, 2020

Minimal example is available in the directx11 sample in https://github.com/kudaba/imgui/tree/docking_crash_example
Simply select layout 1 then layout 2 to cause the crash. The sample can sort of be used to build custom layouts, but it's a bit tailored to this crash. The layout build order plays a role in whether you encounter the crash.

Starting from scratch:
1 Open settings window, close everything else, dock settings window into center, save as layout 1
2.A Add canvas to root (as second tab) close settings, add properties and other window to left and right, save layout. This will cause the crash as described.
2.B Add properties and other window to left and right, then add canvas to the center node and close settings. Save layout. This layout actually doesn't crash because of subtle differences to the dock node layout.

(saving layout means selecting Layouts->Copy and pasting results in the global variables)

@kudaba kudaba force-pushed the docking_settings_fix branch from 7f0d65c to 45363c2 Compare May 14, 2020 00:18
@kudaba kudaba force-pushed the docking_settings_fix branch from 45363c2 to 50acf76 Compare May 14, 2020 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docking settings .ini persistance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants