-
Notifications
You must be signed in to change notification settings - Fork 37
Clean-up editor plugins and class registration #308
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
Conversation
c002d47 to
55038ca
Compare
cdca1da to
2885605
Compare
2cd1f82 to
c9a8287
Compare
|
Not sure if I'm misreading things, generally agree with the improvement but it seems the lynx export setup has gone missing. Or is this because there is nothing to override in the lynx version of the export settings (seeing we currently don't have settings?). In that case I would still suggest we have a class but without any further methods in it just to keep the structure in tact. It's likely that in the future we will need to add settings here as Lynx usage increases, it will be more clear for people who wish to contribute those changes to submit PRs if they can see where the changes are expected. |
|
Also, and I may be getting ahead of myself, seeing what we're planning with godotengine/godot#106891, should we rename the Khronos export to HTC export as it really only implements the HTC bits? Or just split that off into a separate HTC class (something I had a PR up for at some point in time I think), and then mark the Khronos export as deprecated. Might be too early to consider that though but throwing it out there :) |
Yeah, it never had its own
We could! But that's not really related to the rest of the clean up I'm doing here. |
0a13de4 to
c043ce7
Compare
|
My latest push adds a new Renaming the Khronos plugin to HTC does make sense. Especially now that lots of the vendors here actually use the Khronos loader, we should probably be thinking in terms of vendors, and not loaders. But I guess that'll need some more discussion |
c043ce7 to
9efa4b2
Compare
BastiaanOlij
left a comment
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.
Looks good, definitely makes things more manageable!
Agree that we should talk some more about splitting HTC into its own option and do that in a separate PR.
This started because I wanted to do something from an
EditorPluginthat wasn't specific to a single vendor, and realized that we don't have a commonEditorPlugin, we've got one for each vendor.It makes sense that we have an
EditorExportPluginfor each vendor (to keep the logic for each self-contained), but they don't each need anEditorPlugin. I think this is just left over from when we did separate C++ builds for each vendor.Then things snowballed into a number of clean-ups associated with the editor plugins and class registration :-)
Here's all the changes in this PR:
OpenXRVendorsEditorPluginthat registers all theEditorExportPluginsOpenXREditorExportPluginbase class toOpenXRVendorsEditorExportPluginto matchGDREGISTER_INTERNAL_CLASS()GDREGISTER_CLASS()macro, since the macros are the recommended API, over callingClassDB::register_class()