Implement Prefab workflow#776
Conversation
adriengivry
left a comment
adriengivry
left a comment
There was a problem hiding this comment.
Looks really good!
Some feedback I have:
- In Unity, prefabs generally kind-of ignore the transformations on the topmost actor. With your feature, if I make a prefab from an actor, then duplicate this actor many time and place them in a level, reverting them to the original prefab will also move them back to the center of the level. We probably want the "Revert to prefab" to never revert the topmost actor's transform data. The only case the prefab's topmost actor's transform data would be used is when creating a new instance.
- It would be AWESOME if we could drag-and-drop any actor from the hierarchy to any folder in the asset browser to turn it into a prefab (using the actor's name as the prefab's name).
- When instancing a prefab, the prefab name should be used for the topmost actor, instead of the actor's name.
adriengivry
left a comment
adriengivry
left a comment
There was a problem hiding this comment.
Works very well, I tested with a bunch of Cornell boxes!
A few issues I found:
- See comments I added in the code
- It is hard to know when looking at the hierarchy which source prefab an actor might come from. We could add a field to the actor's inspector, below "GUID" that would show an asset field of type prefab, similar to the "Target" field in asset properties, but that would be fully disabled.
- Reverting an actor to its source prefab overrides its name. In the case where you name your objects (e.g. Pillar 1, Pillar 2, Pillar 3, it will always go back to "Pillar" after reverting)
- Reverting an actor to its source prefab also move it in the hierarchy:
| Before Revert | After Revert |
|---|---|
 |
 |
adriengivry
left a comment
adriengivry
left a comment
There was a problem hiding this comment.
A regression I also found is using the "Create" menu to add an actor as a child of another actor end up attaching the new actor to the same parent as the actor the contextual menu has been opened from.
| Before | After |
|---|---|
 |
 |
