DarkRadiant crashes when trying to change the name of a newly created skin in the skin editor

[datiswous]

If I create a new skin in the skin editor it creates a new skin with name new_skin . When I try to edit the name DR immediately crashes.

Maybe I'm doing something wrong? But it should not crash at least.

Using DR 3.7.0 under Linux

[datiswous]

No responses. I created a bug report: https://bugs.thedarkmod.com/view.php?id=6239

[datiswous]

And another bug report for 3.9.0

https://bugs.thedarkmod.com/view.php?id=6514

[MirceaKitsune]

Sorry for the double report, ran into this myself and quickly posted about it so the issue wasn't missed. Currently the skin editor is unusable: Editing the skin name field instantly crashes DR, so does exiting the menu after making any changes, and any changes you do manage to make are immediately reverted and don't stick. The skin editor is a very useful tool and I hope it can be fixed.

[datiswous]

I guess the devs are clueless about this bug, otherwise one would think they would have tried to fix it somewhere during this year.

Edited April 5 by datiswous

[jonri]

I think the devs just haven't had much time to put into it the past year (myself included).  I took care of the mouse issue finally, since it became a 100% showstopper for me to use DarkRadiant at all on Linux.  I've only ever made my skins using a text editor, but I tried reproducing the issue and it crashes with a backtrace for me too.  This means debugging it should be straightforward, I'll look at it this weekend.

 

[MirceaKitsune]

I also take it the skin editor isn't among the most used parts of DarkRadiant, making this less easy to spot. Hopefully now the team will notice the issue, and it's ideally nothing hard to fix since DR has a history of GUI issues like this that stabilized.

[OrbWeaver]

Reproduced with 3.9.0 on Ubuntu. This is the backtrace:
 

ASSERT INFO:
../src/unix/threadpsx.cpp(1678): assert "This() == this" failed in TestDestroy(): wxThread::TestDestroy() can only be called in the context of the same thread

BACKTRACE:
[1] wxThread::TestDestroy()
[2] wxutil::ThreadedResourceTreePopulator::ThrowIfCancellationRequested()
[3] decl::DeclarationManager::renameDeclaration(decl::Type, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)
[4] skins::Doom3SkinCache::renameSkin(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)
[5] wxEvtHandler::ProcessEventIfMatchesId(wxEventTableEntryBase const&, wxEvtHandler*, wxEvent&)
[6] wxEvtHandler::SearchDynamicEventTable(wxEvent&)
[7] wxEvtHandler::TryHereOnly(wxEvent&)
[8] wxEvtHandler::ProcessEventLocally(wxEvent&)
[9] wxEvtHandler::ProcessEvent(wxEvent&)
[10] wxEvtHandler::SafelyProcessEvent(wxEvent&)
[11] wxTextEntryBase::SendTextUpdatedEvent(wxWindow*)
[12..34] <internal GTK stuff> 
[35] wxGUIEventLoop::DoRun()
[36] wxEventLoopBase::Run()
[37] wxDialog::ShowModal()
[38] wxutil::DialogBase::ShowModal()
[39] cmd::CommandSystem::executeCommand(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<cmd::Argument, std::allocator<cmd::Argument> > const&)
[40] cmd::CommandSystem::execute(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)
[41] wxEvtHandler::ProcessEventIfMatchesId(wxEventTableEntryBase const&, wxEvtHandler*, wxEvent&)
[...] <Other GTK/wxWidgets stuff>

 

[jonri]

Looking at the backtrace and the code, I think the culprit is here: https://github.com/codereader/DarkRadiant/blob/fa4c266a6776516b59740f78f57b849097d97618/radiant/ui/skin/SkinEditorTreeView.cpp#L44

The ThreadedDeclarationTreePopulator is created on the stack and thus deallocated at the end of the function.  The EnsureStopped() function is called on deletion (https://github.com/codereader/DarkRadiant/blob/master/libs/wxutil/dataview/ThreadedResourceTreePopulator.cpp#L108) but this doesn't appear to actually wait for the deletion to finish.  By the time the deletion request is processed, the object itself is no longer valid.  Other usages I see put the object in a shared pointer instead of on the stack.

I'll have a chance to look at this properly on Sunday and figure out the best way to fix it.

 

[datiswous]

Why does it not crash on Windows though? Or is that code different there?

[jonri]

14 minutes ago, datiswous said:

Why does it not crash on Windows though? Or is that code different there?

I'm honestly not sure, it's a race condition maybe?  I found and fixed 2 different segfaults (PR is here), one relating to threading and the other one relating to a variable being referenced after it was cleared out.  This stops the skin editor from crashing, but it seems there are still a lot of usability issues.  I'm not sure if they are all Linux only, but here's what I found:

1. The rename operation happens every time you type a character, which is a little laggy.  It would be better to rename it when you are done typing and the field loses focus
2. Typing new characters while renaming works as expected, but deleting characters causes the box to highlight so you have to reset the cursor before deleting another one.
3. On the Material Replacements tab, click to edit is not working for me.  I spent a lot of time on this, I can get a box to show up if I replace the editbox+button with a simple edit box.  I think it's related to the compound control not being able to have focus.
4. The skin name is showing up on top of the Close button

This will require more wxWidget-fu than I have time to do right now.

 

[MirceaKitsune]

2 hours ago, datiswous said:

Why does it not crash on Windows though? Or is that code different there?

It could also be that on Linux we have to compile DarkRadiant ourselves, which means it's being compiled against newer versions of packages (GTK, WxWidgets, etc) and a slightly different configuration overall.

[datiswous]

Well I was using the Flatpack version which is always in line with a release version I think.

[jonri]

2 hours ago, MirceaKitsune said:

It could also be that on Linux we have to compile DarkRadiant ourselves, which means it's being compiled against newer versions of packages (GTK, WxWidgets, etc) and a slightly different configuration overall.

Is not quite that, but more likely because wx uses native Windows controls there and gtk only on Linux. It does a pretty good job of abstracting away the differences but there are always corner cases where a subtle difference turns out to be important.

[OrbWeaver]

On 4/7/2024 at 9:27 PM, jonri said:

1. The rename operation happens every time you type a character, which is a little laggy.  It would be better to rename it when you are done typing and the field loses focus
2. Typing new characters while renaming works as expected, but deleting characters causes the box to highlight so you have to reset the cursor before deleting another one.
3. On the Material Replacements tab, click to edit is not working for me.  I spent a lot of time on this, I can get a box to show up if I replace the editbox+button with a simple edit box.  I think it's related to the compound control not being able to have focus.
4. The skin name is showing up on top of the Close button

I can reproduce all of those.

For (1), I propose that the Skin Name field should be non-editable, and have a separate button (usually the "pencil" icon) to show a popup entry dialog for editing the name. I doubt renaming skins is all that common, and it certainly doesn't need to happen on every keypress. But if people object to additional popups, the field could be editable but only commit the changes on ENTER or if a "tick" button was clicked.

For (3), single-click to edit in a list is rather non-standard (double-click might be more expected). I propose to have two named entry fields below the list for "From" and "To", rather like the key/value fields in the Entity Inspector, with the fields reflecting the currently-selected list item and allowing changes (committed on ENTER or button click).

Not specific to the skin editor, but I think we need an application preference for monospace font size. Reading the declarations is really difficult on my 1440p monitor.

[datiswous]

What is the current status? I see @OrbWeaverproposed something. Is something already implemented?