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

Expose static ScriptPath for scripts in C# for 4.0 #5238

Closed
markdibarry opened this issue Aug 23, 2022 · 3 comments
Closed

Expose static ScriptPath for scripts in C# for 4.0 #5238

markdibarry opened this issue Aug 23, 2022 · 3 comments

Comments

@markdibarry
Copy link

markdibarry commented Aug 23, 2022

Describe the project you are working on

A systems test project for Godot 4.0.

Describe the problem or limitation you are having in your project

I was previously looking for a more dynamic way to instantiate scenes in code than using hardcoded strings, and less brittle than editor exports. For my current solution, I added the following static method in my extensions class to get the script's file path, and then modify it to get the scene's file path:

public static string GetScenePath([CallerFilePath] string csPath = "")
{
    string godotRoot = Config.GodotRoot;
    if (!csPath.EndsWith(".cs"))
        throw new Exception($"Caller '{csPath}' is not cs file.");
    if (!csPath.StartsWith(godotRoot))
        throw new Exception($"Caller '{csPath}' is outside '{godotRoot}'.");

    string resPath = csPath[godotRoot.Length..];
    resPath = Path.ChangeExtension(resPath, ".tscn");
    resPath = resPath.TrimStart('/', '\\').Replace("\\", "/");
    return $"res://{resPath}";
}

In order for this to work, I need to add this method to every script:

public static string GetScenePath() => GDEx.GetScenePath();

Then I can call it like so:

var titleMenuScene = GD.Load<PackedScene>(TitleMenu.GetScenePath());
var titleMenu = titleMenuScene.Instantiate<TitleMenu>();

or even shorter:

var titleMenu = Extensions.Instantiate<TitleMenu>(TitleMenu.GetScenePath());

Pros:

  • No extra editor setup
  • No fear of data loss from exports
  • No magic strings

Cons:

  • Must copy paste a one line method at the top of every script
  • Only works if script and scene named the same and in same folder
  • Requires writing class type twice in same line to instantiate.
  • Uses reflection.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

I noticed awhile ago we added the ScriptPathAttribute via Source Generators:
godotengine/godot#46713
It seems we already get the file path, but can't access it. If it were able to be generated as a constant or static string on each class, I'd be able to construct the scene's file path:
without magic strings
without adding extra methods to each script
without using exports
without editor setup
without redundant calls
without reflection

If any of this is wrong, please feel free to correct me.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

I'm not the most savvy with Source Generators, but along with the attribute, we'd also just create a static or constant string of the ScriptPath.

Then anyone would be able to implement their own extension method, to accommodate their personal organization structure, and call it like so:

Extensions.Instantiate<TitleMenu>();

If this enhancement will not be used often, can it be worked around with a few lines of script?

Unfortunately, it cannot... or at least only with reflection.

Is there a reason why this should be core and not an add-on in the asset library?

I'm not suggesting a full feature, rather just an extra field. I think if this were a full feature to instantiate scenes, requiring a rigid organization structure, this would be better suited as an add-on. I think since this is only exposing one field, it's much more flexible for use in anyone's personal organization system, and possibly for other use cases.

@markdibarry
Copy link
Author

Though, the more I think about it, we'd have no way to know about a constant/static property on a class passed into an extension method, unless it was on all Godot.Objects. Perhaps add a static property and just assign it via Source Gen?

@raulsntos
Copy link
Member

No fear of data loss from exports

I'm not sure what you mean but this sounds like a bug that should be fixed rather than the intended behavior of instantiating scenes using exports.

Must copy paste a one line method at the top of every script

You could use source generators yourself to generate a property that is exposed or even create a static class that contains a reference to all the scene paths (instead of the scripts), similar to how the R class works in Android.

Only works if script and scene named the same and in same folder