Good catch ! |
Thanks ! |
There was a problem hiding this comment.
Note
Didn't look too hard in the code, since there are a few major issues that prevented further testing, and the code is likely gonna change.
Incorrect contextual actions
After creating a scene, adding a prefab, and reloading the scene, the contextual menu doesn't show anything else that "Save as prefab..." anymore, even after creating one.
[Regression] Broken contextual actions on loaded scenes
Whenever a scene is loaded (not new scene), contextual actions on any actor will be broken. e.g. cannot delete, duplicate, save as prefab, "Create..." menu will attach new actor to root instead...
Wrong GUID re-assignation
- Create a prefab with actor as children
- Drag-and-drop first children onto parent
- Notice A GUID is still correct
- Revert to prefab
- Notice A GUID is now different, and C now has A's GUID
Thanks for the detailed feedback, this is now addressed. I fixed the blocking issues in
Related previous fixes are still in place ( |
adriengivry
left a comment
adriengivry
left a comment
There was a problem hiding this comment.
Functionality-wise it seems to be nearly feature-complete. There are a few bugs and issues I found that are listed below.
Otherwise great work!
| std::vector<size_t> unmatchedTemplateChildren; | ||
| }; | ||
|
|
||
| ChildMatchPlan BuildChildMatchPlan(OvCore::ECS::Actor& p_targetActor, OvCore::ECS::Actor& p_templateActor) |
There was a problem hiding this comment.
The whole matching strategy is still very fragile. We should never rely on actor names, or position in the hierarchy.
Here is an example case that fail:
Initial setup:
Stack (GUID 233...)
- A (GUID D72...)
- B (GUID 20B...)
- C (GUID C88...)
Moved A below C. And renamed all 3 to Foo:
Stack (GUID 233...)
- Foo (previously B) (GUID 20B...)
- Foo (previously C) (GUID C88...)
- Foo (previously A) (GUID D72...)
Revert to prefab:
Stack (GUID 233...)
- A (GUID 20B...)
- B (GUID C88...)
- C (GUID D72...)
As you can see, it doesn't match the initial setup anymore, A is now B, B is now C, and C is now A.
Instead of relying on Actor names and GUIDs, we should add new metadata to actors, alongside m_prefabSource, we should have m_prefabNodeGUID. Each actor inside of a prefab would have a unique GUID, that would be carried over when instanced. This way, we can easily track back which Actor in a scene correspond to which actor in a prefab.
| * @param p_onItemClicked | ||
| */ | ||
| static void GenerateActorCreationMenu(OvUI::Widgets::Menu::MenuList& p_menuList, OvCore::ECS::Actor* p_parent = nullptr, std::optional<std::function<void()>> p_onItemClicked = {}); | ||
| static void GenerateActorCreationMenu(OvUI::Widgets::Menu::MenuList& p_menuList, ActorParentProvider p_parentProvider = {}, std::optional<std::function<void()>> p_onItemClicked = {}); |
There was a problem hiding this comment.
Not sure I understand the point of this p_parentProvider. It worked well before without it, with only a pointer to the parent. Why do we suddenly need to patch this?
| if (auto* target = GetTargetActor()) | ||
| { | ||
| EDITOR_EXEC(MoveToTarget(*target)); |
There was a problem hiding this comment.
Why do we need that? It worked fine before.
This look like a workaround on a problem that hasn't been properly root-caused.
Same for all the actions below.
| OvCore::ECS::Actor* GetTargetActor() const | ||
| { | ||
| if (!m_targetActor) | ||
| { | ||
| return nullptr; | ||
| } | ||
|
|
||
| auto* currentScene = EDITOR_CONTEXT(sceneManager).GetCurrentScene(); | ||
| if (!currentScene) | ||
| { | ||
| return nullptr; | ||
| } | ||
|
|
||
| const auto& actors = currentScene->GetActors(); | ||
| const bool actorIsStillInScene = | ||
| std::find(actors.begin(), actors.end(), m_targetActor) != actors.end(); | ||
|
|
||
| return actorIsStillInScene ? m_targetActor : nullptr; | ||
| } |
There was a problem hiding this comment.
Why can't we just use OvCore::ECS::Actor* m_target; like before? I don't understand the problem this is trying to solve.
| std::function<void()> ActorWithComponentCreationHandler(ActorParentProvider p_parentProvider, std::optional<std::function<void()>> p_onItemClicked) | ||
| { | ||
| return Combine(EDITOR_BIND(CreateMonoComponentActor<T>, true, p_parent), p_onItemClicked); | ||
| return [p_parentProvider = std::move(p_parentProvider), p_onItemClicked]() | ||
| { | ||
| EDITOR_EXEC(CreateMonoComponentActor<T>(true, ResolveParent(p_parentProvider))); | ||
|
|
||
| if (p_onItemClicked.has_value()) | ||
| { | ||
| p_onItemClicked.value()(); | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| std::function<void()> ActorWithModelComponentCreationHandler(ActorParentProvider p_parentProvider, const std::string& p_modelName, std::optional<std::function<void()>> p_onItemClicked) | ||
| { | ||
| return [p_parentProvider = std::move(p_parentProvider), p_modelName, p_onItemClicked]() | ||
| { | ||
| EDITOR_EXEC(CreateActorWithModel(":Models\\" + p_modelName + ".fbx", true, ResolveParent(p_parentProvider), p_modelName)); | ||
|
|
||
| if (p_onItemClicked.has_value()) | ||
| { | ||
| p_onItemClicked.value()(); | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| std::function<void()> ActorWithModelComponentCreationHandler(OvCore::ECS::Actor* p_parent, const std::string& p_modelName, std::optional<std::function<void()>> p_onItemClicked) | ||
| std::function<void()> CreateSkysphereHandler(ActorParentProvider p_parentProvider, std::optional<std::function<void()>> p_onItemClicked) | ||
| { | ||
| return Combine(EDITOR_BIND(CreateActorWithModel, ":Models\\" + p_modelName + ".fbx", true, p_parent, p_modelName), p_onItemClicked); | ||
| return [p_parentProvider = std::move(p_parentProvider), p_onItemClicked]() | ||
| { | ||
| CreateSkysphere(ResolveParent(p_parentProvider)); | ||
|
|
||
| if (p_onItemClicked.has_value()) | ||
| { | ||
| p_onItemClicked.value()(); | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| std::function<void()> CreateSkysphereHandler(OvCore::ECS::Actor* p_parent, std::optional<std::function<void()>> p_onItemClicked) | ||
| std::function<void()> CreateAtmosphereHandler(ActorParentProvider p_parentProvider, std::optional<std::function<void()>> p_onItemClicked) | ||
| { | ||
| return Combine(std::bind(CreateSkysphere, p_parent), p_onItemClicked); | ||
| return [p_parentProvider = std::move(p_parentProvider), p_onItemClicked]() | ||
| { | ||
| CreateAtmosphere(ResolveParent(p_parentProvider)); | ||
|
|
||
| if (p_onItemClicked.has_value()) | ||
| { | ||
| p_onItemClicked.value()(); | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| std::function<void()> CreateAtmosphereHandler(OvCore::ECS::Actor* p_parent, std::optional<std::function<void()>> p_onItemClicked) | ||
| std::function<void()> CreateCharacterHandler(ActorParentProvider p_parentProvider, std::optional<std::function<void()>> p_onItemClicked) | ||
| { | ||
| return Combine(std::bind(CreateAtmosphere, p_parent), p_onItemClicked); | ||
| return [p_parentProvider = std::move(p_parentProvider), p_onItemClicked]() | ||
| { | ||
| CreateCharacter(ResolveParent(p_parentProvider)); | ||
|
|
||
| if (p_onItemClicked.has_value()) | ||
| { | ||
| p_onItemClicked.value()(); | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
We shouldn't need to change these, they are working perfectly fine. Same as above, looks like a patch on an issue that hasn't been properly root-caused.
| primitives.CreateWidget<MenuItem>("Cube").ClickedEvent += ActorWithModelComponentCreationHandler(p_parentProvider, "Cube", p_onItemClicked); | ||
| primitives.CreateWidget<MenuItem>("Sphere").ClickedEvent += ActorWithModelComponentCreationHandler(p_parentProvider, "Sphere", p_onItemClicked); | ||
| primitives.CreateWidget<MenuItem>("Cone").ClickedEvent += ActorWithModelComponentCreationHandler(p_parentProvider, "Cone", p_onItemClicked); | ||
| primitives.CreateWidget<MenuItem>("Cylinder").ClickedEvent += ActorWithModelComponentCreationHandler(p_parentProvider, "Cylinder", p_onItemClicked); | ||
| primitives.CreateWidget<MenuItem>("Plane").ClickedEvent += ActorWithModelComponentCreationHandler(p_parentProvider, "Plane", p_onItemClicked); | ||
| primitives.CreateWidget<MenuItem>("Gear").ClickedEvent += ActorWithModelComponentCreationHandler(p_parentProvider, "Gear", p_onItemClicked); | ||
| primitives.CreateWidget<MenuItem>("Helix").ClickedEvent += ActorWithModelComponentCreationHandler(p_parentProvider, "Helix", p_onItemClicked); | ||
| primitives.CreateWidget<MenuItem>("Pipe").ClickedEvent += ActorWithModelComponentCreationHandler(p_parentProvider, "Pipe", p_onItemClicked); | ||
| primitives.CreateWidget<MenuItem>("Pyramid").ClickedEvent += ActorWithModelComponentCreationHandler(p_parentProvider, "Pyramid", p_onItemClicked); | ||
| primitives.CreateWidget<MenuItem>("Torus").ClickedEvent += ActorWithModelComponentCreationHandler(p_parentProvider, "Torus", p_onItemClicked); | ||
| physicals.CreateWidget<MenuItem>("Physical Box").ClickedEvent += ActorWithComponentCreationHandler<CPhysicalBox>(p_parentProvider, p_onItemClicked); | ||
| physicals.CreateWidget<MenuItem>("Physical Sphere").ClickedEvent += ActorWithComponentCreationHandler<CPhysicalSphere>(p_parentProvider, p_onItemClicked); | ||
| physicals.CreateWidget<MenuItem>("Physical Capsule").ClickedEvent += ActorWithComponentCreationHandler<CPhysicalCapsule>(p_parentProvider, p_onItemClicked); | ||
| lights.CreateWidget<MenuItem>("Point").ClickedEvent += ActorWithComponentCreationHandler<CPointLight>(p_parentProvider, p_onItemClicked); | ||
| lights.CreateWidget<MenuItem>("Directional").ClickedEvent += ActorWithComponentCreationHandler<CDirectionalLight>(p_parentProvider, p_onItemClicked); | ||
| lights.CreateWidget<MenuItem>("Spot").ClickedEvent += ActorWithComponentCreationHandler<CSpotLight>(p_parentProvider, p_onItemClicked); | ||
| lights.CreateWidget<MenuItem>("Ambient Box").ClickedEvent += ActorWithComponentCreationHandler<CAmbientBoxLight>(p_parentProvider, p_onItemClicked); | ||
| lights.CreateWidget<MenuItem>("Ambient Sphere").ClickedEvent += ActorWithComponentCreationHandler<CAmbientSphereLight>(p_parentProvider, p_onItemClicked); | ||
| audio.CreateWidget<MenuItem>("Audio Source").ClickedEvent += ActorWithComponentCreationHandler<CAudioSource>(p_parentProvider, p_onItemClicked); | ||
| audio.CreateWidget<MenuItem>("Audio Listener").ClickedEvent += ActorWithComponentCreationHandler<CAudioListener>(p_parentProvider, p_onItemClicked); | ||
| others.CreateWidget<MenuItem>("Camera").ClickedEvent += ActorWithComponentCreationHandler<CCamera>(p_parentProvider, p_onItemClicked); | ||
| others.CreateWidget<MenuItem>("Post Process Stack").ClickedEvent += ActorWithComponentCreationHandler<CPostProcessStack>(p_parentProvider, p_onItemClicked); | ||
| others.CreateWidget<MenuItem>("Reflection Probe").ClickedEvent += ActorWithComponentCreationHandler<CReflectionProbe>(p_parentProvider, p_onItemClicked); | ||
| others.CreateWidget<MenuItem>("Skysphere").ClickedEvent += CreateSkysphereHandler(p_parentProvider, p_onItemClicked); | ||
| others.CreateWidget<MenuItem>("Atmosphere").ClickedEvent += CreateAtmosphereHandler(p_parentProvider, p_onItemClicked); | ||
| others.CreateWidget<MenuItem>("Character").ClickedEvent += CreateCharacterHandler(p_parentProvider, p_onItemClicked); |
There was a problem hiding this comment.
Shouldn't be needed, see comment above.
| if (!prefabName.empty()) | ||
| instantiatedRoot->SetName(prefabName); | ||
|
|
||
| OVLOG_INFO("Prefab instantiated: " + realPath.string()); |
There was a problem hiding this comment.
Not needed. We recently removed the "Actor created" log, this one should follow the same treatment.
2 participants
Description
This PR delivers the prefab MVP workflow in the editor end-to-end: creating prefab assets from actors, instantiating prefabs, maintaining actor-to-prefab links ("source instance"), editing prefab instances (apply/revert), and propagating asset renames through scenes and prefabs.
Implemented:
.ovprefabfile type support in asset typing/parsing.Save as Prefab...in actor contextual menu.PrefabOperations).prefab_sourcesupport (API + serialization/deserialization).Create > From prefab...in actor creation menu (hierarchy + menu bar).Apply to PrefabandRevert to Prefabfor prefab-linked actors..ovprefabfiles..ovprefabfiles.Related Issue(s)
Fixes #25
Review Guidance
Please validate the prefab workflow in this order:
Save as Prefab....Create > From prefab...from hierarchy/menu bar.Apply to Prefab; verify.ovprefabcontent updates.Revert to Prefab; verify actor hierarchy is replaced correctly..ovscenefiles,.ovprefabfiles,Key files:
Sources/OvEditor/include/OvEditor/Utils/PrefabOperations.hSources/OvEditor/src/OvEditor/Utils/PrefabOperations.cppSources/OvEditor/include/OvEditor/Core/EditorActions.hSources/OvEditor/src/OvEditor/Core/EditorActions.cppSources/OvCore/include/OvCore/ECS/Actor.hSources/OvCore/src/OvCore/ECS/Actor.cppSources/OvEditor/src/OvEditor/Panels/Hierarchy.cppSources/OvEditor/src/OvEditor/Utils/ActorCreationMenu.cppSources/OvEditor/include/OvEditor/Panels/SceneView.hSources/OvEditor/src/OvEditor/Panels/SceneView.cppSources/OvTools/include/OvTools/Utils/PathParser.hSources/OvTools/src/OvTools/Utils/PathParser.cppSources/OvCore/src/OvCore/Helpers/GUIHelpers.cppSources/OvEditor/src/OvEditor/Core/EditorResources.cppSources/OvEditor/src/OvEditor/Core/Editor.cppResources/Editor/Textures/Prefab.pngScreenshots/GIFs
AI Usage Disclosure
Generated new code / Refactored code
Checklist