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 TypeDB. Refactor Custom Types, CreateDialog. #27566

Closed

Conversation

willnationsdev
Copy link
Contributor

@willnationsdev willnationsdev commented Mar 31, 2019

  • Create a lightweight editor-only TypeDB mimicry of ClassDB which is aware of inheritance data pertaining to engine classes, script classes, and custom types.
  • Refactor custom types to be indexed by name. This greatly speeds up many operations associated with them.
  • The CreateDialog has undergone numerous optimizations to improve its performance, as well as simplifying the code overall to avoid handling special cases for the type blacklist and the various handling logic for engine classes, script classes, and custom types using the TypeDB as an agnostic wrapper around type names.
    • All CreateDialogs now update their data automatically in response to "filesystem_changed" signal emissions. They do not waste time updating their type list when opened, nor when the search filter is updated.
    • Class-specific exceptions used to be applied during the update search operations. Now the CreateDialog performs these checks when it updates its data. These include...
      • The type blacklist (types that are not allowed to be instantiated from the CreateDialog)
      • Types for which ClassDB::can_instance(type) returns false.
      • Nodes with "Editor" in the first part of their name.
    • Previously, the CreateDialog would check inheritance hierarchies at least twice for all type names, regardless of whether their name is a subsequence match to the search filter's TextEdit content. Now, inheritance checks during Tree building are only performed on types that are a match.
    • The inheritance checks made at runtime are now purely HashMap lookups in the cached inheritance data of the TypeDB. It no longer needs to load scripts and iterate through their base scripts whilst building the tree.

Closes #27333, #24557, #24041 (Issues). Closes #25675 and #25676 (PRs).

@willnationsdev willnationsdev force-pushed the create-refactor branch 2 times, most recently from ee8ef6a to 1a20eef Compare April 1, 2019 02:11
@Chaosus Chaosus added this to the 3.2 milestone Apr 1, 2019
@reduz
Copy link
Member

reduz commented Apr 4, 2019

I am not completely against abstracting this because the logic for the create dialog could be used also in the resourece editor. However, it should be a very simple function to abstract parentship and it should never hide that the typet comes from a script. So, what is needed is not even close to a db.

@willnationsdev
Copy link
Contributor Author

@reduz well, the reason I called it a DB at all was because the entire purpose of it was to cache what the inheritance relationships are, to prevent us from having to recompute which named types (script or engine class) are inheriting which. It is precisely attempting to figure all this stuff out at runtime that causes the CreateDialog to lag so badly when you add more and more script classes. Because I'm caching the information in this class in EditorData, there is no need to recompute inheritance hierarchies for every class every time the Tree in CreateDialog updates. Instead it simply looks up names in a HashMap. That's the only reason I made it a "DB" in the first place. When I was testing things, going with the traditional "if it's a script, check for script inheritance stuff" at runtime wasn't cutting it.

@willnationsdev
Copy link
Contributor Author

willnationsdev commented Apr 4, 2019

Also, custom types currently display themselves in the CreateDialog with just the name, no filename next to them. This change causes them to display themselves just like script classes, so it actually removes ambiguity between scripted types and engine types further.

Every record in the TypeDB has a path to the script matched with it. Engine types will have an empty path, so it is still very easy to check whether a given name belongs to a script vs. an engine class. I don't believe there is a lack of clarity on the part of the user here.

@willnationsdev
Copy link
Contributor Author

willnationsdev commented Apr 4, 2019

@reduz

it should never hide that the typet comes from a script.

Are you suggesting that I just update the method names to have specific tdb.get_base_script() and tdb.get_base_class() methods, to force the user to make the distinction if they work with the TypeDB? That would still cache the inheritance data, but add even more forced recognition of the type of data they are messing with.

@willnationsdev willnationsdev force-pushed the create-refactor branch 2 times, most recently from ae78c3e to 73638cc Compare April 4, 2019 19:41
@willnationsdev
Copy link
Contributor Author

Or maybe, reduz, you'd prefer that I make the TypeDB thing only contain information about the scripted types, so that it gives the appearance of not knowing anything about the C++ classes, i.e. make it a ScriptDB or something?

@reduz
Copy link
Member

reduz commented Apr 21, 2019

It does not need to be a DB because you don't need to cache anything, this is not a performance problem at the moment, but more of a "Just give me anything that inherits from this, including custom types" function.

@HeartoLazor
Copy link
Contributor

Could this be cherrypicked for 3.1.2?
I tested it with 400+ class_names and without this patch the create dialog is unusable.
before:
https://gfycat.com/miserlypointlessbarb
after:
https://gfycat.com/wastefulharmlessislandcanary

