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

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

Closed

Conversation

neikeq
Copy link
Contributor

@neikeq neikeq commented Mar 12, 2019

I propose this as a replacement for the underscore thing.
This PR doesn't remove the existing extra code that is needed to support classes with underscores (#26922 lists most of them).

The namespace names are just placeholders. I don't know what's our naming convention for namespaces.
There is a lot of ::ClassDB::bind_method lines in core_bind.cpp, which could be avoided if we move core_bind::ClassDB to its own namespace so it doesn't bother the other classes. Edit: Moved to its own namespace.

@fire
Copy link
Member

fire commented Mar 12, 2019

Are there any problems from the c-api end? Aka gdnative?

@neikeq neikeq force-pushed the underscores-are-evil-i-tell-ya branch from fe054ba to 3ee87e1 Compare March 12, 2019 23:29
@neikeq
Copy link
Contributor Author

neikeq commented Mar 12, 2019

@fire I don't think there is any, but I didn't check. Most if not all the code that works around these underscores is just to strip it (or add it in case of a lookup).

@neikeq neikeq force-pushed the underscores-are-evil-i-tell-ya branch 2 times, most recently from 619d448 to fc783b9 Compare March 13, 2019 15:45
@reduz
Copy link
Member

reduz commented Apr 4, 2019

I am surprised, never expected anything like this to work, if it really does feel free to go ahead and merge.

@neikeq
Copy link
Contributor Author

neikeq commented Apr 4, 2019

It may be safer to leave this for 4.0. I don't think it breaks anything, but I don't know what some GDNative language bindings out there are doing, so just to be safe...

@neikeq neikeq modified the milestones: 3.2, 4.0 Apr 4, 2019
@neikeq
Copy link
Contributor Author

neikeq commented Apr 4, 2019

The only annoyance I found is that we can't use VARIANT_ENUM_CAST because the template specializations it declares must be in the same namespace where the template was originally declared (which is why I moved all the VARIANT_ENUM_CAST to the bottom outside the namespace in core_bind.h).

@neikeq neikeq force-pushed the underscores-are-evil-i-tell-ya branch 2 times, most recently from 062e3a8 to cbaf746 Compare April 6, 2019 12:49
@aaronfranke
Copy link
Member

Does this change anything on the front end?

ptrojahn added a commit to ptrojahn/godot that referenced this pull request Jun 28, 2019
@ptrojahn
Copy link
Contributor

I created #30153 which only removes the checks.

@vnen
Copy link
Member

vnen commented May 17, 2020

Any news in this regard? I'm now rewriting GDScript code and it would be nice to not have to deal with those pesky underscores.

@neikeq
Copy link
Contributor Author

neikeq commented May 17, 2020

I'll be revisiting this soon, but right now I'm focusing on other tasks.

Xrayez added a commit to Xrayez/godot-geomtools that referenced this pull request May 22, 2020
@Xrayez
Copy link
Contributor

Xrayez commented May 31, 2020

Inspired by this PR, I've already done something similar while developing a custom module. But stumbled upon another bug while regenerating documentation and then generating the mono glue, see Xrayez/godot-geomtools#3.

@neikeq
Copy link
Contributor Author

neikeq commented May 31, 2020

@Xrayez The issue is with your PolyDecompType enum. Enums in namespaces needs the changes I made to type info in this PR.

@Xrayez
Copy link
Contributor

Xrayez commented May 31, 2020

Oh ok, so it would be nice for these changes to be cherry-picked to 3.2, else I'd have to make similar changes and/or revert to how it's now.

@Xrayez
Copy link
Contributor

Xrayez commented May 31, 2020

Just saying that I have some other module with having enums in namespaces which is an issue in and of itself (which is worth treating via separate PR perhaps which can be cherry-picked), for instance see: Xrayez/godot-anl@642b470#diff-142e17f094b421b5777b79be43c94633R336-R339. I've just had to duplicate enums eventually.

@aaronfranke aaronfranke marked this pull request as draft June 17, 2020 01:18
@aaronfranke
Copy link
Member

@neikeq This was last rebased over a year ago, what's the status of this?

@neikeq
Copy link
Contributor Author

neikeq commented Jul 1, 2020

It should be easy to re-do it for the master branch, but I haven't had time for that yet. Maybe next month.

@neikeq
Copy link
Contributor Author

neikeq commented Jul 1, 2020

I've added this to my TODO list for July.

@akien-mga
Copy link
Member

Superseded by #51627.

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.

9 participants