-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OQ_Toolkit as plugin #40
base: master
Are you sure you want to change the base?
Conversation
- Added plugin.cfg - Added plugin.gd: This now automatically adds the vr auto-load singleton when the plugin is enabled or disabled - Moved OQ_Toolskit to addons as that is where plugins go
This is optional, so users can decide when and where it happens on their own if they want
@@ -4,7 +4,13 @@ extends EditorPlugin | |||
|
|||
func _enter_tree(): | |||
add_autoload_singleton("vr", "res://addons/OQ_Toolkit/vr_autoload.gd") | |||
|
|||
add_custom_type("OQ_ARVROrigin", "ARVROrigin", load("res://addons/OQ_Toolkit/OQ_ARVROrigin/scripts/OQ_ARVROrigin.gd"), load("res://addons/OQ_Toolkit/oqt_icon.png")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if making some classes as build in types improves the usability of the toolkit; my main concerns are:
- If not all classes are a custom type it is inconsistent and not clear to a user when to use what
- I would assume that in most cases a user will not need to add nodes of the OQ_Toolkit very often but just setup the basic VR requirements and is done (especially for the origin, camera and controller)
- The folder structure is actually part of the documentation as it makes clear which features should be attached to which node type; making everything a custom class would loose this (and I don't like to add a lot of tool-code to the scripts to check for this at editing time as this will get unreadable and hard to maintain quickly)
# Sets up everything as it is expected by the helper scripts in the vr singleton | ||
func _enter_tree(): | ||
if (!vr): | ||
vr.log_error("in OQ_ARVROrigin._enter_tree(): no vr singleton"); | ||
return; | ||
if (!vr.initialized and initialize_vr): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea to make it easier to use by auto-initializing and not requiring it user code; but as you have written this will make some settings and other required code (like the scene switch root) slightly more intransparent for users who will rely on this mechanism.
Also in my applications I have a OQ_ARVROrigin in every scene I switch to; but this should not be a problem I think with the guard.
Would it be possible to make the option part of the plugin settings (somehting like auto_initialize_vr)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
absolutely, and probably needs to be for flexibility.
Is there any interest in continuing down this road? We could separate out some of the changes. For instance, just having a plugin that adds vr_autoload for you would be beneficial. We can pick and choose whats in here:
1 and 1a make a lot of sense to me. 1c makes some sense if we have a plugin script. But if this is a sticking point, there are options. We could put the plugin.cfg and plugin.gd in /addons/OQT and have the rest at the root level, or even just put the plugin in with the root level OQT directory. Personally I'm rooting for moving the whole OQT directory into addons/ as that's where it lives for me, and would save me some time each time I need to upgrade. But I understand that might not be what others want. 1b and 2 I could take or leave. |
PR for #38