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

Have class_name classes properly show in the inspector #109

Closed
Duroxxigar opened this issue Sep 28, 2019 · 4 comments · Fixed by godotengine/godot#32428
Closed

Have class_name classes properly show in the inspector #109

Duroxxigar opened this issue Sep 28, 2019 · 4 comments · Fixed by godotengine/godot#32428
Milestone

Comments

@Duroxxigar
Copy link

Duroxxigar commented Sep 28, 2019

Describe the project you are working on:
This isn't necessarily relevant to my particular project, but I'm working on a 3D FPS

Describe how this feature / enhancement will help your project:
It will provide a more cohesive flow in the inspector when using class_name and then extending from that newly created class.

Show a mock up screenshots/video or a flow diagram explaining how your proposal will work:
My base Weapon class. Take note of the exported variables.
image

Example class that extends my custom class
image

Notice how this exported variable gets shoved in to the inspector with the class that it inherits from.

Describe implementation detail for your proposal (in code), if possible:
image

I propose that it should follow the built in inspector hierarchy. So, if I put my Weapon class on this standard KinematicBody node, above the KinematicBody node, should be the Weapon class. Then, when I create a new class that extends from the Weapon class and put it on this same node, this new class should be on top of the Weapon class in the inspector. This would mean that the inspector for custom classes wouldn't just read the class as "Script Variables"

If this enhancement will not be used often, can it be worked around with a few lines of script?:
This enhancement will be used all the time as it affects a keyword for GDScript and the way the engine would handle it in the inspector

Is there a reason why this should be core and not an add-on in the asset library?:
Because class_name and the inspector are core.

@willnationsdev
Copy link
Contributor

For the community's reference, I at one point attempted an implementation of this in this PR, but it was, at the time, closed because the design of the implementation wasn't very good. It directly changed Object in the core rather than changing the EditorInspector code like it should. The implementation could probably be greatly simplified too.

There was, at the time, no concept of script classes, so the editor simply went off the file name to decide on a name. Now, however, there is an editor-aware "name" that can be assigned to scripts. And when C# gets script class support (#22), then it will just work out-of-the-box for C# (and other languages).

Some people wanted script annotations to be a thing and for them to allow you to associate properties with a particular category or group. I still think that's a great idea, but we should do it on top of doing this since this makes the script system more consistent with the engine side anyway.

@willnationsdev
Copy link
Contributor

willnationsdev commented Sep 29, 2019

Something that needs to be accounted for is what to do about properties that belong to intermediate scripts. For example:

  • derived_c.gd (class_name DerivedC)
  • derived_b.gd (unnamed) <- where do these properties go? What does it look like?
  • derived_a.gd (class_name DerivedA)
  • Node

My first thought is that there should be a "Script Variables" section at the top that bundles up all the rest of the properties that aren't directly associated with a script.

However, that wouldn't indicate the order of inheritance in detail.

My second thought is to create a new category for every script and just use the actual file name as the category text (not in Title Case) when it isn't a script class. That might make for strange UI though, unless you also converted the file name to Title Case and dropped off the file extension.

Thoughts?

@Duroxxigar
Copy link
Author

@willnationsdev I didn't necessarily have a good idea about that one, but using the file name should work in my opinion. One of the purposes of that hierarchy is to show the inheritance tree, so, we should be consistent.

I'm not sure why you think it wouldn't indicate the order of inheritance though. I believe it would. Using your example...

  • DerivedC
  • derived_b.gd
  • DerivedA
  • Node

It will be a bit jarring to see an class on the hierarchy, but I think it is important to see the file name with the appended .gd so one could tell that it isn't a named class and should be able to tell exactly which .gd file it is. (Though, without the file type, one may still be able to tell)

@willnationsdev
Copy link
Contributor

@Duroxxigar

I'm not sure why you think it wouldn't indicate the order of inheritance though.

I was thinking of a scenario where the script class properties displayed in their own categories and then any property that didn't directly belong to a script class would be shuffled up to the top under the "Script Variables" header.

It will be a bit jarring to see an class on the hierarchy, but I think it is important to see the file name with the appended .gd so one could tell that it isn't a named class and should be able to tell exactly which .gd file it is.

This is what I decided to go with, after your and groud's feedback. The PR is ready to be merged I believe, barring any more recommended changes / CI issues.

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.

4 participants