Also the script loading for the database at start of the editor isn't so friendly with autoload classes, preload and some scripts, here are some of the error logs in my project at start with this patch:

ERROR: Resource: 'res://resources/globals/utils/utils.tscn' is already being loaded. Cyclic reference?
   At: core\io\resource_loader.cpp:363
SCRIPT ERROR: GDScript::reload: Parse Error: Identifier 'save_manager' is not declared in the current scope.
          At: res://resources/globals/input_manager/input_manager.gd:462
ERROR: GDScript::reload: Method/Function Failed, returning: ERR_PARSE_ERROR
   At: modules\gdscript\gdscript.cpp:580
ERROR: CanvasItem::get_global_transform: Condition ' !is_inside_tree() ' is true. returned: get_transform()
   At: scene\2d\canvas_item.cpp:478
ERROR: Node::get_tree: Condition ' !data.tree ' is true. returned: 0
   At: C:\Git\godot\source\scene/main/node.h:263
SCRIPT ERROR: GDScript::reload: Parse Error: Could not fully preload the script, possible cyclic reference or compilation error. Use 'load()' instead if a cyclic reference is intended.
ERROR: ScriptServer::get_global_class_path: Condition ' !global_classes.has(p_class) ' is true. returned: String()
   At: core\script_language.cpp:185

utils and save_manager are autoload classes

@willnationsdev
Copy link
Contributor Author

@HeartoLazor Thanks for the visual demonstration! I will try to look into the issues you brought up when I get a chance. I have a few PRs that need work and not a lot of time, but I'll switch to prioritizing this one.

@ghost
Copy link

ghost commented May 1, 2019

Well hopefully things get resolved. I do recall when I tried using class_name months ago, one of the reasons I had to abandon it was how cluttered and slow the add node dialog became. Didn't want every class in the list, but it mainly became more and more delayed with each new entry.

@HeartoLazor
Copy link
Contributor

@willnationsdev Hey here is a minimal test project with all the errors, those errors only happens with this patch. Tested using as base 3.1.1 stable, and some of them happens when I press play too.
test_new_add_dialog.zip
const referenced between class_names classes and addons are the main culprits.
Also there are another issues too:

  • Missing addons in the create dialog.
    3.1.1 without patch
    H2CTt9w
    3.1.1 with patch
    cBkFjky

  • Missing icons in scene
    3.1.1 without patch
    HI5pNX8
    3.1.1 with patch
    jkpfNgU

@akien-mga akien-mga requested a review from reduz May 28, 2019 10:43
@aaronfranke
Copy link
Member

Does this have to do with custom types defined via add_custom_type() or with class_name in scripts? What are the user-facing changes to custom types and the create dialog?

@willnationsdev
Copy link
Contributor Author

@aaronfranke There are no user-facing changes to custom types involved here (no changes to the API). The backend implementation is just swapped to index custom type records by their class name (to easily pinpoint single records or look up parents) rather than by checking all the direct descendants of engine classes. This change also enables custom types to extend each other rather than only have a single level of inheritance from engine types.

For the CreateDialog, aside from potentially seeing custom types inherit each other, there are no visual differences that users will see; only the performance of the dialog itself is improved (so no more lag).

This has to do with both custom types and class names. Strictly speaking, when you have many user-defined types with names defined, the CreateDialog becomes too slow, so this contains changes to how the data is organized and cached to ensure that the user experience is smoother.

@Xrayez
Copy link
Contributor

Xrayez commented Aug 11, 2019

Any progress on this? I'm experiencing the same performance issue, I'm considering unexposing some of the classes but I'd eventually end up with @HeartoLazor's situation anyway.

@willnationsdev
Copy link
Contributor Author

There has not been much progress on this, but I plan on returning to this issue very soon.

@akien-mga
Copy link
Member

What's the status on this PR? Many of the issues it referenced are now closed, and I remember @reduz was not keen on adding a TypeDB API. I'm not saying this is not a good solution though, but we need to clarify what are the exact problems that this aims to solve in the current master branch, provided that they are still relevant.

@akien-mga akien-mga requested a review from vnen November 7, 2019 08:32
@willnationsdev
Copy link
Contributor Author

@akien-mga Most of this content is planned to be re-implemented entirely differently to fall in line with godotengine/godot-proposals#22 and godotengine/godot-proposals#68, some of godotengine/godot-proposals#43, etc. So this can really be closed as it will never be merged.

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.

Script classes make the CreateDialog search filter prohibitively slow
7 participants