-
-
Notifications
You must be signed in to change notification settings - Fork 22.8k
Add option to apply instance changes to base scene #85562
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
base: master
Are you sure you want to change the base?
Conversation
Is it intentional that editable children gets disabled when you apply? It would probably make sense to keep them on so you could continue tweaking and applying values and see them being applied to other instances in the scene without having to re-enable editable children. If you want to disable editable children and apply there's the checkbox thing |
It's not intentional. |
I think it is much better to disable the "Editable Children" setting automatically, as an indicator that all changes have been applied. Seeing the child nodes expanded is a strong indication that they may have been edited. If the "Editable Children" setting is left enabled, how would one know if there are changes that haven't yet been applied to the base scene? Will there be some other indicator? |
b662530
to
6e29ed6
Compare
I think there's other ways to indicate that the scene isn't saved (for example an asterisk on the side of the name) |
What is the status of this feature? This is exactly what I was looking for and need it for my workflow. Can anyone help me regarding this? |
If you want to use this feature before it gets integrated you can build the engine yourself with this included |
Will do that. Is it 100% sure that it will be implemented? Edit: Have implemented it (had to change minor things built without errors. It does work, however, when applied to base scene it does lock the scene. So when I try to edit children again, I have to make the material first 'unique' before I am able to change it again. Would be great if this part isnt needed. It does also change the position when apply to base scene, so it cannot be used as a 'prefab' thing. To bypass this issue, is by saving the scene again which will keep the model link when reimporting the model, but keep the position and other settings apart from it as an variant. I hope it can be sorted out that it would work fine. |
I was just looking for this feature! Does anyone know what exactly needs to get resolved? I read through this conversation, but it seems there is some disagreement and ambiguity on what would make this "done" |
Had a quick discussion with Kobewi in chat and added this to 4.5, not as a promise but as a "hey let's see if we can get this looked at by 4.5", since this is pending bugfixing and reviews. |
6e29ed6
to
0f546f5
Compare
That's because the editable node is replaced back by the modified scene. Fixing it would require some unrelated changes, so I'd leave it for later.
This is a tricky one. I think the perfect option is to preserve original transform. However, with how the PR is currently implemented, I can either keep it or reset it. Also, there is position, rotation and scale. I imagine some people would want either of them to be applied to the base scene.
One raised concern in that discussion were local resources. I just tested and they are correctly overwriting the originals. There is a pesky case of some resources shared between scenes, but that's unrelated. |
The way Unity handles this is to not override position or rotation changes when pushing "all" changes back to the prefab by default, but scale is included. But if you want to apply position/rotation you can directly right click on the property and choose to override that specific value. This works for any property too, not just transform values. Something similar might work well here if it's viable! But if that's too granular for now, maybe we could simply have 2 buttons next to each other, one that ignores transform changes that does what it does now and one for "Apply Transform Changes to Base Scene" that includes position, rotation, and scale? Regardless, I'd really love to see this merged for 4.5, would definitely help with workflow a lot! |
The main problem with adding buttons is that there are 2 dialogs that can apply changes. I'll just make it skip transform for now; it can be adjusted later based on feedback. |
a936df9
to
ac7c769
Compare
a6a3927
to
c68c0fa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally (rebased on top of master
dbddc9e), it mostly works as expected.
I've tested all these scenarios:
- Adding a new child node with Editable Children, disabling Editable Children then choosing Apply to Base Scene in the confirmation dialog.
- Adding a new child node without Editable Children, right-clicking the parent (the instanced scene) and choosing Apply to Base Scene.
- Modifying a child node's property with Editable Children, then applying it.
- Modifying a instanced scene root node's property without Editable Children, then applying it.
These all work correctly, however, I noticed two issues that affect all workflows:
- Changes made to the base scene don't apply to other instances until you use Scene > Reload Saved Scene, even though they are already visible when you run the project. This also affects live scene editing – any changes applied this way won't be visible until you restart the project.
- Undo/redo appears not to work, so there should be a confirmation dialog whenever you use this feature (which could be skipped by holding Shift as usual).
What do you mean "as usual"? Does such skip exist somewhere? I recall some PRs, but not sure if they were merged and where. |
c68c0fa
to
d601453
Compare
If you hold Shift when removing a node, the confirmation dialog is skipped. |
That's because we have a separate "Delete (no confirm)" shortcut, which defaults to Shift+Delete. When you activate the option from context menu, you have no way to skip the dialog. |
While I would love this feature as it makes the entire scene concept more powerful for developers and brings Godot closer to Unity's implementation (of prefabs), a previous bug that overrides values on the instances incorrectly needs to be fixed in my opinion. If we can't reliably trust that the value is correctly set on the instance, then pushing this value back to the base scene sounds rather precarious to me. I've scoured the github and found many issues related to scene instances not correctly holding the values because of the current state of scene inheritance. #3904 I believe the majority of these issues are the same bug. Some of them may not be. |
Is there any blockers on this? |
Needs to be reviewed and approved. |
Closes godotengine/godot-proposals#7649
godot.windows.editor.dev.x86_64_IfbzLfhz5L.mp4
This is an experimental approach. I was wondering about best way to update properties in a scene and what I did is simply overwriting the original scene with instance branch 🤷♂️ There are some problems with editor not reloading properly, but overall it seems to work. Unless I'm missing some use-case that's completely broken with this solution?
Putting as draft for now. I'll finish up UX (e.g. by adding "Apply Changes" checkbox when disabling editable children) when the base idea turns out ok.