Skip to content

Add support for custom resource types#2

Open
toasterofbread wants to merge 4 commits intonobuyukinyuu:masterfrom
toasterofbread:master
Open

Add support for custom resource types#2
toasterofbread wants to merge 4 commits intonobuyukinyuu:masterfrom
toasterofbread:master

Conversation

@toasterofbread
Copy link
Copy Markdown

Adds support for custom resource types by checking if the object has a script, then comparing the script filename to the type name stored in settings.

Example:
The type of a resource using the script res://src/SaveFile.gd would be SaveFile

@nobuyukinyuu
Copy link
Copy Markdown
Owner

nobuyukinyuu commented Mar 14, 2022

This is neat, and I should probably look into supporting custom resource types by class name and inspecting the script, but is there any reason why it should do pattern matching for a filename? Considering that this check is done before checking the resource's class_name registered class, it seems like an easy way to have other add-ons (or resources) potentially hijack the functionality or create conflicts...

@toasterofbread
Copy link
Copy Markdown
Author

This is neat, and I should probably look into supporting custom resource types by class name and inspecting the script, but is there any reason why it should do pattern matching for a filename? Considering that this check is done before checking the resource's class_name registered class, it seems like an easy way to have other add-ons (or resources) potentially hijack the functionality or create conflicts...

Forgot you could access source code from script resources, added that! Now it checks if the script is GDScript, then parses the source until it finds class_name and gets the name from that lie. Still uses the file name if no class_name is found or if the script isn't GDScript.

@nobuyukinyuu
Copy link
Copy Markdown
Owner

Searching for a class_name line prefix is not robust enough yet in this implementation; I'd recommend the following.

  1. Upon encountering a line starting with class_name, traverse until reaching the first comma or EOL. Current implementation appears it would consider an icon resource as part of the qualified name.
  2. Consider exiting the check early if a line doesn't begin with a whitelisted word (such as tool, extends or blank lines). class_name basically needs to be before any var declarations iirc.
  3. Consider not using split() to parse the entire script if 1 and 2 can be done by grabbing one line at a time from the file.

Before potentially accepting the PR, I have to consider whether the specificity offered by this new feature should override the native class name or not. I can see the benefit to caching the resource type so this whole parsing function isn't repeated on future accesses, but I'm still not sure on the overall performance impact.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants