Registry settings cannot be read or get deleted

[jonathanlinat]

System Information

TrenchBroom V2.1.0-RC2 on Windows 10

1. Fresh installation. Opened TB and a custom personal map. All OK. 👍
2. Reset to defaults in each of the View panels, and applied new default settings.
3. Closed and opened TB. Entities are not shown.

4. Closed and opened TB. Entities are shown but... error 87

5. Closed and opened. All OK. 👍

[muk0r]

I received a similar error.

Id guess these errors are related to my loss of preferences I reported in Discord.

[kduske]

Ok. This Camera key error is triggered modifying the FOV value from the View panel.

[kduske]

Fresh RC2 installation. Opened up a new map. Opened View panel. Changed FOV value to 100. Applied. Closed window. Saved map. Closed TB. Opened TB and then error 87.

System language is en_us. Well possible that it's an issue with how floats are serialized (converted to strings and unconverted).

[jonathanlinat]

windows10_tbv210rc1_firstlaunch_enyb0la.zip
windows10_tbv210rc2_firstlaunch_enyb0la.zip

[kduske]

windows10_tbv210rc2_after_fov_crash_enyb0la.reg.zip
windows10_tbv210rc2_before_fov_crash_enyb0la.reg.zip

[jonathanlinat]

Error 6

[jonathanlinat]

Build gad2e770

Crashed too.

tb-v210rc2-gad2e770-error6.zip

[jonathanlinat]

Build gad2e770

Something really weird.

1. Fresh installation. No registry. Opened TB for the first time and closed it. This is the result:

2. Deleted previous registry. Fresh installation. Opened TB for the second time and closed it. This is the result:

The tree is different, for the same exact behavior.

[jonathanlinat]

Build g4a2f915

1. No previous registry. Fresh install. First launch.

2. Deleted previous registry. Fresh install. Second launch.

So, no more weird tree. I'll try to modify settings now.

3. Deleted previous registry. Fresh install. First launch. Opened Preferences. Applied a FOV of 110. Closed TB.

I'll keep this registry and try to modify the FOV value again.

4. Previous registry NOT deleted. Fresh install. Other launch. Opened Preferences.

FOV value in TrenchBroom preferences differs from the registered value in Registry.

5. Closed previous TB instance. Previous registry NOT deleted. Fresh install. Other launch. Opened Preferences.

FOV value in TrenchBroom preferences is now well retrieved from the Registry. 🤔

[jonathanlinat]

👍

So yeah, at this moment, no more error 6 reported. Last commit seems to have fixed the issue.

But, sometimes FOV value in Preferences differs from the registered one present into Registry.

[jonathanlinat]

Build g4a2f915

Two attempts. Actions between Reset to defaults, Ok, Apply, closing the app, opening it.

1. First attempt. error 0

2. Second attempt. Value from Registry not retrieved + error 0

[ericwa]

I'm convinced it's a wxWidgets bug, or at least the registry access is failing inside wxWidgets somehow. I made a branch where all config access is via absolute paths, and I still get this error on commit 40578eb (it's random but happens on around 40% of launches. Seems to happen only when a Release build exe is launched via double clicking the .exe in Windows Explorer).

This reproduces @jonathanlinat 's last post. I verified that we're calling the wxConfig API with "\TrenchBroom\Controls\Camera\Field of vision" and that key exists in the registry, but the error shows that there was an attempt to access "\TrenchBroom\Field of vision" for some reason.

wxRegConfig::DoReadString uses wxConfigPathChanger internally so maybe that fails which leads to accessing the wrong path?
https://github.com/wxWidgets/wxWidgets/blob/master/src/msw/regconf.cpp#L560

All I can think of now is stop using the wx registry code. I still have no idea what changed between RC1 and RC2 that caused this, and I've had no luck debugging it because i can only reproduce it in Release builds :-/

[kduske]

Maybe if we rename that key to "FOV" it will help? But I'd rather switch to a file based config at this point.

[kduske]

Oh and while we're destroying people's preferences, we should also switch to writing / reading typed values instead of serializing / deserializing with strings.

[ericwa]

I posted on discord but it turns out the fly mode thread is accessing preferences from a background thread which is probably breaking everything. Call stack looks like:

0116233D (TrenchBroom): (filename not available): TrenchBroom::Preference<float>::load
01240A1A (TrenchBroom): (filename not available): TrenchBroom::View::FlyModeHelper::moveDelta
012406DB (TrenchBroom): (filename not available): TrenchBroom::View::FlyModeHelper::Entry

Oh and while we're destroying people's preferences, we should also switch to writing / reading typed values instead of serializing / deserializing with strings.

Yes, if we switch to JSON this would make sense. If we stick with the INI format that wxFileConfig uses, it doesn't make a difference, since INI doesn't distinguish strings from numbers.

For 2.1.0 I suggest we stick with the registry, get rid of any preference accesses from background threads, stick ensure(wxThread::IsMain(), "called on non-main thread"); anywhere that accesses wxConfig.

[kduske]

Aha, good find! So we need to synchronize access from the fly mode thread, or rewrite fly mode as we discussed to processing events on the main thread. I vote for the latter.