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

Add a way to add categories to the script variables section from GDScript #4378

Closed
bojidar-bg opened this issue Apr 19, 2016 · 28 comments
Closed

Comments

@bojidar-bg
Copy link
Contributor

(Inspired by a question on QA)

When doing scripts with a lot of configurable variables, it is nice to be able to group them in the inspector.

So, instead of having:

> Script Variables
| - Not totally unrelated var = false
| - Unrotate children = true
| - Rotate speed = 60
| - Rotate timeout = 100
| - ...

We can have

> Script Variables
| - Not totally unrelated var = false
| > Rotate
| | - Unrotate children = true
| | - Rotate speed = 60
| | - Rotate timeout = 100
| - ...

With @kubecz3k we discussed on IRC about how might we do the syntax, and we ended up with the following:

  • (1) A new keyword that starts a category
    • Pros: Allows for this issue to be closed
    • Cons: Impossible to end a category, no nesting
category rotate
export var unrotate_children = false
  • (2) An addition to export hints
    • Pros: Not much additional code to be written
    • Cons: You have to retype the name of the category for each variable
export("rotate") var unrotate_children = false
export("rotate", float) var rotate_speed = 60
  • (3) Allow variables with non-identifier names
    • Pros: Stuff like CJK support comes naturally
    • Cons: You have to retype the name of the category for each variable, doesn't look good at all
export var "rotate/unrotate_children" = false
  • (4) Add a keyword that starts a "category block"
    • Pros: Which variable is where is evident from afar, nesting of categories is supported
    • Cons: Raises suspicion on how to get that variable... should it be via category_name.variable_name, or just variable_name
category rotate:
    export var unrotate_children = false

@reduz ~ Which one of the four proposals above do you like the most?

@Zylann
Copy link
Contributor

Zylann commented Apr 19, 2016

Could this be a subset of a more generalist inspector system like Unity?
You could write a whole custom inspector as a class so you can have controls, layout and name the elements the way you like.
Then, export(), annotations etc would just be a shortcut for most of the cases.

@kubecz3k
Copy link
Contributor

kubecz3k commented Apr 19, 2016

@Zylann I believe in Unity it's like it's because they cant modify C# directly, while we have the power to adapt our language to the editor. I think the things you want to do will be possible with more complex plugin api (currently in the works).

@kubecz3k
Copy link
Contributor

kubecz3k commented Apr 19, 2016

I personally like the 1 option the most, it's the most straightforward and simple to me, the second for me would be 4, however maybe 2 and 3 are better fit for current Godot syntax?

@Zylann
Copy link
Contributor

Zylann commented Apr 19, 2016

@kubecz3k they use annotations most of the time. But using a custom class also allows to present an inspector that doesn't looks 1:1 as the script, because sometimes a script is too complex to be just presented as its key/value properties. Sometimes you even have popups and contextual buttons, which in Godot are located in the toolbar when you select a node. You need a way to specify that too, and annotations/language additions become limited at this if you don't want your script to become a mess, so... to decouple editor and script, you end up making a separate class for editor features :)

But I digress. +1 for having categories :)

@bojidar-bg
Copy link
Contributor Author

@Zylann ~ That's the idea behind editor plugins/addons.

@akien-mga
Copy link
Member

akien-mga commented Apr 19, 2016

I'd be for the hint solution, i.e. (2):

export("rotate", float) var rotate_speed = 60.0

That's the most intuitive syntax IMO (and most GDScript-like, as well as flexible for potential future bindings), and having to respecify the category for each variable is IMO better than having a new keyword that would needlessly complexify the parser (we'd have to add a is_category boolean and check against it everywhere in the parser so that we can turn it off at the first occurence of a non export line).

@RayKoopa
Copy link
Contributor

@kubecz3k Unity has its own C# interpreter, it's not Microsoft's, they just try to be as compatible as possible to the "official" C# (but fail at that here and there). They could still be C#-compatible and use attributes for that, so that's probably not the reason they don't do it like that. I guess they wanted a rather "complete" solution for even more specific inspectors.

