godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.07k stars 69 forks source link

Add annotation to postpone loading of an @export-ed `Resource` within a `PackedScene` until instantiation. #8483

Open mrpedrobraga opened 7 months ago

mrpedrobraga commented 7 months ago

Describe the project you are working on

Many things, but mainly an RPG, which has many inter-connected rooms.

Describe the problem or limitation you are having in your project

It's common that developers will @export a variable in a script which requires a PackedScene, as a reference to another scene.

You might imagine something like a world with many rooms (and pathways in it), or perhaps scenes using nodes to represent behaviour (like code) and then having functions that call each other.

Using a supported Resource such as PackedScene would be great because we can depend on Godot's resource handling as the scene is renamed, moved, etc. It's also the best UX you'll get if you're making an addon that's PackedScene-powered.

Currently, if you have this situation:

scene1.tres
 \  Node2D (script.gd)
scene2.tres
 \  Node2D (script.gd)
# script.gd
@export target_scene: PackedScene

scene1.tres having its target_scene holding scene2.tres as a PackedScene is impossible if scene2.tres has scene1.tres as a PackedScene within. It causes a weird error message spawned in the .tres resource loader, but we can infer it's impossible since PackedScenes just load all their nested Resources into _bundled.variants.

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

My proposal is adding an annotation (mustn't be an annotation really, but this is the best scenario I came up with) to disallow an exported Resource to be preloaded when its parent PackedScene is loaded.

scene1.tres
   Node2D  (script.gd)
@no_preload
@export
var a: Resource

When scene1.tres is loaded into memory, a won't be expanded. It will only be loaded when the scene1.tres is instantiated.

Doing it this way instead of doing something like

@no_preload @export
var scene: PackedScene

is great because it allows you to fine-tune which properties don't get preloaded -- doubles down as a way to avoid preloading Resources that are problematic while allowing preloading of the last. This change also won't introduce any breaking changes.

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

Modify The Resource Loaders to check if an external resource musn't be preloaded and then not preload it.

I'm not Godot-savvy enough to know how the validity checks here work, but I believe it's possible to just keep the unloaded reference somewhat.

This also requires a change on the .tscn format to support not preloading an ext_resource, but this should be trivial.

[ext_resource type="PackedScene" path="res://other_scene.tscn" id="2_b5eg1" no_preload="yep"]

This also requires a change in PackedScene's bundled to hold unloaded resources. Currently, resources are stored in _bundled.variants as is. My favourite implementation would be to have an intermediary ResourcePointer resource which pointed to a Resource in the project without loading it -- but at this point I'm already asking for a lot; A simple wrapper or a new property in _bundled would suffice also.

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

It can, but it's highly inconvenient. It will be used often, though.

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

Proposed implementation can't be done as an add-on per se.

TheDuriel commented 7 months ago

Supporting this as quasi coauthor. I have similar needs with a Text Adventure / RPG in which Room A > Room B > Room A structures are common place. Available workarounds that preserve the full functionality of an exported packed scene, are dozens of lines of code long.

IntangibleMatter commented 7 months ago

This is probably a better way of doing it than #8484, as it's more versatile.

lemilonkh commented 7 months ago

This is a much better way of having e.g. maps reference their neighbors than keeping string references (which don't update when you rename/ move files), assuming there are back references.

Otherwise this boilerplate code is necessary for every single scene you want to reference:

https://twitter.com/the_duriel/status/1726608201309896962?s=19

Calinou commented 7 months ago

Is this similar to InstancePlaceholder, which you can obtain by right-clicking an instanced scene in the Scene tree dock and choosing Load as Placeholder?

I wonder what happens if you try to @export an InstancePlaceholder.

TheDuriel commented 7 months ago

I would assume that: The InstancePlaceholder stores a reference to a PackedScene, and that PackedScene is loaded when the PackedScene containing the InstancePlaceholder is loaded, and thus... creates an infinite cycle and breaks!

image

The editor indeed detects this, and prevents instancing scene A inside scene B when scene A contains a placeholder for scene B. Note: Within the editor, instances marked to be loaded as placeholders, are not actually replaced with placeholders.

Note that cyclical instancing of Scenes is bogus and of course will never work. And this proposal does not desire to enable such a thing.

This proposal addresses cyclic loading of PackedScenes in circumstances where these PackedScenes will not be instanced as children of another.

heck, In my personal project, neither Scene is ever added to the SceneTree after instancing.

mrpedrobraga commented 7 months ago

Is this similar to InstancePlaceholder, which you can obtain by right-clicking an instanced scene in the Scene tree dock and choosing Load as Placeholder?

I wonder what happens if you try to @export an InstancePlaceholder.

Well, it lets you export it (It's just a Node after all) and asks for you to select a node from the tree. You can't manually add it to the tree, though.

image

I also highly doubt that, if it could, it would update its resource path within whenever you rename the scene or move it (it only stores the path, and it doesn't exist in Editor time in normal use cases).

And yeah, like @TheDuriel said, the values it'll apply on the export properties are saved as Variants -- Resources are loaded.

image

TheDuriel commented 7 months ago

image Here is the full "workaround" in the form of an exportable resource that will hold the UID of a packed scene for the user...

hsandt commented 2 months ago

It seems that a circular dependency of PackedScene is causing the initial issue triggering the suggestion, so check out meta issue https://github.com/godotengine/godot/issues/80877 esp. issues related to packed scenes.

Now if the proposal also has side advantages like reducing RAM usage until needed, it could be kept even if this was fixed (and I'm not sure when circular deps issues will be fixed...)