Skip to content
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

Cleanup some GLOBAL_DEFs #80972

Merged
merged 1 commit into from
Sep 16, 2023
Merged

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Aug 24, 2023

Addresses #55730 (enough to close it?)

Based on godotengine/godot-proposals#4771 (comment) I experimented a bit with registering settings. Moving setting definitions to _bind_methods() sounds like feasible idea, but it has a few problems:

  • Some classes are not registered. This means their _bind_methods() is not called until a first instance is created. The workaround I found is manually calling initialize_class().
  • Some defines may be used in platform-specific code that isn't compiled on other platforms (e.g. display.iOS/use_cadisplaylink, which is the only not documented project setting after this PR). This is a bigger problem, because there might be whole platform-specific classes that bind public methods, but aren't documented. I don't think we have any in the official engine though, it's more a problem with custom platforms.
    • btw display.iOS/use_cadisplaylink is not even available in the editor. It's basically a dead setting that you can set either in iOS editor build or manually in project.godot.
  • Some classes aren't even GDClass. MessageQueue is an example; its setting is registered though, because the class is created early and the define is in the constructor. I think such classes would be exception.
  • A whole another problem are editor settings. register_editor_types() is called before EditorSettings are created, which means no editor setting can be registered inside _bind_methods() of editor classes. This would require Main reorganization.

As for what this PR does:

  • remove duplicate setting definition from EditorRunBar
  • properly expose "collada/use_ambient" and document it
  • fix how "network/limits/webrtc/max_channel_in_buffer_kb" is defined. The definition did not make sense on many levels, unless you assume that it was some form of code obfuscation 🙃 (although the main reason why I fixed it is that it messed with my setting scanner script)

I might try to do something about the EditorSettings problem next.

@@ -258,6 +259,8 @@ void register_editor_types() {
EditorPlugins::add_by_type<TileSetEditorPlugin>();

// For correct doc generation.
EditorSceneFormatImporterCollada::initialize_class();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it simpler / less hacky (or differently in a more common way) to just move that editor setting to editor_settings.cpp with all the other ones?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a whole idea behind this.

Originally I suggested in my proposal (linked above) that all settings should be in one place, but recently I though it's better to keep them closer to their origin. For modules it's pretty much a must. For core classes it makes them less prone to being forgotten in case the class is removed/changed etc. Also when trying to add a setting, it's easier to drop it near the class than in some jumbo list, where you have to wonder where is the best place to put it.

I opened that proposal, because the settings were defined all over the place and with multiple definitions. But another way for cleanup is to define a location where a setting can be placed in any class, which would make them easy to find and avoid duplication. The only drawback would be that there is less control over the order of definition (and probably that it requires "hacks" like this one).

This PR is kind of experimental; I tested this new approach when implementing it and the initialize_class() is one of the solutions I came up with. Maybe it doesn't make sense though and it's better to move it to editor_settings.cpp, idk.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I also think that settings definition should be standardized, while keeping it modular / self-contained, which as you say is a must for modules.

I recently tried something similar for editor settings defined in modules: #80576 (comment)

The big issue which I noticed while working on this is that the settings would still end up in doc/classes/{Editor,Project}Settings.xml, so they can't be fully self-contained for modules. We'd need a way to split the module specific settings to their own XML files in <module path>/doc_classes/, and append those to the in-editor ProjectSettings and EditorSettings reference (possibly with an indication that those settings depend on a module, since we'd have this extra piece of info then).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the time being, for Collada, I'd suggest to just add the EDITOR_DEF in editor_settings.cpp's constructor like we did for many others.

What's done here as a PoC is interesting but also inconsistent, so I'd prefer not to merge this approach until we have a solid design on how things should work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK the XML files are only for the descriptions and some metadata. The documentation data will still be generated by the editor even if it doesn't have XML source.

The documentation concern mostly affects custom modules. I think we could allow for duplicate XML files inside modules, which would be used to fetch descriptions missing in the base file. Core modules can still register in the base file; even if someone compiles them out, the description will be simply unused.

@KoBeWi
Copy link
Member Author

KoBeWi commented Sep 15, 2023

Removed the collada changes, now it's defined in project settings instead.

@akien-mga akien-mga modified the milestones: 4.x, 4.2 Sep 15, 2023
@akien-mga akien-mga changed the title Cleanup some GLOBAL_DEFs Cleanup some GLOBAL_DEFs Sep 15, 2023
@akien-mga akien-mga merged commit 33b95f0 into godotengine:master Sep 16, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants