Namespaces instead of underscore prefix for binds, remove cruft #51627

mhilbrunner · 2021-08-13T14:47:34Z

Redoing #26990 for current master.

This is now fully working and tested :)

The first commit moves underscored classes to namespaces,
the second removes a lot of hacks/cruft checking for underscores on classes, which we now no longer need to do.

I recommend reviewing both commits sperately.

Note: This currently means that for namespaced classes, the macros will check for defines like ClassDB_Disable_core_bind::Directory instead of ClassDB_Disable_Directory. (In the GDREGISTER_CLASS macro).
(Used when passing disabled_classes to SCons.)
This only affects the small handful of classes that were previously underscored:

ClassDB
Directory
Engine
File
Geometry2D
Geometry3D
GodotSharp
Marshalls
Mutex
OS
ResourceLoader
ResourceSaver
Semaphore
Thread
VisualScriptEditor

Getting rid of the current special casing for all those underscore classes seems prudent, as even while working on this PR I found various bugs related to it - cases where underscores were checked in some cases, in some not, or were applied inconsistently; sometimes classes were displayed with underscores in Godot UI, sometimes not. The list of underscore_classes this removes even exhibits a few errors: JSON is no longer current, and Geometry has become Geometry2D and Geometry3D.

aaronfranke · 2021-08-13T14:55:39Z

core/core_bind.cpp

@@ -41,39 +41,41 @@
 #include "core/os/keyboard.h"
 #include "core/os/os.h"

-////// _ResourceLoader //////
+namespace core_bind {


Shouldn't it be:

Suggested change

namespace core_bind {

namespace CoreBind {

I took it from the original PR, and it seems common - if we want CamelCase for namespaces in Godot, I can switch it to that.

Namespaces in C++ are generally lower case/snake case. See for instance Google's C++ style guide: https://google.github.io/styleguide/cppguide.html#Namespace_Names.

We don't have strong guidelines on this as we haven't been using namespaces much so far. Current master branch does have some and most are PascalCase:

$ rg -g'!thirdparty' -g'!mono' "^namespace " tests/test_aabb.h 40:namespace TestAABB { tests/test_path_follow_2d.h 39:namespace TestPathFollow2D { tests/test_crypto.h 38:namespace TestCrypto { tests/test_local_vector.h 38:namespace TestLocalVector { tests/test_dictionary.h 40:namespace TestDictionary { tests/test_geometry_2d.h 39:namespace TestGeometry2D { tests/test_path_3d.h 39:namespace TestPath3D { tests/test_vector.h 38:namespace TestVector { tests/test_shader_lang.h 36:namespace TestShaderLang { tests/test_path_follow_3d.h 39:namespace TestPathFollow3D { tests/test_hashing_context.h 38:namespace TestHashingContext { tests/test_physics_3d.h 36:namespace TestPhysics3D { tests/test_variant.h 39:namespace TestVariant { tests/test_image.h 40:namespace TestImage { tests/test_marshalls.h 38:namespace TestMarshalls { tests/test_physics_3d.cpp 410:namespace TestPhysics3D { tests/test_text_server.h 40:namespace TestTextServer { tests/test_translation.h 40:namespace TestTranslation { tests/test_gui.cpp 59:namespace TestGUI { tests/test_astar.h 43:namespace TestAStar { tests/test_pck_packer.h 40:namespace TestPCKPacker { tests/test_node_path.h 38:namespace TestNodePath { tests/test_utils.h 36:namespace TestUtils { tests/test_expression.h 38:namespace TestExpression { tests/test_math.h 36:namespace TestMath { tests/test_oa_hash_map.h 36:namespace TestOAHashMap { tests/test_gui.h 36:namespace TestGUI { tests/test_geometry_3d.h 41:namespace Test3DGeometry { tests/test_render.cpp 44:namespace TestRender { tests/test_resource.h 41:namespace TestResource { tests/test_xml_parser.h 41:namespace TestXMLParser { tests/test_basis.h 40:namespace TestBasis { tests/test_json.h 38:namespace TestJSON { tests/test_array.h 43:namespace TestArray { tests/test_command_queue.h 45:namespace TestCommandQueue { tests/test_rect2.h 38:namespace TestRect2 { tests/test_file_access.h 37:namespace TestFileAccess { tests/test_math.cpp 51:namespace TestMath { tests/test_string.h 45:namespace TestString { tests/test_gradient.h 40:namespace TestGradient { tests/test_ordered_hash_map.h 41:namespace TestOrderedHashMap { tests/test_class_db.h 45:namespace TestClassDB { tests/test_paged_array.h 38:namespace TestPagedArray { tests/test_oa_hash_map.cpp 36:namespace TestOAHashMap { tests/test_color.h 38:namespace TestColor { tests/test_list.h 38:namespace TestList { tests/test_object.h 59:namespace TestObject { tests/test_random_number_generator.h 37:namespace TestRandomNumberGenerator { tests/test_physics_2d.cpp 398:namespace TestPhysics2D { tests/test_config_file.h 38:namespace TestConfigFile { tests/test_curve.h 38:namespace TestCurve { tests/test_physics_2d.h 36:namespace TestPhysics2D { tests/test_method_bind.h 42:namespace TestMethodBind { core/variant/type_info.h 66:namespace GodotTypeInfo { tests/test_render.h 36:namespace TestRender { scene/resources/surface_tool.cpp 907:namespace { tests/test_shader_lang.cpp 44:namespace TestShaderLang { platform/uwp/app.h 43:namespace GodotUWP modules/regex/tests/test_regex.h 39:namespace TestRegEx { modules/navigation/nav_utils.h 44:namespace gd { modules/fbx/fbx_parser/FBXDocumentUtil.cpp 83:namespace FBXDocParser { 84:namespace Util { modules/fbx/fbx_parser/FBXDocument.h 50:namespace FBXDocParser { 1234:namespace std { modules/fbx/fbx_parser/FBXImportSettings.h 79:namespace FBXDocParser { modules/fbx/fbx_parser/FBXMaterial.cpp 86:namespace FBXDocParser { modules/fbx/fbx_parser/FBXPose.cpp 81:namespace FBXDocParser { modules/fbx/fbx_parser/ByteSwapper.h 82:namespace FBXDocParser { 244:namespace Intern { modules/fbx/fbx_parser/FBXAnimation.cpp 83:namespace FBXDocParser { modules/fbx/fbx_parser/FBXTokenizer.h 86:namespace FBXDocParser { modules/fbx/fbx_parser/FBXModel.cpp 82:namespace FBXDocParser { modules/fbx/fbx_parser/FBXUtil.cpp 82:namespace FBXDocParser { 83:namespace Util { modules/fbx/fbx_parser/FBXBinaryTokenizer.cpp 84:namespace FBXDocParser { 140:namespace { modules/fbx/fbx_parser/FBXNodeAttribute.cpp 82:namespace FBXDocParser { modules/fbx/fbx_parser/FBXDeformer.cpp 85:namespace FBXDocParser { modules/fbx/fbx_parser/FBXParser.h 90:namespace FBXDocParser { modules/fbx/fbx_parser/FBXParser.cpp 90:namespace { 107:namespace FBXDocParser { 516:namespace { modules/fbx/fbx_parser/FBXUtil.h 82:namespace FBXDocParser { 84:namespace Util { modules/fbx/fbx_parser/FBXTokenizer.cpp 83:namespace FBXDocParser { 101:namespace { modules/fbx/fbx_parser/FBXCommon.h 80:namespace FBXDocParser { modules/fbx/fbx_parser/FBXProperties.cpp 82:namespace FBXDocParser { 94:namespace { modules/fbx/fbx_parser/FBXProperties.h 85:namespace FBXDocParser { modules/fbx/fbx_parser/FBXMeshGeometry.h 89:namespace FBXDocParser { modules/mbedtls/tests/test_crypto_mbedtls.h 38:namespace TestCryptoMbedTLS { modules/fbx/fbx_parser/FBXMeshGeometry.cpp 85:namespace FBXDocParser { modules/fbx/fbx_parser/FBXDocumentUtil.h 84:namespace FBXDocParser { 85:namespace Util { modules/mbedtls/tests/test_crypto_mbedtls.cpp 36:namespace TestCryptoMbedTLS { modules/fbx/fbx_parser/FBXDocument.cpp 90:namespace FBXDocParser { editor/debugger/debug_adapter/debug_adapter_types.h 37:namespace DAP { modules/gdscript/tests/test_gdscript.cpp 50:namespace GDScriptTests { modules/gdscript/tests/gdscript_test_runner.cpp 48:namespace GDScriptTests { servers/physics_3d/gjk_epa.cpp 86:namespace GjkEpa2 { modules/gdscript/tests/gdscript_test_runner.h 40:namespace GDScriptTests { modules/gdscript/tests/test_gdscript.h 37:namespace GDScriptTests { servers/rendering/renderer_rd/forward_mobile/scene_shader_forward_mobile.h 38:namespace RendererSceneRenderImplementation { servers/rendering/renderer_rd/forward_clustered/scene_shader_forward_clustered.h 38:namespace RendererSceneRenderImplementation { servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.h 41:namespace RendererSceneRenderImplementation { servers/rendering/renderer_rd/forward_mobile/render_forward_mobile.h 40:namespace RendererSceneRenderImplementation { modules/gdscript/language_server/lsp.hpp 38:namespace lsp { 349:namespace TextDocumentSyncKind { 658:namespace DiagnosticSeverity { 766:namespace MarkupKind { 829:namespace CompletionItemKind { 862:namespace InsertTextFormat { 1076:namespace SymbolKind { 1338:namespace FoldingRangeKind { 1398:namespace CompletionTriggerKind { drivers/png/png_driver_common.cpp 38:namespace PNGDriverCommon { drivers/png/png_driver_common.h 36:namespace PNGDriverCommon { modules/gdnative/tests/test_variant.h 39:namespace TestGDNativeVariant { modules/gdscript/tests/gdscript_test_runner_suite.h 37:namespace GDScriptTests { tests/test_lru.h 39:namespace TestLRU { tests/test_time.h 47:namespace TestTime {

PascalCase looks more natural to me in Godot's context, but again since we don't use those much, it's not set in stone.

mhilbrunner · 2021-08-13T20:22:32Z

Update: this is now halfway working, singleton classes like OS work correctly from GDScript.

Simple core_bind namespace classes like File, Directory and Mutex however show up as not exposed/GDNativeClasses in GDScript.

core/object/class_db.h

neikeq · 2021-08-16T15:53:26Z

Missing renaming _GodotSharp to mono_bind::GodotSharp here:

godot/modules/mono/editor/editor_internal_calls.cpp

Line 244 in 1057a10

    
           _GodotSharp::get_singleton()->call_deferred(SNAME("_reload_assemblies"), (bool)p_soft_reload);

neikeq · 2021-08-16T15:56:48Z

Simple core_bind namespace classes like File, Directory and Mutex however show up as not exposed/GDNativeClasses in GDScript.

I just tested this PR. Not sure what problem you're having with those classes. They show up in the documentation view and auto-completion. They're also generated in the C# bindings, which only happens if they're exposed.

mhilbrunner · 2021-08-17T11:13:53Z

Missing renaming _GodotSharp to mono_bind::GodotSharp here:

Fixed, thanks. Also fixed some formatting things the CI complained about.

mhilbrunner · 2021-08-17T12:08:55Z

Update: Everything actually works, but this check triggers for classes like File/Directory/Thread changed to a namespace with this PR:

godot/modules/gdscript/gdscript_analyzer.cpp

Lines 3289 to 3295 in ca6c5cf

    
           if (base_native != StringName()) { 
        
           	// Empty native class might happen in some Script implementations. 
        
           	// Just ignore it. 
        
           	if (!class_exists(base_native)) { 
        
           		ERR_FAIL_V_MSG(false, vformat("Native class %s used in script doesn't exist or isn't exposed.", base_native)); 
        
           	} 
        
           }

Seems like these are just superfluous warnings, and they even come with a comment saying they seem harmless. Everything works with this PR, but it'd still be nice to know the cause for these log messages.

mhilbrunner · 2021-08-17T13:06:27Z

Okay, turns out there was a lot of hacks testing for underscores, and there was a list of classes that have underscores, and if they have not, they get the underscore added back.

Removed all of those hacks, and now it works beautifully without warnings, errors, or log messages :)

See the below, which the second commit now removes:

godot/modules/gdscript/gdscript_parser.cpp

Lines 97 to 129 in ca6c5cf

    
           // TODO: Move this to a central location (maybe core?). 
        
           static HashMap<StringName, StringName> underscore_map; 
        
           static const char *underscore_classes[] = { 
        
           	"ClassDB", 
        
           	"Directory", 
        
           	"Engine", 
        
           	"File", 
        
           	"Geometry", 
        
           	"GodotSharp", 
        
           	"JSON", 
        
           	"Marshalls", 
        
           	"Mutex", 
        
           	"OS", 
        
           	"ResourceLoader", 
        
           	"ResourceSaver", 
        
           	"Semaphore", 
        
           	"Thread", 
        
           	"VisualScriptEditor", 
        
           	nullptr, 
        
           }; 
        
           StringName GDScriptParser::get_real_class_name(const StringName &p_source) { 
        
           	if (underscore_map.is_empty()) { 
        
           		const char **class_name = underscore_classes; 
        
           		while (*class_name != nullptr) { 
        
           			underscore_map[*class_name] = String("_") + *class_name; 
        
           			class_name++; 
        
           		} 
        
           	} 
        
           	if (underscore_map.has(p_source)) { 
        
           		return underscore_map[p_source]; 
        
           	} 
        
           	return p_source; 
        
           }

mhilbrunner · 2021-08-17T13:41:40Z

Rebased and fixed conflicts, and added Neikeq as co-author, as a large part of this is based on the original PR. :)

Thanks to neikeq for the initial work. Co-authored-by: Ignacio Roldán Etcheverry <neikeq@users.noreply.github.com>

Way less cruft. :) Co-authored-by: Ignacio Roldán Etcheverry <neikeq@users.noreply.github.com>

mhilbrunner · 2021-08-17T18:49:42Z

@akien-mga This is now fully working and should be good to go.
Also removes quite some brittle code :)

Let me know if you want me to change the namespaces to PascalCase.

The only thing to note is in the PR description about class names for disable_classes in SCons. If we want to change that handling, I'd prefer to do that in further PRs, because this thing is getting unwieldy and hard to maintain with master/other PRs.

neikeq · 2021-08-17T23:18:20Z

Note: This currently means that for namespaced classes, the macros will check for defines like ClassDB_Disable_core_bind::Directory instead of ClassDB_Disable_Directory. (In the GDREGISTER_CLASS macro).
(Used when passing disabled_classes to SCons.)

This will be a problem because AFAIK the disabled_classes option is intended to be used by the Godot editor in the future, and the Godot editor has no knowledge of those namespaces. That's why I recommended adding a new/overload of the GDREGISTER_CLASS macro for classes with namespaces if the namespace part cannot be trimmed with the current one. Something like GDREGISTER_NS_CLASS(core_bind::Directory, Directory).

akien-mga · 2021-08-17T23:22:20Z

I'm not sure it would make sense to disable any of those classes anyway, they're the most core you can get (aside from VisualScriptEditor but that's the odd one out, I don't even know why it's exposed this way, it's probably unnecessary).

mhilbrunner · 2021-08-17T23:24:46Z

I'm not sure it would make sense to disable any of those classes anyway, they're the most core you can get (aside from VisualScriptEditor but that's the odd one out, I don't even know why it's exposed this way, it's probably unnecessary).

That was my thinking too. However if we decide this needs to work, I'm happy to work on that. I'd appreciate if we could merge this PR first however, I'm so not looking forward to conflicts and rebasing, as this mostly touches function/class signatures and so has a lot of surface area.

Fixing up the macro or adding another one should be a small additional PR, easier to review that way. But I'd need time to get into the Godot macros and to test it, and thats time for conflicts to arise. :P

aaronfranke · 2021-08-17T23:39:48Z

There isn't a use case for disabling most of those, except that GodotSharp should be disabled when compiling without the Mono module, and VisualScriptEditor only makes sense to include in the editor so it should be disabled on export templates.

akien-mga

Looks great! Thanks @mhilbrunner and @neikeq!

mhilbrunner added enhancement topic:core labels Aug 13, 2021

mhilbrunner added this to the 4.0 milestone Aug 13, 2021

mhilbrunner requested review from a team as code owners August 13, 2021 14:47

mhilbrunner marked this pull request as draft August 13, 2021 14:47

mhilbrunner force-pushed the todo-for-neikeq branch from 8bec623 to dbcc2ba Compare August 13, 2021 14:52

aaronfranke reviewed Aug 13, 2021

View reviewed changes

mhilbrunner force-pushed the todo-for-neikeq branch from dbcc2ba to 6eaa125 Compare August 13, 2021 15:12

neikeq reviewed Aug 13, 2021

View reviewed changes

core/object/class_db.h Show resolved Hide resolved

mhilbrunner force-pushed the todo-for-neikeq branch 2 times, most recently from 6220cf3 to 129cc41 Compare August 17, 2021 11:13

mhilbrunner marked this pull request as ready for review August 17, 2021 11:14

mhilbrunner force-pushed the todo-for-neikeq branch from 129cc41 to d9ce1ec Compare August 17, 2021 11:56

mhilbrunner requested review from a team as code owners August 17, 2021 13:07

mhilbrunner changed the title ~~[DRAFT] Namespaces instead of underscore prefix for binds~~ Namespaces instead of underscore prefix for binds Aug 17, 2021

mhilbrunner force-pushed the todo-for-neikeq branch from bfbc73a to 3da0905 Compare August 17, 2021 13:41

mhilbrunner changed the title ~~Namespaces instead of underscore prefix for binds~~ Namespaces instead of underscore prefix for binds, remove cruft Aug 17, 2021

mhilbrunner force-pushed the todo-for-neikeq branch from 3da0905 to a0fb712 Compare August 17, 2021 13:57

mhilbrunner and others added 2 commits August 17, 2021 16:10

Namespaces instead of underscore prefix for binds

81f7d18

Thanks to neikeq for the initial work. Co-authored-by: Ignacio Roldán Etcheverry <neikeq@users.noreply.github.com>

Remove underscore hacks

5161c97

Way less cruft. :) Co-authored-by: Ignacio Roldán Etcheverry <neikeq@users.noreply.github.com>

mhilbrunner force-pushed the todo-for-neikeq branch from a0fb712 to 5161c97 Compare August 17, 2021 14:18

akien-mga approved these changes Aug 18, 2021

View reviewed changes

akien-mga merged commit de7b6d1 into godotengine:master Aug 18, 2021

akien-mga mentioned this pull request Aug 18, 2021

Use namespaces instead of underscore prefix for core bind classes #26990

Closed

mhilbrunner deleted the todo-for-neikeq branch August 18, 2021 18:37

This was referenced Aug 19, 2021

ERROR: Class 'VisualScriptEditor' already exists #51894

Closed

Fix VisualScriptEditor after namespaces #51916

Merged

Fix VisualScriptEditor after namespaces #52023

Merged

KoBeWi mentioned this pull request Aug 28, 2021

No autocompletion for some singleton classes in an "if" statement #26114

Closed

KoBeWi mentioned this pull request Feb 20, 2022

[3.x] GDScript Editor code completion, docs clickthrough fails for OS class and its methods (fixed in master) #58369

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Namespaces instead of underscore prefix for binds, remove cruft #51627

Namespaces instead of underscore prefix for binds, remove cruft #51627

mhilbrunner commented Aug 13, 2021 •

edited

Loading

aaronfranke Aug 13, 2021

mhilbrunner Aug 13, 2021 •

edited

Loading

Xrayez Aug 13, 2021

akien-mga Aug 13, 2021

mhilbrunner commented Aug 13, 2021

neikeq commented Aug 16, 2021

neikeq commented Aug 16, 2021

mhilbrunner commented Aug 17, 2021

mhilbrunner commented Aug 17, 2021 •

edited

Loading

mhilbrunner commented Aug 17, 2021 •

edited

Loading

mhilbrunner commented Aug 17, 2021 •

edited

Loading

mhilbrunner commented Aug 17, 2021 •

edited

Loading

neikeq commented Aug 17, 2021

akien-mga commented Aug 17, 2021

mhilbrunner commented Aug 17, 2021 •

edited

Loading

aaronfranke commented Aug 17, 2021 •

edited

Loading

akien-mga left a comment

Namespaces instead of underscore prefix for binds, remove cruft #51627

Namespaces instead of underscore prefix for binds, remove cruft #51627

Conversation

mhilbrunner commented Aug 13, 2021 • edited Loading

aaronfranke Aug 13, 2021

Choose a reason for hiding this comment

mhilbrunner Aug 13, 2021 • edited Loading

Choose a reason for hiding this comment

Xrayez Aug 13, 2021

Choose a reason for hiding this comment

akien-mga Aug 13, 2021

Choose a reason for hiding this comment

mhilbrunner commented Aug 13, 2021

neikeq commented Aug 16, 2021

neikeq commented Aug 16, 2021

mhilbrunner commented Aug 17, 2021

mhilbrunner commented Aug 17, 2021 • edited Loading

mhilbrunner commented Aug 17, 2021 • edited Loading

mhilbrunner commented Aug 17, 2021 • edited Loading

mhilbrunner commented Aug 17, 2021 • edited Loading

neikeq commented Aug 17, 2021

akien-mga commented Aug 17, 2021

mhilbrunner commented Aug 17, 2021 • edited Loading

aaronfranke commented Aug 17, 2021 • edited Loading

akien-mga left a comment

Choose a reason for hiding this comment

mhilbrunner commented Aug 13, 2021 •

edited

Loading

mhilbrunner Aug 13, 2021 •

edited

Loading

mhilbrunner commented Aug 17, 2021 •

edited

Loading

mhilbrunner commented Aug 17, 2021 •

edited

Loading

mhilbrunner commented Aug 17, 2021 •

edited

Loading

mhilbrunner commented Aug 17, 2021 •

edited

Loading

mhilbrunner commented Aug 17, 2021 •

edited

Loading

aaronfranke commented Aug 17, 2021 •

edited

Loading