Anyway, I'd be against the category "block", and would prefer the hint solution (2). Introducing a whole new block feels rather messy than python-esque.

@akien-mga
Copy link
Member

Not critical for the upcoming 2.1, so moving to the next milestone.

@akien-mga akien-mga modified the milestones: 2.2, 2.1 Jul 15, 2016
@Barina
Copy link

Barina commented Sep 20, 2016

I would vote for '4. category block' I think it will make the code looks good also

@SuperUserNameMan
Copy link
Contributor

SuperUserNameMan commented Sep 20, 2016

As a workaround, and as the inspector respects the orders of my exported variables, to categorize them, i use pseudo-exported vars as separators :

export var __R_O_T_A_T_E__ = "----------";
export var unrotate_children = false;
export var rotate_speed = 60.0;
export var __C_A_T_E_G_O_R_Y__2____ = "----------";
export var blablbla = Vector2(5,5);

@hubbyist
Copy link

hubbyist commented Sep 21, 2016

I think of a fifth option by adding an "into" word:

category rotate
category foo

export into rotate var unrotate_children = false
export into foo var bar = false
export into rotate var baz = false

also nesting can be possible:

category rotate
category foo into rotate

I am not sure about the implementation pros and cons.

@RayKoopa
Copy link
Contributor

Everything but not more keywords please. It is of good design to not introduce needless keywords. export(category="blablabla") or anything, just in the context of export, would be more useful.

@hubbyist
Copy link

hubbyist commented Sep 22, 2016

What about mixing (2) and (3). Need for retyping category names may be little discouraging

export ("rotate", float) var rotate_speed = 60
export ("rotate/limits") var unrotate_children = false
export ("rotate/limits/axis/z") var max = pi 
export ("rotate/limits/axis/z") var min = 0
export ("rotate") var rotate_inside_tree = false

but sort of nested tree as in (4) may eleminate retyping need.

export ("rotate"):
    var rotate_speed = 60
    export ("limits"):
        var unrotate_children = false
        export ("axis"):
            export ("z"):
                var max = pi
                var min = 0
    var rotate_inside_tree = false

For clarity "export" may be changed with a "category" keyword which implies exporting as well.

category ("rotate"):
    var rotate_speed = 60
    category ("limits"):
        var unrotate_children = false
        category ("axis"):
            category ("z"):
                var max = pi
                var min = 0
    var rotate_inside_tree = false

OR as @RayKoopa said using category in hinting may be more simple

export (category="rotate"):
    var rotate_speed = 60
    export (category="limits"):
        var unrotate_children = false
        export (category="axis"):
            export (category="z"):
                var max = pi
                var min = 0
    var rotate_inside_tree = false

But I fear that this solution may be like torturing export hinting mechanism and coders as well. :)

@bojidar-bg
Copy link
Contributor Author

I think retyping the category name is not a really big deal...

@Zylann
Copy link
Contributor

Zylann commented Sep 22, 2016

