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

Allow namespace for cleaner class names #21215

Closed
QbieShay opened this issue Aug 20, 2018 · 16 comments
Closed

Allow namespace for cleaner class names #21215

QbieShay opened this issue Aug 20, 2018 · 16 comments

Comments

@QbieShay
Copy link
Contributor

Godot version: 3.1 custom

Issue description:
When making a game with multiple minigames, using class names becomes impossible: you need to use Objective-c style horribly long names for your classes because every classname is global.

Proposal:
Allow namespaces (nestable would be the best).
I imagine them to look like this:

MiniGameA.Objective
MiniGameB.Objective
@akien-mga
Copy link
Member

Just to be sure, you're talking about the new class_name feature, right?

Because otherwise if you don't register your class names as global, you can keep all minigames self-contained with one game never having to know about the other game's internal classes.

@QbieShay
Copy link
Contributor Author

Yes, I am talking about the new class_name feature.

Imagine a game composed of several minigames. The current options at the moment is to end up with either horribly long paths(If you want your folders to be nicely organized) or horribly long classnames. I know Godot was meant to work with paths, but if you work with paths and move something, you risk high breakage in your project that you can't really discover until you either grep for the old filename or run every single script. Not sure how it works when it comes to refactoring classnames.

Nothing that can't be worked around, but it would be nice for me to have a solid system, the current one feels very fragile.

@QbieShay
Copy link
Contributor Author

I would also be interested on @vnen opinion's on this

@Zylann
Copy link
Contributor

Zylann commented Aug 20, 2018

