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.

Sign up for GitHub

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

Jump to bottom

Track View: Optional fix for GHI #11500: fix prefabs previously stored with invalid entity parents. #18548

Open
wants to merge 3 commits into
base: development
Choose a base branch
from
Open

Track View: Optional fix for GHI #11500: fix prefabs previously stored with invalid entity parents. #18548

wants to merge 3 commits into from
+121 −2

Conversation

Aleks-Starykh-TR
Copy link
Contributor

What does this PR do?

Fixes one consequence of the issue #11500:

With earlier code prefabs could be saved with "torn-off" Entities having a TrackView EditorSeqenceComponrnt:
with a spoiled (empty) "Parent Entity" sentence in the TransformComponent of this entity.
Such prefab is still loaded wrongly with the current code without even warning, which seems not very user - friendly.

This optional fix:

  • Adds checks for "Parent Entity" sentences in the "Entities" container of a prefab being loaded;
  • When an empty parent statement is found, reports error and links the entity to the ContainerEntity of the prefab;
  • In case a user resaves this PR, it will be finally fixed.

In most practical cases the AssetBuilder now will deserialize a buggy prefab into a correct spawnable (with error report).

This PR is optional, but I remember that similar problems with "torn-off" entities made porting of LY slices into prefabs very difficult (especially for levels), so I dared to spend some (minimal) time to implement this.

How was this PR tested?

Saved (and artificially) broken prefabs with the linked Camera2 and Sequence entities evaluated by AssetProcessor and loaded into Editor under Windows:

  • expected error report noted both in AssetProcessor and Editor;
  • loaded prefabs had correct structure;
  • in case of resaving a previously broken prefab, it is fixed.

@Aleks-Starykh-TR Aleks-Starykh-TR requested review from a team as code owners December 11, 2024 16:07
@Aleks-Starykh-TR Aleks-Starykh-TR mentioned this pull request Dec 11, 2024
Open
@guillaume-haerinck guillaume-haerinck added feature/trackview This item is related to the Trackview and cinematic feature. sig/content Categorizes an issue or PR as relevant to SIG Content. feature/prefabs This item is related to the prefab system labels Dec 11, 2024
@Aleks-Starykh-TR
Address PR Comments
30b38b7 
Signed-off-by: Aleks Starykh <[email protected]>
@Aleks-Starykh-TR Aleks-Starykh-TR changed the title Track View: Optional fix for GHI #1500: fix prefabs previously stored with invalid entity parents. Track View: Optional fix for GHI #11500: fix prefabs previously stored with invalid entity parents. Dec 11, 2024
guillaume-haerinck
guillaume-haerinck reviewed Dec 11, 2024
View reviewed changes
Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabLoader.cpp
@@ -605,7 +605,10 @@ namespace AzToolsFramework

PrefabDom& loadedTemplateDomRef = loadedTemplateDom->get();

// first, decode the template DOM into actual Instance data. This will actually create real
// first, check and fix prefabs which could be previously stored with invalid entities' parents (GHI #11500)
PrefabDomUtils::SubstituteInvalidParentsInEntities(loadedTemplateDomRef);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its a good change and I'll approve, but thinking long-term I don't think we should pay the cost upon loading a level for validation checkups as it might get expensive and basically you should only do it once ever per broke dataset.

We don't have these facilities right now in the editor, but I would rather have a batch/script UI tool that you can trigger on a provided list of files/current open file to fix the data and call it done. A quick way to have something like that would be as an additional button in the menu "Fix prefab data"

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tagging @lsemp3d for vis, as in general as we will have bigger projects in o3de, we'll need facilities to fixup large batch of data, auto-checkout the files, and generate a text report

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guillaume-haerinck guillaume-haerinck guillaume-haerinck left review comments

@lsemp3d lsemp3d lsemp3d approved these changes

Assignees
No one assigned
Labels
feature/prefabs This item is related to the prefab system feature/trackview This item is related to the Trackview and cinematic feature. sig/content Categorizes an issue or PR as relevant to SIG Content.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Aleks-Starykh-TR @guillaume-haerinck @lsemp3d