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 ClassType wrapper and Editor SceneTemplates #22181

Closed

Conversation

willnationsdev
Copy link
Contributor

@willnationsdev willnationsdev commented Sep 17, 2018

Closes #21187, #21203, and #22181.

This PR adds a "SceneTemplates" tab to the ProjectSettings window. This allows you to assign specific names to scenes, allowing them to then show up in the CreateDialog.

Because the logic of tracking inheritance hierarchies between engine classes, script classes, and scene templates was becoming quite complicated, I wrapped all of the associated logic into a ClassType class in the new class_type module. This will also enable us in the future to add class-name features to other parts of the engine (for example, exposing a class name string that must inherit from a particular class and provides enumerated options in the Inspector).

Adding scenes to the Scene dock in this manner executes more or less the same logic as using the chain-link icon.

You can also prefill a new entry in the SceneTemplates tab by using context menus in the Scene dock (for nodes that represent a scene) and in the FileSystem dock (for scene files).

These "SceneTemplates" are only accessible from the editor at editor-time. That is, these are NOT runtime-accessible typenames like script classes are.

(Note that in the below examples, "MySprite" is a scene with a Node2D root and a child Sprite)

SceneTemplates tab:
scene_template_settings

CreateDialog entry:
scn_temp_search

Adding a root "scene" node, i.e. inheriting a scene:
scn_temp_inherited_scene

Adding a child "scene" node, i.e. instancing a scene:
scn_temp_instanced_scene

Context menu in Scene dock:
context_menu_scene

Context menu in FileSystem dock:
context_menu_filesystem

Prefilled value:
settings_populated

Let me know what you guys think.

@Piet-G
Copy link
Contributor

Piet-G commented Sep 17, 2018

Wish this could've been in 3.1.

And yeah that class_type module was really needed.

@willnationsdev
Copy link
Contributor Author

@dualmatrix Yeah, I'm hoping we'll be able to cherry-pick the module and the SceneTemplates into 3.1.1.

@Piet-G
Copy link
Contributor

Piet-G commented Sep 17, 2018

I'm all for at least getting that module in 3.1 i've seen the code it is supposed to replace and eww, didn't look good.

@willnationsdev
Copy link
Contributor Author

@dualmatrix Well, if we were to merge it into 3.1 without the SceneTemplates, I'd have to go in and remove all the "scene template" code first. XD

@PLyczkowski
Copy link

This needs an ability to add built-in script to the instanced scene that extends SceneTemplate script - either via checkbox when adding the scene, or just after with "Extend Script" scene tree context menu option.

@PLyczkowski
Copy link

^ Related to my comment - #17368

@willnationsdev
Copy link
Contributor Author

@PLyczkowski As discussed in the mentioned Issue, the existing script class editor changes and this PR together will enable a comfortable enough workflow. I think expecting people to make the root node of a scene template be a non-built-in script (so that they can extend an actual file or script class) is a reasonable expectation. Therefore, I don't think we need to add any further functionality beyond what has already been detailed.

if (StringName() == base)
return NULL;
Object *obj = ClassDB::instance(base);
obj->set_script(script.get_ref_ptr());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to return the obj here.

@willnationsdev
Copy link
Contributor Author

Since #25675 has been slated for 3.2, I will probably just merge it and #25690, which depends on it, into this PR since they both involve making simplifications and utilities for the CustomType API.

@willnationsdev
Copy link
Contributor Author

Going to refactor this so that ClassType is no longer an integrated portion of the implementation. I'll probably move a large amount of its code into the EditorData class so that I can still use its utility methods in the CreateDialog though (which may also prove useful for evaluating resource inheritance relationships in other parts of the editor I suspect). I am still going to have a ClassType script in my godot-next plugin though, for those interested.

This coming as a result of a discussion with reduz and Akien on IRC chat. For reference:

[12:24] <reduz> willnationsdev: as I told you a lot of times, I think these things are fine the way they are, explicit is better.
[12:25] <reduz> willnationsdev: hiding these things under the carpet is shameful, there is no need to make things transparent when they are not, one needs to accept living with inconsistency.
[12:25] <reduz> willnationsdev: users are not fools,they don' t need to be hidden how things work.
...
[13:13] <Akien> It's the same as always. There needs to be real use cases that solve actual issues before any such massive change can be added to the core.
[13:13] <Akien> Writing a whole abstraction layer for the purpose of avoiding a couple if checks in some plugin's boilerplate is no go.
[13:14] <Akien> If there's a real use case that would fix issues that people are having, then we can definitely discuss how to solve it, which can be your proposal.
[13:14] <Akien> But it only works in that way. We can't start from an abstraction proposal and then try to find where it would be useful.
[13:15] <Akien> And typically if the change adds 100 LOCs to core to remove 20 LOCs in an editor-only plugin like CreateDialog, that's not going to be accepted.

@willnationsdev
Copy link
Contributor Author

Based on the discussion in #21187, there is no need to continue working on this. Anything that can assist both the editor (can't be a module, must be in core or editor) and runtime (can't be in editor) would have to be in core, and any and all core changes that would enable a user-defined type system will not be approved by reduz.

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.

4 participants