This is also even more relevant when you make a plugin that contains class_names, as you just can't guarantee it's not gonna conflict with other plugins using class_name, other than using aggressive prefixes or not using class_name at all in plugins (I saw that request coming from miles away, I'm sure there was mention of that in the past).
Those prefixes are even used already under the form of very explicit file paths (such as addons/zylann.hterrain/hterrain.gd or addons/vnen.tiled_importer/tiled_map_reader.gd) which are better used with imports into consts (only in code though), but if we want to use class_names from now on we need to solve the same issue better.

@vnen
Copy link
Member

vnen commented Aug 20, 2018

I was expecting someone to ask about it. I'm not against it but there are a few considerations. For instance, I assume people expect a sort of using namespace keyword to avoid typing full names all the time, and that can bring in ambiguity in a particular file which needs to be checked for.

@eon-s
Copy link
Contributor

eon-s commented Aug 20, 2018

For this, class_name could add package attribute, then using or import to look up in these packages when using a named class inside the script.

@paulhocker
Copy link

@eon-s +1 for going the pythonic way

I was testing out the new class_name features today and it just felt odd, that now we have this wonderful class_name which allow extending, but I still have to do a "preload" into a constant to use it in any other script. Seems that an import statement would be much cleaner.

# animal.gd

extends Reference
class_name Animal

func speak():
    return "(silent)"

func name():
    return "animal"


# lion.gd

extends Animal

func name():
    return "lion"

func speak():
    return "roar"


# test.gd

extends Node

# today we have to do this
const Animal = preload("./animal.gd")
const Lion = preload("./lion.gd")

# in the future it would be nice to be able to
import Animal
import Lion

func _speak(animal):
    if animal is Animal:
        print("the %d year old '%s', says '%s'" % [animal.age, animal.name, animal.speak() ])
		
		
func _ready():
	
    var animal = Animal.new()
    var lion = Lion.new()
	
    _speak(animal)
    _speak(lion)```

@eon-s
Copy link
Contributor

eon-s commented Sep 8, 2018

@paulhocker open a new issue for that (but search first, I think that was mentioned somewhere else), I remember having tested with static functions and these do worked with the class name.

@willnationsdev
Copy link
Contributor

Technically, all you have to do to define a "namespace" is make another script class that references the other scripts as constants. So, you can already do this kind of thing out-of-the-box. It's just that there is manual work involved in creating the namespace through another script.

# res://addons/my_plugin/my_namespace.gd
extends Reference
class_name MyNamespace
const MyClass = preload("my_class.gd")

# res://my_node.gd
extends Node
func _ready():
    print(MyNamespace.MyClass)

Right now, creating that intermediate script is done manually, and to put multiple classes in it, you have to create the scripts and then update the information in the namespace script.

On the other hand, it would be more convenient if you could just namespace the type directly into the class_name bit and have the engine generate this namespace-like behavior on the backend. For example, it could create a faux script class in C++ for the top-level namespace and then recursively generate constants for each sub-namespace until you get to the final script. Then all of the namespacing would work out-of-the-box, with autocompletion, for all languages. The only change to the logic necessary would be...

  1. enabling languages to "parse" out the full namespace (allowing periods) rather than using only a single identifier.
    • GDScript is actually the only language that would need to change since it's the only one that uses a hard identifier.
    • C# is already context-aware of its own full namespace.
    • VisualScript and NativeScript both use editable text fields that accept any viable string value (including periods).
  2. implementing a class that can be interpreted as a Script but which only stores constants for other Scripts.
  3. properly generating instances of the above class and recursively generating constants when creating global access points for the scripts.
    • GDScript stores the global variables in its GDScriptLanguage class. We would have to generate instances of these classes and store them here.
    • the complete, but not yet PRed VisualScript implementation branch I have for script classes just directly loads the path stored in the ScriptServer on command (so it does not store them as global variables anywhere). We would have to make sure that the load() operation would work for this, or - even better - make it so that VisualScript generated these faux scripts upon request (which would mean they have to go in core, probably in /core/script_language.h).
    • No idea how I'm gonna implement C# script class access points, but I suspect it will involve creating constants in the Godot.GD namespace, allowing for GD.MyClass or some such.
    • NativeScript would require the base API to include a method of fetching the information. My first thought would be exposing ScriptServer as a singleton that has properties for each registered script class, therefore allowing something like const Script *my_class = ScriptServer.get("MyClass");. I will probably need to expose it anyway in order to implement Simplify scripting API "type" and "icon" data acquisition for scripts and scenes godot-proposals#103.
  4. updating the CreateDialog to parse the individual class name out of the full namespace name that the ScriptServer provides to it (and perhaps showing just the namespace beside the script filename?).
  5. updating the CreateDialog's search filter to account for the full namespace name (since the text will just become the last name).

@vnen what do you think of this idea? You seem to know the most about Godot's scripting API / script class plans over time.

@Zylann
Copy link
Contributor

Zylann commented Oct 2, 2019

@willnationsdev aren't globally named classes a Godot feature? It's used to register in dialogs or other systems and allows to specify an icon). Since it is, C# might as well need such namespacing, even if language-wise it sounds more like a redundant burden.
But if namespaces boil down to having a . to specify the class name, I think we should have an "import" of namespaces in GDScript (like using in C#) to actually be useful when writing scripts. Because if not, we could as well just allow the . in class names and call it a day (or just use _). Unless I'm missing something?
They could be used as categories though, but it kinda conflicts with the inheritance-based class display in dialogs.

@willnationsdev
Copy link
Contributor

@Zylann

aren't globally named classes a Godot feature? It's used to register in dialogs or other systems and allows to specify an icon).

Yes, this is the "script class" system.

Since it is, C# might as well need such namespacing, even if language-wise it sounds more like a redundant burden.

Well, as for accessing things by name, I am suggesting that if, say, GDScript had MyNamespace.MyClass, then we would...

  1. Register "MyNamespace.MyClass" as a path to the GDScript in the ScriptServer.
  2. Generate a faux C++ Script type and assign it as a constant in C#'s Godot.GD namespace called MyNamespace.
  3. Recursively continue down the path and identify that MyClass is the last value. Create the script stored in ScriptServer and assign it a MyClass constant in the MyNamespace type.

Now C# has a global, namespaced access point.

As for providing a namespaced name for C# types, we can program them to already do that on the backend because they are already aware of their context. Don't have to explicitly give them any kind of name. Adding a ScriptClass class attribute would be enough.

I think we should have an "import" of namespaces in GDScript (like using in C#) to actually be useful when writing scripts.

That would already exist:

const MyClass = MyNamespace.MyClass

Unless we also needed people to be able to "import" multiple types from MyNamespace all at once (a valid concern).

we could as well just allow the . in class names and call it a day

This is already what I am planning on doing.

@Zylann
Copy link
Contributor

Zylann commented Oct 2, 2019

That would already exist:

const MyClass = MyNamespace.MyClass

That's back to square one^^ I mean, being able to actually scope class name lookup by saying "I am within this namespace" at the beginning of the script, so that such const isn't needed in the first place. Yeah that's what I meant by multiple imports.

@vnen
Copy link
Member

vnen commented Oct 2, 2019

I mean, being able to actually scope class name lookup by saying "I am within this namespace" at the beginning of the script, so that such const isn't needed in the first place. Yeah that's what I meant by multiple imports.

This is the main reason why I haven't bothered with namespaces so far. Need some time to design a proper way to use it. I expect users to be able to get inside the namespace scope for the whole script, or for the rest of block, so this needs some way to do it (while it might seem trivial, there are always some edge cases hiding in the corners that we have to take in account). I also want it to be simple to use.

Another point is whether or not to allow MyNamespace to also be a class. That is: what happens if I declare a class_name Shrubbery somewhere and another script defines a class_name Shrubbery.Shrub elsewhere. Is this a conflict? Who'll have precedence (maybe just who arrived first, in filesystem scan order)? Will all the scripts depending on Shrubbery.Shrub break if someone introduces a Shrubbery class?

Implementation is just a detail after all questions are answered.

Though I don't really like creating empty scripts just to provide namespace functionality. What if the user has their own script with the same name? The system will find a script there (since there is one) but will be empty and without the methods defined by the user, generating cryptic error messages. I think a system like this should be done properly. Probably have to change ScriptServer and how classes are stored in ProjectSettings.

Well, I could simply allow . to be part of the identifier, and everything would be solved already (since for ScriptServer it's only a string->path mapping). But then we wouldn't be able to enter the namespace scope and have to enter the FQCN every time, which is not what users usually expect.

(I just realized I commented on this before. I even forgot about ambiguity, which is also an issue. My concerns are the same, I just never had time to think them through).

@willnationsdev
Copy link
Contributor

@vnen

I expect users to be able to get inside the namespace scope for the whole script, or for the rest of block, so this needs some way to do it (while it might seem trivial, there are always some edge cases hiding in the corners that we have to take in account). I also want it to be simple to use.

That's fair, but it's more of a GDScript-specific concern that shouldn't impact us being able to design/figure out the backend support in the meantime.

Once we have a backend implementation in place, it will be easier to create a GDScript Issue that discusses more concrete front-end design and implementation for hooking up with the backend.

But then we wouldn't be able to enter the namespace scope and have to enter the FQCN every time, which is not what users usually expect.

This is the crux of the backend issue. The way the data is represented has an effect on how easy it is to navigate the data.

Our options, as I see it, are as follows:

  1. The script server maps FQCNs to file paths and we expose each one as a property, exactly as its identifier exists, in a ScriptServer singleton, enabling users to write ScriptServer.get("MyNamespace.MyClass"). This would be the most abysmal user experience of all the options.
    • Users would not be able to easily...
      • acquire autocompletion support.
      • import namespace scopes.
    • Doing either of the above would require iterating through all records to find out which ones were in the section you wanted (a linear time operation).
  2. The script server maps FQCNs to the paths. Then, on the scripting language side of things, we create intermediate Objects to take advantage of the property system's autocompletion features in each language (my earlier proposed idea).
    • This does not change most of the core engine code and has the least impact on existing systems.
  3. script_language.h defines the namespace Reference objects (or Dictionaries, if you want). The script server stores a hierarchy of them. All script class methods are refactored to look up FQCNs by breaking them up into segments and searching through the tree hierarchy (converting what were previously O(1) operations into O(N) operations where N is the depth of the tree).
    • The advantage is that you then have one tree hierarchy of namespaced classes that all languages can draw from to query their data. You don't have 3-4 separate namespace hierarchy definitions. You have 1 in the core that the whole engine (and even the editor) can have access to.
  4. Same as the previous option, except you also keep a map of FQCN->ScriptClass* records. That way you can also do constant-time lookups for non-hierarchical name lookups (which the editor does a lot and which languages would also do when performing validation).
    • The main problem here would be ensuring the synchronization and memory safety between the two map structures referencing the same data (one a flat map, the other a segmented hierarchical one).

Out of these options, I'm actually starting to like that last one the most.

Another point is whether or not to allow MyNamespace to also be a class. That is: what happens if I declare a class_name Shrubbery somewhere and another script defines a class_name Shrubbery.Shrub elsewhere. Is this a conflict? Who'll have precedence (maybe just who arrived first, in filesystem scan order)? Will all the scripts depending on Shrubbery.Shrub break if someone introduces a Shrubbery class?

If a user created a script that shared a name with a namespace, then the ScriptServer would need to confirm that the given script and the namespace both define the same set of access points to other namespaces / named types. And if not, then the namespace would still be used and the script would fail to load / give an error. The error would say something along the lines of "does not define requisite classes registered elsewhere," (and then the ScriptEditor can list those FQCNs/filepaths so that users can find/fix them). Only when all the requisites are satisfied would the script load successfully and replace the namespace object.

You would probably have to do a secondary sweep over the script classes to validate that information. After all, you cannot know the full set of requisite classes until all script classes have been defined. That is also the time, though, that we would already need to be doing a secondary parsing process to figure out where our holes are, for the sake of generating the namespace objects.

If you don't support allowing users to define scripts that replace namespaces, then I think you stand to get a lot of confusion and negative feedback since the manual process fully supports that (as described), so the automated process should also support that.

If you have another idea on how to implement the backend's data structure in the ScriptServer/languages, I'd be happy to hear your ideas.

@Xrayez
Copy link
Contributor

Xrayez commented Oct 6, 2020

I think the discussion can continue in godotengine/godot-proposals#1566.


All feature and improvement proposals for the Godot Engine are now being discussed and reviewed in the dedicated Godot Improvement Proposals (GIP) (godotengine/godot-proposals) issue tracker. The GIP tracker has a detailed issue template designed so that proposals include all the relevant information to start a productive discussion and help the community assess the validity of the proposal for the engine.

The main (godotengine/godot) tracker is now solely dedicated to bug reports and Pull Requests, enabling contributors to have a better focus on bug fixing work. Therefore, all feature proposals on the main issue tracker are being closed.

@Calinou
Copy link
Member

Calinou commented Oct 6, 2020

Closing per @Xrayez's comment.

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

No branches or pull requests

9 participants