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

Always duplicate internal subresources on node duplicate (except when it does not make sense to) #4672

Open
reduz opened this issue Jun 15, 2022 · 26 comments

Comments

@reduz
Copy link
Member

reduz commented Jun 15, 2022

Describe the project you are working on

Godot

Describe the problem or limitation you are having in your project

It happens too often that Godot users are working on something that requires subresources (like particles, particle materials, materials, etc), then when duplicating the node the fact that these subresources are shared is entirely unexpected (you modify the subresource in one node and it changes in others).

Even if expected, it's very hard to track what is referenced internal within a scene. In contrast, users understand fine that if you reference a resource saved in a file (not internal), its always the same one even if you duplicate.

Sharing stuff on copy was made for the sake of helping users make things more efficient behind their back, but I think it annoys more than it helps in the end.

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

The idea is that, on the node duplication within the editor, if the resource is internal it should be almost always copied (non referenced) and if its external (saved to file) it will be referenced. This is far more intuitive for users and should avoid surprises.

The problem with this is that there are some resource types that, even if internal, are quite expensive to have duplicated. They may consume a lot of resources for the user if they duplicate them without actually realizing that. Its the case of:

  • Meshes
  • Textures
  • Materials

So, the proposed solution for this is to have a setting in the editor settings dialog that that lists the types that by default are not copied (and users have to use the Make Unique button).

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

  • When duplicating, if the resource is not saved to disk, it will be copied.
  • If added to the "forced share" list (that comes with some types by default), they are not copied, users have to use "Make Unique" if they really want this.
  • When duplicating resources within the above list, a warning popup or stack message that can be shown for the first time (that can be later dismissed) saying "Resources of type XXX will remain shared by default because this is more optimal for the engine. If you need them to be separate, use the 'Make Unique' option in the resource menu.".

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

N/A

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

N/A

@h0lley
Copy link

h0lley commented Jun 15, 2022

I think it makes sense, but ultimately what would be the real game changer would be a better visualization of sub-resources in the editor UI.

@reduz
Copy link
Member Author

reduz commented Jun 15, 2022

@h0lley I think a popup to see this could be useful (I originally tried this in the scene tree but its very messy so I removed it. What remains is RMBing), but the main problem to solve in this proposal is resources being shared without the user noticing.

@Zireael07
Copy link

@h0lley: Related to your point, #904, but I agree this proposal here sidesteps 90% of the problem

@eon-s
Copy link

eon-s commented Jun 15, 2022

Well, this may be a source of misleading metrics when having performance issues for not reusing Resources (and not to mention how much scene files will grow this way), but if is optional and maybe with a shortcut shallow clone/duplicate at least, may be fine.

A better long term approach could be to force the creation of resources in files (scene subfolders or something like that) by default.

@reduz
Copy link
Member Author

reduz commented Jun 15, 2022

@eon-s Well, this is why we have an exclusion list of the resources that can really cause bloat if copied (and maybe a warning is shown). Encouraging the creation of resources in files is also a good idea I think, like we have for things like scripts and shaders. IMO it should be done for StandardMaterials too.

@dploeger
Copy link

I absolutely agree with this as I've had multiple facepalm events because of this as well.

I'd like to add that we should aim for consistency here. Maybe make everything duplicated per default and give the aforementioned options to speed up (with the given documentation)

It would be confusing for people if only some resources duplicate by default.

@Riteo
Copy link

Riteo commented Jun 15, 2022

I kinda agree with @dploeger. What would the duplicable list be compared to the heavy stuff?

@reduz
Copy link
Member Author

reduz commented Jun 15, 2022

@Riteo @dploeger The list is in the proposal itself.

@RandomShaper
Copy link
Member

I agree on the consistency goal. I think that after a node copy I'd have to check what has happened to the resources because the list may not be easy to interiorize. What if the list is merely for warning purposes? I mean, resources are copied by default and, when some of the types in the list is involved in a copy, a dialog tells you, 'The following resources have been copied, which are potentially heavy. Please consider sharing them to avoid bloat in scene files' and shows a list of the affected ones as node path + property + resource name/type. The dialog could also have a button to store them in files or just keep them as references to an unique instance.

