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

Resource's Local to Scene is counter-intuitive and should be improved or removed #1848

Open
gcardozo123 opened this issue Nov 16, 2020 · 12 comments

Comments

@gcardozo123
Copy link

Describe the project you are working on:
A basic 2D project.

Describe the problem or limitation you are having in your project:
Everytime I use "Instance Child Scene" and try to customize something on that "instance" I have to be super careful to make sure every customizable resource has "Local to Scene" checked, otherwise I end up messing the original scene. Since you're supposedly creating an instance of a scene, I feel that having to check every "Local to Scene" resource is counter intuitive. Why "Local to Scene" true isn't the default?

Describe the feature / enhancement and how it helps to overcome the problem or limitation:
I feel that "Local to Scene" true should be the default and I imagine there's a pre-existing reason to not do that. Maybe it would duplicate the resources? If that's the case, why not cache the original scene resources so that they can be re-used on instances that don't customize those resources?
There might be a way to completely remove the "Local to Scene" checkbox and let the engine figure out the optimal situation by itself.
I also feel that customizing an instance should never modify the original scene. Both the instance and the original scene could have an "Apply changes to all instances" button, other option would be letting changes on the original scene be completely ignored by pre-existing instances (because they can be customized instances).

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:
Summing up, speaking about Godot's editor:

  • Remove the "Local to Scene" checkbox, the editor should decide that by itself;
  • Instanced scenes modifications should never affect the original scene;
  • Changes on the original scene should be only applied to instances via an "Apply changes to: " button, which could have options "all instances" or "non customized instances" button.

If this enhancement will not be used often, can it be worked around with a few lines of script?:
Can't be worked around without modifying the editor behaviour.

Is there a reason why this should be core and not an add-on in the asset library?:
Yes, after looking at the community forum and reddit I would say that the new behaviour is more intuitive.

@YuriSizov
Copy link
Contributor

YuriSizov commented Nov 17, 2020

I am confused as to what kind of changes and modifications you do to your instanced scenes, which are affected by Local to Scene. Can you give a more concrete example?

@Calinou Calinou changed the title [Godot Editor] Local to Scene is counter intuitive and should be improved or removed Resource's Local to Scene is counter-intuitive and should be improved or removed Nov 17, 2020
@Calinou
Copy link
Member

Calinou commented Nov 17, 2020

Remove the "Local to Scene" checkbox, the editor should decide that by itself;

Unfortunately, there's no way for the engine to figure that out by itself. (PS: Local to Scene is implemented in the core engine, not the editor.)

If you duplicated resources for everything, you'd be wasting CPU cycles and memory on that. Also, sometimes, you actually want your changes to be shared across instances.

@gcardozo123
Copy link
Author

Hey guys, thank you for your quick feedback, I really appreciate it! 😃

I am confused as to what kind of changes and modifications you do to your instanced scenes, which are affected by Local to Scene. Can you give a more concrete example?

Yes, I'm speaking about changes on resources, for example, the Extents on a CollisionShape2D -> Shape. The use case that I was testing is basically creating a blueprint scene that has a Sprite, an Area2D with its CollisionShape2D and some script. I want to use that scene as a blueprint/prefab so that I can instantiate it (via Godot Editor) and customize it changing its texture, its CollisionShape2D Extents, etc. There are two things that I find annoying:

  1. After instanciating the scene I have to right click on its node and check "Editable Children";
  2. If I forget to check "Local to Scene" on the original scene resources I end up messing them when customizing an instance of that scene, and since it is an instance of that scene I find the "Local to Scene" approach counter intuitive. I feel that changes on instances should never affect their blueprints (original scene).

@gcardozo123
Copy link
Author

gcardozo123 commented Nov 17, 2020

Unfortunately, there's no way for the engine to figure that out by itself. (PS: Local to Scene is implemented in the core engine, not the editor.)

I see, I'm saying "Godot Editor" all the time just to make sure you guys don't think I'm speaking about runtime behaviour.

If you duplicated resources for everything, you'd be wasting CPU cycles and memory on that.

Would it be possible to the following?

  1. Remove the "Local to Scene" checkbox from the editor;
  2. In the editor: when creating an instance of that scene, don't duplicate the resources, but if the user modifies resources that would affect the blueprint (original scene) then, only at that moment, the necessary resources would be duplicated (again, so that they don't affect the original scene).
  3. Not sure if that's the case, but it seems that the "Editable Children" checkbox (right click an instance of a scene) only exists to prevent users from messing the original scene while customizing an instance. If that's the case, with the changes that I'm proposing it would be possible to remove the "Editable Children" checkbox and let it be always editable.

