Immediate-Mode-UI / Immediate-Mode-UI/Nuklear
Hidden/closed window is to be ignored due to nk_clear
- Dominant language
- C
- Stars
- 11.4k
- Forks
- 686
- Avg merge
- 4d 1h
- Merged PRs (30d)
- 3
Description
There's a issue when you set a window hidden (either manually set its `NK_WINDOW_HIDDEN` flag or do `nk_window_show()` ), or you close the window (window with `NK_WINDOW_CLOSED` flag), they will only take effect for a period, only then to be reinitialized, which defeats the purpose of hiding,closing and them being flags in the first place.
So this line of code in `nk_clear` function (defined in nuklear_context.c, is called everytime you render a frame)
https://github.com/Immediate-Mode-UI/Nuklear/blob/a329721db93eb336f8eaae20f9c822730dd2ed6c/src/nuklear_context.c#L137-L147
(do cleanups for 'active' window that isn't really active)
specifically the line `ctx->begin = 0;` (reset the start pointer of a linked list of windows)
the problem here is it assumes that the hidden window isn't at the 'ctx->begin' (where the 'win->prev' doesnt exist), which isn't always the case, but it set to that anyway without checking
then it choose the window before the hidden one, which may not exist, so it trigger the code and make Nuklear completely forget the window list,
and what worse is that the forgotten window (and the ones after it) won't be freed from memory by the later section of the code since its `win->seq` (I guess this is like its heartbeat) is valid with the `ctx`'s
https://github.com/Immediate-Mode-UI/Nuklear/blob/a329721db93eb336f8eaae20f9c822730dd2ed6c/src/nuklear_context.c#L165-L171
since the reference is gone, the windows will live in the memory with the program, causing memory leak (thanks you @sleeptightAnsiC for letting me know)
and in the next frame, `nk_begin` (how you start to draw things) can't find the existing window you're talkiing about (since the list reference is gone) so it will create new one with your spec, that means your hidden window is now reseted and live somewhere else in the memory, and when it's hidden and its 'win->prev' gets invalid (due to some internal reorder for Z indexing or something), that cleanup code (for invalid active window) will be triggered, now you're stuck in a loop where you hide window just to see it popup again.
this issue doesn't happen if window is both HIDDEN and CLOSED
About its history/changelog
(hello)
---
As @sleeptightAnsiC said, it really is regression!
for example, the [web demo](https://dexp.github.io/nuklear-webdemo) of Nuklear made by DeXP ~9 years ago, the closing window feature works perfectly fine
but today it breaks?
so I went to digging, bc im very bored
So i found out that code block back then was just a single one-liner, also the version that DeXP's webdemo (i mentioned) used, first introduced in https://github.com/vurtun/nuklear/commit/36c60548a6f02d15f5891579c0654ab7a4ea8a72
```c
/* remove hotness from hidden or closed windows*/
if (((iter->flags & NK_WINDOW_HIDDEN) ||
(iter->flags & NK_WINDOW_CLOSED)) &&
iter == ctx->active)
ctx->active = iter->next;
...
```
and in the https://github.com/vurtun/nuklear/commit/43b41f92bd411c67c920b5f6526b1acd7a80c7e6 it have a longer version, tends to make it behave as expected i guess?
```c
if (((iter->flags & NK_WINDOW_HIDDEN) ||
(iter->flags & NK_WINDOW_CLOSED)) &&
iter == ctx->active)
ctx->active = iter->prev;
ctx->end = iter->prev;
if (ctx->active)
ctx->active->flags &= ~NK_WINDOW_ROM;
}
```
and https://github.com/vurtun/nuklear/commit/76107a2cd2cd8819c36725fcd1812ccfa8a07705 is the version we see it today, tends to fix some crashing
```c
if (((iter->flags & NK_WINDOW_HIDDEN) ||
(iter->flags & NK_WINDOW_CLOSED)) &&
iter == ctx->active)
ctx->active = iter->prev;
ctx->end = iter->prev;
if (!ctx->end)
ctx->begin = 0;
if (ctx->active)
ctx->active->flags &= ~NK_WINDOW_ROM;
}
```
though, these older versions is even more glitchy with modern Nuklear for some reasson (one of them even cause other windows reinitialized too)
i was just tryna to find out why are we modifying it, which i still dont know
so it's better just forget them lol
(im sorry if this causes you extra mental load)
---
Steps to produce
1. You can choose a demo of your preferred render engine provided by Nuklear
2. define `INCLUDE_ALL` macro in main.c & compile
3. Run the program and then close "NodeEdit" or "Canvas" window
4. Click on the window that is getting overlapped by others (which clicking will raise them)
And then you will see the window you just closed will reappear again
@sleeptightAnsiC provided a video in (https://github.com/Immediate-Mode-UI/Nuklear/issues/896#issuecomment-3897951017) and it sure will help you visualize it, thanks to him!
---
# Solution?
The code shouldn't modify the list (by `ctx->begin` or `ctx->end`), not at all, there's no clear benefit on doing it
I recommend @MrOneTwo's solution for patched version, the hidden/close code block handle should looks like this
```c
/* remove hotness from hidden or closed windows*/
if (((iter->flags & NK_WINDOW_HIDDEN) ||
(iter->flags & NK_WINDOW_CLOSED)) &&
iter == ctx->active) {
ctx->active = iter->prev;
if (ctx->active)
ctx->active->flags &= ~(unsigned)NK_WINDOW_ROM;
}
```
the code above solve the issue and also easy to understand the goal! Again, credit to him!
(this is my first time doing this, im sorry for any mistakes or English errors, thank!)
Contributor guide
Research direction
Start in src/nuklear_context.c at nk_clear and inspect the hidden/closed-window cleanup around the linked-list pointers. Reproduce with a preferred demo after defining INCLUDE_ALL, compiling, and closing NodeEdit or Canvas. Done means hidden or closed windows do not reappear, the window list remains reachable, and the reported memory leak path is avoided.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- c
- Domain
- desktop-dev
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Stale
- Clarity
- Clearly specified
- Newbie friendliness
- 45/100