Remove all direct CyclopsAutoload usage from plugin code

[jkulawik]

There's a long story here, but the short version is that I misspelled the autoload name (CyclopsAutoLoad instead of CyclopsAutoload, perhaps worth pointing out in the upgrade instruction) and spent a good 1.5h debugging the plugin and noticed something.

As far as I can tell, the plugin already mostly follows this undocumented guideline:

The correct way to handles [using autoloads in the plugin source], as the plugin developer, is to not rely on the name. Neither should the user be required to add the autoload, the plugin can do that. What to do: In the EditorPlugin's _enter_tree(), you register the autoload under a name. This name is purely for the user, not for the plugin. In the plugins various scripts, you add this line: var plugin_autoload: Node = $"/root/plugin_autoload", and use this variable instead of the autoloads name.

Looking through the source code, that approach is already used, but I also found many references to CyclopsAutoload as if it was an existing singleton:

snapping/snapping_system_grid.gd:   snap_to_grid_util = CyclopsAutoload.calc_snap_to_grid_util()
snapping/snapping_system_grid.gd:   var snap_angle:float = CyclopsAutoload.settings.get_property(CyclopsGlobalScene.SNAPPING_GRID_ANGLE)
snapping/snapping_system_grid_properties_editor.gd: CyclopsAutoload.settings.set_property(CyclopsGlobalScene.SNAPPING_GRID_POWER_OF_TWO_SCALE, int(value))
snapping/snapping_system_grid_properties_editor.gd: CyclopsAutoload.save_settings()
snapping/snapping_system_grid_properties_editor.gd: CyclopsAutoload.settings.set_property(CyclopsGlobalScene.SNAPPING_GRID_UNIT_SIZE, value)
snapping/snapping_system_grid_properties_editor.gd: CyclopsAutoload.save_settings()
snapping/snapping_system_grid_properties_editor.gd: CyclopsAutoload.settings.set_property(CyclopsGlobalScene.SNAPPING_GRID_USE_SUBDIVISIONS, toggled_on)
snapping/snapping_system_grid_properties_editor.gd: CyclopsAutoload.save_settings()
snapping/snapping_system_grid_properties_editor.gd: CyclopsAutoload.settings.set_property(CyclopsGlobalScene.SNAPPING_GRID_SUBDIVISIONS, int(value))
snapping/snapping_system_grid_properties_editor.gd: CyclopsAutoload.save_settings()
snapping/snapping_system_grid_properties_editor.gd: CyclopsAutoload.save_settings()
snapping/snapping_system_grid_properties_editor.gd: CyclopsAutoload.settings.set_property(CyclopsGlobalScene.SNAPPING_GRID_UNIT_SIZE, unit_size)
snapping/snapping_system_grid_properties_editor.gd: CyclopsAutoload.settings.set_property(CyclopsGlobalScene.SNAPPING_GRID_SUBDIVISIONS, int(subdiv))
snapping/snapping_system_grid_properties_editor.gd: CyclopsAutoload.save_settings()
snapping/snapping_system_grid_properties_editor.gd: CyclopsAutoload.settings.set_property(CyclopsGlobalScene.SNAPPING_GRID_TRANSFORM, xform)
snapping/snapping_system_grid_properties_editor.gd: CyclopsAutoload.save_settings()
menu/editor_toolbar.gd: CyclopsAutoload.settings.set_property(CyclopsGlobalScene.SNAPPING_ENABLED, toggled_on)
commands/cmd_snap_to_grid.gd:   var snap_to_grid_util:SnapToGridUtil = CyclopsAutoload.calc_snap_to_grid_util()
commands/cmd_intersect_block.gd:    var snap_to_grid_util:SnapToGridUtil = CyclopsAutoload.calc_snap_to_grid_util()
commands/cmd_subtract_block.gd: var snap_to_grid_util:SnapToGridUtil = CyclopsAutoload.calc_snap_to_grid_util()
actions/cyclops_action.gd:  var snap_to_grid_util:SnapToGridUtil = CyclopsAutoload.calc_snap_to_grid_util()
actions/action_snap_to_grid.gd: #var snap_to_grid_util:SnapToGridUtil = CyclopsAutoload.calc_snap_to_grid_util()
cyclops_level_builder.gd:const AUTOLOAD_NAME = "CyclopsAutoload"
cyclops_level_builder.gd:   mgr.snap_enabled = CyclopsAutoload.settings.get_property(CyclopsGlobalScene.SNAPPING_ENABLED)