I don't like this kind of nesting. It takes metadata to code structure, which can be confusing.
Also, typing again categories is not a big deal, because I will otherwise 1) make a specialized view if needed with an editor plugin, or 2) put variables in a class/resource if they are really many (however it's a bit more tedious currently).

Again, in other languages what I would do is something as simple as an attribute like [Category("This Category")], which is usually a standalone feature that doesn't dictates how different properties should be written.
So in GDScript I would expect something as simple as that, which is currently covered by export(), somehow.

btw, what about translation? Maybe sounds crazy, It just went into mind for a second :p

@hubbyist
Copy link

hubbyist commented Sep 22, 2016

So in the end something like

export ("category" type) var name = value

will be simple and sufficient without nesting, and with nesting

export ("category_path/category" type) var name = value

will do the trick IMO.

@akien-mga
Copy link
Member

@hubbyist Yes, but with a comma separator. There's no existing precedent for a "category" type syntax in Godot, I'd expect a parsing error.

So export("category", type) var name = value, i.e. (2). That's the most natural in GDScript, and the most flexible IMO. And Godot itself already works around having categorized stuff as "category/subcategory/parameter", so it makes perfect sense.

@henriquelalves
Copy link
Contributor

I'm assuming creating the "category" for export like export("category", type) will break some compatibility; is someone already working on it for 3.0? This would be a great feature for more complex plugin nodes.

@bojidar-bg
Copy link
Contributor Author

@henriquelalves I would break no compatibility -- since we would make the category optional.

I have some unpushed local changes concerning it, but the biggest issue with it is not the export syntax -- it is making GDScript code concerning set/get aware of categories.

@akien-mga
Copy link
Member

I guess it should be made similar to the new property grouping in the inspector for 3.0+.

@akien-mga akien-mga removed this from the 2.2 milestone Aug 8, 2017
bojidar-bg added a commit to bojidar-bg/godot that referenced this issue Aug 15, 2017
Closes godotengine#4378 via the 4th proposal, as it is easiest to implement
@nhold
Copy link
Contributor

nhold commented Dec 10, 2018

I am not sure if this exists, but wouldn't it make sense to create a new class\struct for categories of variables which would then be the category? I.e

class axis
    export var max = pi
    export var min = 0

class limit
    export(bool) var unrotate_children = false
    export(axis) var xAxis = null
    export(axis) var yAxis = null
    export(axis) var zAxis = null

class rotate
    export(int) var rotate_speed = 60
    export(bool) var rotate_inside_tree = false
    export(limit) var limits = null                

This to me feels more OOP-like, which is the feel Godot is going for I think.

@Zylann
Copy link
Contributor

Zylann commented Dec 10, 2018

@nhold while it would make sense, this is not always the best solution. Sometimes you just need to group many variables in categories but still have them in the same class, as well as their getter/setters and associated code. Having that working for subclasses would be nice, but this should not be the only way of making categories (and isn't for the editor anyways).

@nhold
Copy link
Contributor

nhold commented Dec 10, 2018

@Zylann Great point, then yeah I would personally prefer both options available(Composed classes and manual export with category string) as well as base classes(Ones we extend from) showing under their own categories similar to built in nodes.

@FrederickDesimpel
Copy link

FrederickDesimpel commented Dec 10, 2019

Is this still considered ?

While looking into this, would it also be possible to optionally add limits and other possible properties relevant to the type?

something like:

export("rotate", float, min: 0, max: 90) var rotate_speed = 60.0

would be nice if the system for this is extensible.

@FrederickDesimpel
Copy link

FrederickDesimpel commented Dec 10, 2019

could be interesting to brainstorm a list of possible 'export arguments'.

like the former, there could be a ui-min and hard-min, to suggest a min value but still possible to override until hard-min.

Not sure if exporting user defined types are supported, have not tried that yet... , possibly to restrict what you can set as a nodepath for example ? That would be a hard one , ie a specific sub type allowed, or a list of allowed / disallowed types.

@bojidar-bg
Copy link
Contributor Author

bojidar-bg commented Dec 10, 2019

@FrederickDesimpel Reading through the GDScript export docs might be helpful -- min/max hints for floats are already supported (and ui-min/max would be through something like #32935).

Also, if brainstorming, please do the brainstorming in chat and then open an issue on the godotengine/godot-proposals repository -- we wouldn't want this issue to have too many off-topic comments.

@ChronoDK
Copy link

As a workaround, and as the inspector respects the orders of my exported variables, to categorize them, i use pseudo-exported vars as separators :

export var __R_O_T_A_T_E__ = "----------";
export var unrotate_children = false;
export var rotate_speed = 60.0;
export var __C_A_T_E_G_O_R_Y__2____ = "----------";
export var blablbla = Vector2(5,5);

Sadly this is actually the only easy/practical way to group export vars.

@AaronRecord
Copy link
Contributor

AaronRecord commented May 14, 2021

This should probably be closed in favor of godotengine/godot-proposals#1255

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

Successfully merging a pull request may close this issue.