GodotSharp.SourceGenerators icon indicating copy to clipboard operation
GodotSharp.SourceGenerators copied to clipboard

[SceneTree] does not work if the script is not part of a root scene node

Open valkyrienyanko opened this issue 1 year ago • 3 comments

Untitled

I want to access LanguageButton with [SceneTree] attribute. However I cannot because UIOptionsGeneral.cs (the script attached to the node named "General") is not a scene.

I could convert General node into a scene called UIOptionsGeneral.tscn and move the UIOptionsGeneral.cs script to be next to this scene however I would like to avoid this.

valkyrienyanko avatar Sep 22 '24 19:09 valkyrienyanko

Very true. [SceneTree] needs to parse a .tscn to populate. By default, this is done using a simple filename match, but you can specify an alternative as a property of the attribute. In this case, however, that would mean UIOptionsGeneral.cs would contain a scene tree structure from the Options node, which is not ideal, but could work.

Another option could be for the Options script to call an Init method on the General script (which should be a UIOptionsGeneral instance in the scene tree) and pass in the Language button. (If General was also uniquely named, this should be as simple as calling General.Init(LanguageButton) in OnReady).

So... To actually solve the problem for this use case, I'm thinking we could add another property to [SceneTree] to specify a root path (default '.'), so when parsed, everything outside of said root is ignored. When used in conjunction with the alternative path to .tscn property, I think we could achieve a working SceneTree solution for sub-nodes. On the other hand, the above Init approach might be easier and reduce the need for additional complexity in an already complex parsing environment.

Your thoughts?

Cat-Lips avatar Sep 23 '24 01:09 Cat-Lips

I'm not sure how to use any of the params in the [SceneTree] attribute maybe you could show examples on how to use them in the readme or if you think that will clutter the readme maybe include it in GitHubs wiki feature.

If everything above UIOptionsGeneral.cs in the scene tree is not ignored, I assume trying to get anything above would result in a error, unless this was handled with some GetParent() magic. If there are no errors on getting any node above then I think it would be fine to include everything above. (This could be added as a bool param to the attribute but it looks like the attribute already has a lot of params to begin with)

I don't think I would like doing General.Init(LanguageButton).

valkyrienyanko avatar Sep 23 '24 01:09 valkyrienyanko

I'm not sure how to use any of the params in the [SceneTree] attribute maybe you could show examples on how to use them in the readme or if you think that will clutter the readme maybe include it in GitHubs wiki feature.

Yep, always good to add more examples. The tests have some if you need it now:

If everything above UIOptionsGeneral.cs in the scene tree is not ignored, I assume trying to get anything above would result in a error, unless this was handled with some GetParent() magic. If there are no errors on getting any node above then I think it would be fine to include everything above. (This could be added as a bool param to the attribute but it looks like the attribute already has a lot of params to begin with)

Sorry, I meant ignore (skip) everything above when parsing the tscn, so the _ entry point would be the General node rather than the scene root (Options). That being said, if the full scene is sufficient, then you can achieve this already with [SceneTree("<relative_path_to>options.tscn")] (ie, doesn't recognise res://).

In the meantime, I'll create a branch to allow sub-node scripts to access SceneTree (although I can't see how to do it (yet) without having to provide a path to the tscn)

I don't think I would like doing General.Init(LanguageButton).

As a fellow artist, I can fully appreciate that ;)

(Of course, another option is to call GetNode directly in UIOptionsGeneral.cs, ... or move functionality to root script, but yeah, I get it - code bloat in root class. In the past, I've used a mix of partial classes and nested classes to try to deal with that, but as with everything, pros and cons...)

Cat-Lips avatar Sep 24 '24 02:09 Cat-Lips

I am having a similar issue in #81.

So... To actually solve the problem for this use case, I'm thinking we could add another property to [SceneTree] to specify a root path (default '.'), so when parsed, everything outside of said root is ignored. When used in conjunction with the alternative path to .tscn property, I think we could achieve a working SceneTree solution for sub-nodes. On the other hand, the above Init approach might be easier and reduce the need for additional complexity in an already complex parsing environment.

I think it could be possible to access the entire scene tree from any descendant node with this approach:

  1. Ensure via analyzer that [SceneTree] is used only on scripts whose instances all appear in the same scene (.tscn). If a script is used in multiple scenes, there is always the danger of using the generated tree structure in an unrelated scene which leads to NullReferenceExceptions at best and unexpected behavior at worst. This also removes the need for specifying a scene as it can be inferred.
  2. Parse the scene tree and create a type structure (this is already implemented)
  3. Generate the code that infers the position of the script instance in the scene relative to the scene root (not necessarily the root of the instantiated tree) when it first accesses the generated scene tree (_-property). This should always be possible if the instantiated node hierarchy is not altered, which I is a requirement for the [SceneTree] to be reliable anyways.
  4. Generate the _-property that prepends the inferred relative scene root path to the node-path created by the generated type structure.

benjiwolff avatar Oct 20 '24 22:10 benjiwolff

@valkyrienyanko @benjiwolff This is now operational and included in the new pre-release nuget package.

Cat-Lips avatar Nov 02 '24 01:11 Cat-Lips