2 Jan 2024 |
k1_801 | I would consider it both | 20:21:10 |
k1_801 | If we allow the plugin to add any objects, we need to know that we remember which plugin they belong to | 20:21:56 |
k1_801 | And if the plugin is removed, those objects need to be removed from our managers | 20:22:19 |
chr1sno | Sounds like a job for the plugin's destructors | 20:23:09 |
k1_801 | I.e. if the plugin can call, say, "RegisterRgbController", we need to know that this controller doesn't come from the core and that it comes from this specific plugin, and if the plugin fails to clear those up, we should do that still | 20:23:33 |
morg7672 | that's the plugin's responsibility to delete all new objects | 20:23:36 |
morg7672 | we already do it | 20:23:58 |
morg7672 | plugins are notified when they have to free stuff | 20:24:15 |
k1_801 | Okay then | 20:24:34 |
morg7672 | void OpenRGBPluginInterface::Unload() | 20:25:19 |
k1_801 | Are we 100% okay with possible crashes if a plugin forgot to remove an object in time? | 20:25:22 |
morg7672 | that's the plugin responsibilit | 20:25:36 |
morg7672 | * that's the plugin responsibility | 20:25:38 |
morg7672 | if it crashes because of a plugin, blame (and fix ) the plugin | 20:27:01 |
chr1sno | Then why did the code get reverted? | 20:28:03 |
sir_qwex | So on windows by default you could not remove a loaded plugin so this is the fix for that | 20:28:48 |
morg7672 | it crashes on load, not on unload | 20:28:50 |
chr1sno | Ok, sure but not for all plugins | 20:29:22 |
morg7672 | whatever, the fix tries to fix unload, but breaks load | 20:31:01 |
morg7672 | i'm ok with fixing unload, that's what we're after | 20:31:31 |
morg7672 | but you cannot accept a merge request that breaks load | 20:31:47 |
chr1sno | Ok, am right there with you but what was the root cause? You've identified the timing and the change causing the issue. What's different about the plugins causing the crash compared to the ones that don't segfault?? | 20:34:54 |
morg7672 | maybe, we need this fix + some plugins rework, maybe not | 20:38:44 |
morg7672 | but, that's not addressing all OS; only windows, the issue is probably more complex than just adding a flag | 20:39:20 |
chr1sno | That issue was just resolved by a commit added to the mainline codebase a few hours ago. Please download the pipeline build of OpenRGB to use it. | 20:41:36 |
chr1sno | https://tenor.com/view/the-rock-youre-welcome-moana-gif-10446310 | 20:43:59 |
chr1sno | diogotr7, apologies for the ping. Was wondering if you still have a Wooting V1 KB that you could use to test? | 20:49:10 |
diogotr7 | A wooting one, yes I do | 20:49:38 |
chr1sno | Ah, awesome. Would you mind helping with some regression testing? | 20:50:04 |
chr1sno | Am moving the Wooting controller to the KLM | 20:50:13 |