While I can't guarantee it, it seems to me that changing all of this to use get_node("/root/CyclopsAutoload") should remove the need for the workaround of manually adding the autoload before enabling the plugin, leading to a much smoother and issue-free user experience. Is there a reason why you would need to use the "direct" approach over the get_node approach in some places?

[jkulawik]

Well, after trying to make a pull request for this, it seems that the issue is that the autoload is called in classes which don't inherit from node, e.g. Resource and RefCounted. These can't use get_node nor get_tree().

To remove the autoload call from them, a reference to it would have to be injected in _init or right after. These classes also seem to have a reference to the main plugin script/class, so the autoload ref could be acquired from there instead.

Alternatively to make the code more robust, don't touch the autoload in those at all and instead get data from these classes and save the autoload settings externally (i.e. wherever they're instanced).

[jkulawik]

I've worked on this a bit more and here's what I found.

The remaining (i.e. in code which doesn't inherit from Node) direct CyclopsAutoload usage consists of 6 calls to calc_snap_to_grid_util() and one CyclopsAutoload.settings.get_property(CyclopsGlobalScene.SNAPPING_GRID_ANGLE). calc_snap_to_grid_util() heavily references the settings as well.

Settings seems to reimplement features which already exist in Godot, such as:

• var_to_str and str_to_var which handle all built-in type conversion
• Dictionary.get() which checks whether a key exists and returns a supplied default value if needed

In general, I have a gut feeling this whole thing could be replaced with built-in ConfigFile. But I can't be sure because the codebase is really complex and it's hard for me to know how everything works. Anyway, the point being: rewriting the settings would enable getting rid of the direct CyclopsAutoload usage.

Here's a few proposals for that, and other adjacent things mentioned above:

• Rename Settings to CyclopsSettings (like with https://github.com/blackears/cyclopsLevelBuilder/issues/122, to not take up users' namespace)
• Implement the setting utils statically or in a way which won't rely on the settings object state. In other words, for each get/set, load the file or the wanted setting instead of loading once and referencing the data later. Using static functions has the benefit of being able to call without instancing the object: CyclopsSettings.get_property().
• This might be bikeshedding, but I'd move calc_snap_to_grid_util to SnapToGridUtil, perhaps even to its _init. This would relegate this responsibility where it belongs and remove the need to reference the settings each time that SnapToGridUtil is needed.

I might work on these myself, but the test project seems broken (I get tons of errors in most scenes and on project start) so I can't really test if these changes would influence other parts of the code, so I'm afraid my changes might cause some bugs.

[blackears]

Which test project are you talking about? The files named test_* in the root of the project?

The main purpose of the autoload is to let various tools draw to the screen, which is currently implemented by having a global scene draw blocks, wireframes, points, etc between input events. I'm hoping that moving to a separate viewport will eliminate the need for this class, but it's going to be a while before I can do that.

[jkulawik]

I mean the entire project from this repo. When I launched it the first few times, I got many errors (mostly related to GUT if I remember correctly) and cyclops blocks didn't get drawn in the editor viewport, instead spamming the log with thousands of errors. I can't seem to replicate this now unfortunately.

However, I still do get errors, despite the autoload and plugin being enabled. When opening a scene with cyclops blocks, I get one or a few of these: Can't take value from empty array.

But that's almost off-topic. How can I test the settings save/load?

[blackears]

It's hard for me to debug an error that I can't replicate on my machine. I'm not sure why. I also don't have the time to look into this just now. I hope to get to it soon.

I don't know what's causing the Can't take value from empty array. error. Godot is giving me no additional info.

The settings cache is meant to hold changes you've made to tools property window. Also potentially other things as the project evolves. You should see things written there if you make changes to the tool properties. You might need to switch between several tools to make sure the changes flush to the cache.

[blackears]

I've looked into this and implemented some of your suggestions. I think some of this would likely best wait on moving Cyclops to its own window, which should get rid of the CyclopsAutoload as part of the move. I also do need to come up with some sort of settings system - I'm not been sure how to design it since it should likely be a singleton but not outside of the Autoload system so that non-nodes can refer to it too.

[Calinou]

Out of curiosity, why not use add_autoload_singleton() in the EditorPlugin's _enter_tree() method to automatically register the autoload (in a way that persists to project.godot)? The user can then reload the editor.

[blackears]

I am in res://addons/cyclops_level_builder/cyclops_level_builder.gd on line 108. However, it doesn't seem to work for some reason. I'd look into fixing it, but I'm hoping to remove the need for this autoload all together by creating a new main window for editing.