@Calinou
Copy link
Member

Calinou commented Jun 15, 2022

Related to #4503.

I'd like to add that we should aim for consistency here. Maybe make everything duplicated per default and give the aforementioned options to speed up (with the given documentation)

Duplicating materials when not needed has a significant impact on performance, both in terms of shader compilation and number of draw calls. It's a bad idea to do so by default. Vulkan makes draw calls cheaper, but it doesn't make them free. Also, not everyone will be using Vulkan in Godot 4.

What we can do is print a message when a resource is kept shared on duplication because of its type (similar to something like godotengine/godot#61497).

@RandomShaper
Copy link
Member

I didn't consider materials in my comment above. @Calinou is right. My idea still stands, but reversed: share by default, but allow in the dialog to make copies of the non-heavy resources (informed by the list).

@Riteo
Copy link

Riteo commented Jun 15, 2022

@reduz I mean, how big would the duplicable (eg. the lightweight stuff) list be? If it's big enough I guess that it may be easier to accept these cases as exceptions, although I'd still argue for consistency.

@QbieShay
Copy link

QbieShay commented Jun 15, 2022

I think materials should be duplicated by default if they are spatial materials or particle materials, but shader shouldn't be duplicated.

In addition, we should probably signal that the resource is shared/linked with a little icon

@TokisanGames
Copy link

TokisanGames commented Jun 15, 2022

I don't think it's a good idea. The engine should generally be more optimal by default, not wasteful by default. I don't want materials, textures, collision shapes, meshes, shaders to duplicate themselves in memory if I'm just making another instance of them.

Unintentional duplicate materials would be the worst, as all the redundant textures will waste vram, and since the engine already has issues halting when compiling, more materials compounds the issue.

What would be a good idea is changing the UI so shared resources have a small link icon on them. And clicking the icon makes unique, so I don't have to go into the context menu.

Newbies are not the primary audience, as I've written before. Every new person eventually joins the middle of the userbase as they learn the quirks. So changing the workflow to accommodate newbies should never come at the cost of becoming less optimal by default.

I do agree that we need better resource visualization. The vram table is great. Let's make something like that for all resources. And one for all materials. And built in tools for the engine to precompile materials on demand. And on screen shader complexity visualization and other metrics (see UE for ideas). Overall we need better resource management, not more redundant resources which this change will create for everyone.

Edit: The proposal talks about not duplicating resources shared to disk. That's good. However, it's very easy for someone to disconnect resources from disk files. Far easier than I know the core devs believe. It's happened to us countless times where one team member is making scenes and objects with the same materials yet some meshes have duplicate in-memory/scene resources and other objects have the version that is on disk. The only indication of whether resources are on the disk or not is if you hover the mouse over and see a filename in the tooltip. This is a far larger problem than what this proposal seeks to fix. Changing behavior based on if connected to a disk file or not, or having inconsistent behavior is a very bad idea. Instead, we need better auditing tools (a resources report and the way resources look) to determine if files are in memory or tied to disk, unique, or shared. Another useful aspect of the resources report are the number of scenes using a resource (visible when you try to delete one), and the option to view all of them (summary and optional detail). You could also put a # of uses on the resource in the inspector instead of a link icon to indicate shared references.

@noidexe
Copy link

noidexe commented Jun 15, 2022

I've been teaching Godot for a while and modifying a shared CollisionShape when you just wanted to duplicate it seems to be the Godot equivalent of drawing in the wrong layer in Photoshop 😄. It can happen even to experienced devs from time to time. So I agree that having some resources duplicate by default is a good idea.

On top of that, for the cases where I want something to be shared it'd be good to have an option to set it per resource rather than an editor-wide or project-wide setting per resource type. And in fact, there is a way to do it, but I think it's poorly communicated and doesn't work as expected. That is the resource_local_to_scene property.

If you don't know about the option, you probably won't ever stumble upon it since it's hidden inside the collapsed "Resource" section. If you do find it, the name is not very indicative of what it does.

Even if you know about it, you may not know that instancing the same scene twice duplicates the resource, but instancing the scene once and then duplicating it with Control-D doesn't. Of course it makes sense from a programming standpoint, the property values are copied so all the references are copied, but it's really unexpected for the end user.

I think it'd be much easier to understand for the end user if it were something like this:

image

"Local To Scene" can be replaced by "Shared" or "Shared Across Instances" and have a consistent reference vs dupicate behavior whether you duplicate a node, duplicate a scene instance or instantiate a scene. Is there a technical reason why it's hidden under Resource or why Resource is collapsed by default?

I can make it a separate proposal if you want but I think it's part of the same workflow

@bodamat
Copy link

bodamat commented Jun 15, 2022

Great idea, because it is really sometimes so annoying. What about adding an extra option for a duplicate with shared subresources? So, when you click the right mouse to the node you will have 2 options:

  • Duplicate (Ctrl+D)
  • Duplicate with shared subresources (Ctrl+Shift+D)

I also completely agree with @noidexe. But could show (shared) text every time not when hovering.

@Calinou Calinou changed the title Always duplicate internal subresources on node duplicate (except when it does not make sense to). Always duplicate internal subresources on node duplicate (except when it does not make sense to) Jun 15, 2022
@noidexe
Copy link

noidexe commented Jun 15, 2022

Great idea, because it is really sometimes so annoying. What about adding an extra option for a duplicate with shared subresources? So, when you click the right mouse to the node you will have 2 options:

Yeah I think Blender has something like that. If I'm not mistaken "Paste Objects" creates a copy of the materials while "Duplicate Objects" keeps the references to the same materials.

https://docs.blender.org/manual/en/2.80/scene_layout/object/editing/duplication.html

[Update]: It seems Blender has a setting exactly like what reduz is proposing.
https://docs.blender.org/manual/en/2.80/editors/preferences/editing.html#prefs-editing-duplicate-data

I also completely agree with @noidexe. But could show (shared) text every time not when hovering.

I meant to say that (shared) always appears if the resource has more than one "owner" (Blender calls it "users"). It'd be extra nice if hovering on it showed you which nodes/scenes use that resource but maybe it's too much.

@h0lley
Copy link

h0lley commented Jun 15, 2022

It'd be extra nice if hovering on it showed you which nodes/scenes use that resource but maybe it's too much.

I think the ability to understand at a glance what resources are used where is more important / helpful than the custom duplication behavior proposed. I'm still in favor of this proposal, just saying we additionally really should want better visualization, it's not too much. however, a tooltip would likely not suffice as it can't be made interactive. let's continue that in a separate proposal such as #4567 though.

@JyveAFK
Copy link

JyveAFK commented Jun 15, 2022

It'd be extra nice if hovering on it showed you which nodes/scenes use that resource but maybe it's too much.

To quickly see /something/, it should be the icon itself in the node tree denoting if it's a copy or a ref, with hovering over it giving more info of it's parent item/other nodes/scenes making use of that item, make it quicker to decide if you want to leave as is, or the right click from there makes it a unique copy. But as is, it DOES show the Instance when you hover over it, but that's not clear instantly from just looking at the scene.

I'd want to look at a scene and quickly see which nodes are ref'd rather then click each node/hover over to figure that out at first.

Don't know if that'd be an easy thing to show... an underline of the icon? to a node to show it's a ref'd item.

ref

@Zylann
Copy link

Zylann commented Jun 15, 2022

When it comes to the editor, I always tended to agree on that internal resources should be duplicated most of the time. When the resource is in a file, it is very clear that it should be referenced, while an internal one is usually internal in the first place for being specific to an instance.

Problematic resources:
I'm not sure meshes or textures should be a problem in practice, we need to see what use cases these come up as internal. It seems like a mistake to me that an external mesh or texture would be made internal...
Internal scripts are an odd one here once again tho: they are not just placed in Godot, they are also edited more deeply through the script editor, so them becoming duplicated can be very confusing and noticed late. Personally, I always hated them and avoided them, so I have no different solution to propose here...

Unintentional duplicate materials would be the worst, as all the redundant textures will waste vram, and since the engine already has issues halting when compiling, more materials compounds the issue.

About nesting:
If the duplicated node has an external resource containing internal sub-resources, the resource should be referenced, and the sub-resources should NOT be duplicated. i.e recursion stops on the the first external.
If the duplicated node has an internal resource which contains external sub-resources, the resource should be duplicated, but its external sub-resources should stay referenced, again stopping recursion.
So in the case of materials, textures most of the time come from files, so there won't be duplication.

About importing:
Importing 3D scenes can come with resources being internal. 3D scenes, in their usable form (instances), are not resources. That makes duplicating instances also duplicate their resources right off the bat... so you'd really have to make sure importing uses files, and tbh, that's what I always go for regardless because it is clearer to navigate and makes it easier to re-use stuff like materials.

Note comparing to Unity, because I can't help it:
In Unity, all resources are files. There are no internal resources. Instead there are components and nestable object exports, which always copy naturally. Resources aren't used for as many things as in Godot. Consequence is, this whole ambiguity doesn't exist here. And when a resource needs duplication, it will be a new file, which is clear and has some other benefits in asset navigation and referencing. In Godot, even a cube shape or a sub-rectangle can be a resource, and they can be internal, nested in others, even scripts are treated this way... which opens many more moving parts (aka "powerful") but raises problems far more challenging to solve.

@wareya
Copy link

wareya commented Jun 16, 2022

Would it be possible to somehow CoW internal resources?

@Zireael07
Copy link

@h0lley and the others discussing showing shared vs duplicated display: that belongs in #904, go in there and show that it's needed!

@graugraugrau
Copy link

graugraugrau commented Jun 16, 2022

I would agree. By copying or duplicating a node I would expect a deep copy.
Additional it would be nice if there would be an indicator if one resource is used by more than one node. That would at least make it easier to detect. Maybe something like that:

godot_proposal

@SlugFiller
Copy link

I've been personally burned on collision shapes. It makes sense for multiple runtime instances of an object (e.g. projectiles, enemies, etc) to share the collision shape instead of duplicating it, small as it may be. But if you're duplicating an object, that is not a scene, in the editor, you're generally expecting it to be a whole new object.

Meshes are not an example for an exception, either. Rather, one must ask, what is the use case where you duplicate an object with a mesh in the editor, and not wrap it in a scene? If it's a prefab of some sort, you would definitely put it in its own scene, so it can also have an associated collision shape and physics object. I can't imagine a case of using the same mesh twice in a scene.

As for materials, it certainly makes sense for textures to be shared, but very much NOT so for materials. If you have multiple objects with the same shader material, for example, you're likely interested in having different shader parameters applied. Things like tint for player color also comes to mind.

The worst case scenario is if an internal resource is copy and pasted across scenes. Godot does NOT handle this case gracefully. A resource that is internal to one scene, being referenced from another scene? It's an invitation for disaster. By all rights, this should be impossible to achieve, and be considered an invalid state. And yet, this is the default behavior for a simple copy and paste.

So I agree with what many others said above: If a resource is meant to be shared, it should be a file. Any other resource should be scene and object local. And should make it as simple as possible to determine and set whether it is instance local as well.

@TokisanGames
Copy link

TokisanGames commented Jan 11, 2023

Materials should be shared as much as possible as well. For unique variables on different objects, there are uniform instances now in gd4.
https://godotengine.org/article/godot-40-gets-global-and-instance-shader-uniforms

@Calinou
Copy link
Member

Calinou commented Nov 3, 2023

I've started working on this: https://github.com/Calinou/godot/tree/editor-duplicate-resources-on-node-duplicate

Example with material/script duplication enabled for illustration purposes (the final PR shouldn't duplicate those as described in the proposal):

simplescreenrecorder-2023-11-03_18.35.35.mp4

Unfortunately, it doesn't always work reliably in all situations for reasons that elude me:

  • Some properties are simply ignored, such as Sprite2D's texture (again for illustration purposes), or WorldEnvironment's environment property.
  • Attempting to use it with nodes such as MeshInstance3D will print the following to console:
ERROR: Condition "!is_inside_tree()" is true. Returning: Transform3D()
   at: get_global_transform (./scene/3d/node_3d.cpp:343)

This seems to be simply caused by looping over the properties and checking whether they extend from Resource – maybe this is due to the global_transform property in Node3D? Either way, I'd need to exclude properties that cause known errors before attempting to detect which properties are resources.

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