godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.26k stars 101 forks source link

Allow instantiating scenes without any scripts #12647

Open ryevdokimov opened 2 weeks ago

ryevdokimov commented 2 weeks ago

Describe the project you are working on

https://github.com/Open-Industry-Project/Open-Industry-Project

and

https://github.com/4d49/scene-library (specifically for generating thumbnails)

Describe the problem or limitation you are having in your project

Generating thumbnails is tricky, and one of the things that makes it less problematic is disabling tool scripts when they are instantiated in their own viewport for generation. This proposal has the added benefit of allowing users to generate decorative version of scenes with scripts in the editor or during runtime.

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

Add a flag to the instantiate method, to allow disabling scripts.

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

https://github.com/godotengine/godot/pull/107817

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

Theoretically you can traverse the scene and strip the scripts or disabling processing, but I don't believe that is either performant or reliable.

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

This is core.

Bromeon commented 2 weeks ago

Generating thumbnails is tricky, and one of the things that makes it less problematic is disabling tool scripts when they are instantiated in their own viewport for generation. This proposal has the added benefit of allowing users to generate decorative version of scenes with scripts in the editor or during runtime.

Is "disable all scripts" a good approach to handle this though? Some scripts are required for visual representiation, to the point that a scene becomes non-functional without them.


Theoretically you can traverse the scene and strip the scripts or disabling processing, but I don't believe that is either performant or reliable.

It seems to me that it heavily depends on the scene setup, to know whether it's meaningful to run the scene without scripts, run it without some scripts, or with scripts in a different configuration.

Iterating scripts is one way, but the scripts themselves could query some configuration upon their creation, and act accordingly. This seems much more flexible than the suggestion here, because you cannot just turn scripts on/off, but tweak behavior precisely depending on needs.


Another point, this reserves the second parameter of PackedScene::instantiate for a very specific feature. instantiate as such a central function in the API might see the need to be extended in the future to support other functionality (e.g. passing arguments to root constructor), and it's not clear whether flags would not clash with such an idea. We should be 100% sure about the API design here, as compatibility will need to be maintained for an indefinite amount of time.

ydeltastar commented 2 weeks ago

Is "disable all scripts" a good approach to handle this though? Some scripts are required for visual representiation, to the point that a scene becomes non-functional without them.

Custom scripts cause issues since we don't know what they will do. We usually want an instant, pure built-in nodes representation of the scene for thumbnail generation. Also, something that can't be solved by traversing after instantiation are script methods that run immediately like _init() or _static_init().

Just disabling scripts is not the best solution, though. In my SceneTexture extension, I had to hack out anything that wasn't a VisualInstance3D. It is 3D exclusive so 2D nodes are unnecessary, but most importantly, some nodes do things on their own just like scripts. Audio nodes, for example, can autoplay when instantiated.

A better approach is what https://github.com/godotengine/godot/pull/102313 did (relevant part below). It directly modifies the PackedScene using SceneState. Unfortunately, we can only retrieve SceneState from the scripting API. There is no exposed way to modify it before instantiating a scene.

Relevant code from the PR ```cpp bool EditorPackedScenePreviewPlugin::_setup_packed_scene(Ref p_pack) const { // Refer to SceneState in packed_scene.cpp to see how PackedScene is managed underhood. // Sanitize Dictionary bundle = p_pack->get_state()->get_bundled_scene(); ERR_FAIL_COND_V(!bundle.has("names"), false); ERR_FAIL_COND_V(!bundle.has("variants"), false); ERR_FAIL_COND_V(!bundle.has("node_count"), false); ERR_FAIL_COND_V(!bundle.has("nodes"), false); ERR_FAIL_COND_V(!bundle.has("conn_count"), false); ERR_FAIL_COND_V(!bundle.has("conns"), false); const uint8_t supported_version = 3; uint8_t current_version = bundle.get("version", 1); if (current_version > supported_version) { WARN_PRINT_ONCE(vformat("Scene thumbnail creation was built upon PackedScene with version %d, but the version has changed to %d now.", supported_version, current_version)); // And assume it's safe to continue, there should have no reason to change the main structure of PackedScene } // Find and remove variants in scene const Ref Githubissues.
  • Githubissues is a development platform for aggregating issues.