Also, sometimes, you actually want your changes to be shared across instances.

As a programmer I never expect that a change on an instance will modify its blueprint. The other way around is ok: if I change a default value on a blueprint, I expect that only instances that didn't modify the default value will be updated.

@Zireael07
Copy link

There are many cases where duplicating resources on modifying would not be desired, mostly for performance reasons (eg. collision shapes, materials, shaders)

@YuriSizov
Copy link
Contributor

YuriSizov commented Nov 17, 2020

@gcardozo123 I see. I personally have never found a use-case for editable children mode in Godot yet.

I think you can make this work if you start exposing the parts that you want to change via exported script variables on the root node of your instanced scenes. Think about it like a blackbox package. You create a scene that does the basic thing and is configurable, but to configure it you only need to change public-facing properties. You don't need to go into the implementation or even be aware of it. This way your "blueprint" scene is safely isolated, and you still can edit everything via the inspector dock.

As a programmer I never expect that a change on an instance will modify its blueprint.

The problem is similar to "by reference" vs "by value". Look at it this way: resources are by default referenced when instancing PackedScenes, unless you make them local, so each instance has it's own copy based on the default value.

@Calinou
Copy link
Member

Calinou commented Nov 17, 2020

I personally have never found a use-case for editable children mode in Godot yet.

Editable Children will likely become more useful (or even a necessity in some cases) if #1823 is implemented.

@Zireael07
Copy link

@pycbouh: The first use-case for editable children I have is a card game. I have a single card scene that is a template - then when I need different cards, I just modify the base card scene by using editable children. It's faster than changing variables from script and I can just see the changes at a glance. Changing things via exported script variables on root node can break (e.g. if you move children nodes around), especially in conjunction with setget. Also, if your scene is complex it'll make the list of exported variables on root node HUUGE - as in, so huge that you need to scroll several times to see non-exported vars.
Similarly, I have a basic planet scene that I use editable children on if I want this one instance to be somehow special.

@YuriSizov
Copy link
Contributor

@Zireael07

Changing things via exported script variables on root node can break (e.g. if you move children nodes around), especially in conjunction with setget.

I don't get it. On the contrary, proper abstractions reduce the chances of breakage. You don't need to worry about what changes happen internally as long as the public interface is kept. So move your nodes however you like, instances will still work. It is the benefit of blackboxing.

Also, if your scene is complex it'll make the list of exported variables on root node HUUGE

If you need that many variables to configure your scene, you have way too many permutations which is a problem in and of itself. You shouldn't have abstractions with too many permutations, you should instead split them more into dedicated, more manageable parts. Also, groups would help here, but we don't have them yet for exported variables.

@Xrayez
Copy link
Contributor

Xrayez commented Nov 18, 2020

I think it's not enabled by default because there are things which you definitely wouldn't really want to make unique per instance in most cases. An example for this is Script, if you enable "Local To Scene" there, it potentially means that you wouldn't be able to compare different objects at run-time, leading to difficult to debug situations. I'm not even sure whether it would actually work correctly.

I certainly accept that this is indeed an issue with the mentioned CollisionShape nodes with Shape resources in them, especially if you need to change properties dynamically via code while instancing scenes.

That said, I see it as a project setting option: Set Local To Scene By Default: yes|no. Yet the engine would need to blacklist certain resources which should not be set local to scene by default, like with scripts, shaders etc.

Another issue is the "recursive" problem. Some resources are designed to work as nested, and I think in some cases this could lead to cyclic dependencies, which is a difficult limitation to resolve according to long-standing issues like godotengine/godot#21461.

Regarding counter-intuitiveness, see also godotengine/godot#16863 (comment) with suggestion of renaming this to something else.

@golddotasksquestions
Copy link

Yes, I'm speaking about changes on resources, for example, the Extents on a CollisionShape2D -> Shape. The use case that I was testing is basically creating a blueprint scene that has a Sprite, an Area2D with its CollisionShape2D and some script.

@gcardozo123 You are not alone with your confusion and frustration. How resources are shared and letting the user know they are shared is definitely something that need improvement. However I don't think a project wide check box that makes everything unique is the solution.

Also see this issue:
#317

@cyraid
Copy link

cyraid commented Oct 29, 2023

In Unity you can make a prefab variant, or just make an instance and it highlights changes in the inspector and allows you to apply specific ones to the original prefab. I think that was pretty straightforward. That and having to use scenes in place of prefabs or groups is also confusing. I'm gonna make a "template asset", okay, let me see what I should do.. oh.. I have to create a new scene and that's my model? Hmm ..

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

7 participants