This would still be the case since ScriptPath contains the path to the script and it seems you want the path to the scene (.tscn file) but a script and a scene are not a 1 to 1 relation (see #1909 and #1935).

Requires writing class type twice in same line to instantiate.

I don't see how having a ScriptPath property exposed would fix this. You would still need to use the type to access the ScriptPath property and then again after instantiating the scene to cast the Node instance.

Uses reflection.

The sample you provided is not using reflection so I'm not sure what you mean. However, you could use reflection to access the value of the ScriptPathAttribute that is added to your script classes.

Though, the more I think about it, we'd have no way to know about a constant/static property on a class passed into an extension method, unless it was on all Godot.Objects. Perhaps add a static property and just assign it via Source Gen?

You can't access static properties or constants of a class in a generic context, the only way to do this would be using an interface with static virtual methods which is only available in .NET 7.0 (not released yet, will be released in November).


Here is another way of doing what you are doing in your sample but using reflection and the ScriptPathAttribute. This method avoids repeating the class type twice and adding a one line method at the top of every script but it still requires the script and the scene files to be in the same directory and have the same name.

using System;
using System.IO;
using System.Reflection;
using Godot;

public static class SceneInstantiater
{
	public static T Instantiate<T>() where T : class
	{
		var type = typeof(T);
		var attr = type.GetCustomAttribute<ScriptPathAttribute>();
		if (attr == null)
			throw new InvalidOperationException($"Type '{type}' does not have a ScriptPathAttribute.");

		var path = Path.ChangeExtension(attr.Path, ".tscn");
		var scene = GD.Load<PackedScene>(path);
		return scene.Instantiate<T>();
	}
}

To avoid reflection altogether you could, like I mentioned above, implement a source generator that exposes your own ScriptPath property or, even better, that adds an Instantiate method to every class which already returns an instance of the right type but this would still require the scene and script files to be in the same directory and have the same name since, like I said, scripts and scenes are not a 1 to 1 relation so you'd have no way to know which scene to use.

Alternatively, you can instead generate something similar to the R class in Android like I described earlier which might be better for what you want.

@markdibarry
Copy link
Author

I'm not sure what you mean but this sounds like a bug that should be fixed rather than the intended behavior of instantiating scenes using exports.

I think this conversation is outside the scope of this proposal, but there are plenty of bugs that can cause it, however the root of it is the design. The editor has access to modify .tscn files and all inherited .tscn files. The reactive approach is reverting your git branch. I usually work with large projects, and have had a large number of scenes get corrupted or lose data from either updates (expected) or needing to modify the base scene. This isn't an easy thing to fix, because by design if there's some bad code in the inheritance chain between two .cs files, it throws an exception. If there's a problem in the inheritance chain of two .tscn files, it will (in some situations) try to correct it, sometimes by removing data, sometimes silently. It can be fixed with some search/replace if you notice the problem before resaving, but I prefer to just not have to hunt around and avoid the problem altogether if possible.

You could use source generators yourself to generate a property that is exposed or even create a static class that contains a reference to all the scene paths (instead of the scripts), similar to how the R class works in Android.

This is interesting and I haven't learned enough about source generators enough to utilize this, but I'm planning to spend some time this weekend to learn more.

This would still be the case since ScriptPath contains the path to the script and it seems you want the path to the scene (.tscn file) but a script and a scene are not a 1 to 1 relation (see #1909 and #1935).

What you're referencing is me talking about a drawback of my current implementation. As you say, scripts and scenes are not 1 to 1, so there is no reliable way for Godot to automatically instance a scene from a script. It has to be up to the user and their personal organization structure to make that connection. As I mentioned, the goal of this proposal is not to enforce a specific way of organizing your scenes/scripts (I think that would be a bad idea), but to allow the user a way to know where the script is, giving them a starting point, so they can make their own implementation and find scenes relative to the script.

I don't see how having a ScriptPath property exposed would fix this. You would still need to use the type to access the ScriptPath property and then again after instantiating the scene to cast the Node instance.

In my comment, I realized I hadn't thought this through entirely, unfortunately. I had forgotten that you can't access static methods or properties of a constrained generic type parameter. Looks like another avenue would be necessary.

The sample you provided is not using reflection so I'm not sure what you mean. However, you could use reflection to access the value of the ScriptPathAttribute that is added to your script classes.

I didn't realize that [CallerFilePath] happens at compile time. I thought it was using reflection. My bad!

To avoid reflection altogether you could, like I mentioned above, implement a source generator that exposes your own ScriptPath property or, even better, that adds an Instantiate method to every class which already returns an instance of the right type but this would still require the scene and script files to be in the same directory and have the same name since, like I said, scripts and scenes are not a 1 to 1 relation so you'd have no way to know which scene to use.

Just to restate: this was just to find a way to expose the script's path in a reliable way without reflection so the user can create their own Instantiate method based around their project organization. Now that I know that CallerFilePath doesn't use reflection under the hood, I think the only way I can improve on my original implementation is to automatically generate the method that calls the extension method on every script class.

I don't really see any other way to be able to make an instantiate method without two references to the class type without having to generate the specific method implementation on every script, which seems like a lot of bloat.

I'm going to wait a day and then close this unless anyone has a better solution.

@markdibarry markdibarry closed this as not planned Won't fix, can't repro, duplicate, stale Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants