-
-
Notifications
You must be signed in to change notification settings - Fork 1k
feat: Hierarchical Model Importer #2898
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
. 1. Changes to MeshConvertor and MeshAssetImporter to be able to import sub meshes in model as individual models in project assets
Checkbox option for split at time of import
With all fixes suggested stride3d#2553 (comment)
@dotnet-policy-service agree |
Looks like materials are not handled properly: every individual object is set to the same material. The prefab's hierarchy is almost right, there is an additional Here's the file I used for testing - this is not CC0, do not use in projects. |
Thanks for sharing the test file. I’ve checked in updates, and both issues are now fixed:
|
We're back to the previous issue with the materials, we need to only have the materials that are assigned to an object on that object, not every material that was contained in the original scene. Also, the pivot position for individual models is not retained, they all have their pivot set to the origin instead of wherever it actually is in the file. |
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.
You haven't thoroughly gone through the LLM's work there it seems, there's a fairly large amount of redundant logic, and comments that are clearly targeted at someone interacting with an LLM, not at describing the logic being implemented.
I'm okay with reviewing someone's work, not going over what an LLM spit out, that is your burden to carry - and you're definitely capable of that, I have seen you write better stuff than this.
I haven't gone through the whole thing yet, but do go over your entire PR and clean things up, there's a lot of low hanging fruits I have not marked.
You could also look into using/enabling nullable, there's a bunch of null checks and tests that are redundant or definitely should throw in the null case.
if (splitHierarchy) | ||
{ | ||
var entityInfo = importer.GetEntityInfo(file, parameters.Logger, importParameters); | ||
if (entityInfo != null && (entityInfo.Models?.Count ?? 0) > 0) |
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.
Can be simplified if (entityInfo?.Models?.Count > 0)
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.
fixed
|
||
|
||
// No combined "(All)" model when splitting; Prefab is the combined representation | ||
AssetItem allModelAsset = null; |
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.
Unused
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.
fixed, no longer needed
// Build prefab using the per-mesh models in the same order we created them | ||
var perMeshModels = perMeshAssets; | ||
|
||
|
||
for (int i = 0; i < entityInfo.Models.Count; i++) | ||
{ | ||
var item = perMeshModels[i]; | ||
if (item?.Asset is ModelAsset modelAsset) | ||
{ | ||
TrimModelAssetToSingleMaterialByIndex(modelAsset, | ||
entityInfo.Models[i].OriginalMaterialIndex); // <- the index from step 1 | ||
} | ||
} |
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.
Looks like splitting into a new loop is unnecessary, you could just append that logic at line 212 ? And then remove perMeshAssets
?
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.
fixed with new mesh to material mapping
asset.Materials.Add(keep); | ||
} | ||
|
||
private static AssetItem BuildPrefabForSplitHierarchy(string baseName, EntityInfo entityInfo, IList<AssetItem> perMeshModels, AssetItem allModelAsset, UDirectory targetLocation) |
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.
Annotate return type with CanBeNull
, remove AssetItem allModelAsset
parameter
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.
fixed
else | ||
{ | ||
// Split OFF: keep a single Model named exactly after the source file (no "(All)") | ||
var idx = assets.FindIndex(a => a.Asset is ModelAsset); | ||
if (idx >= 0) | ||
{ | ||
var old = assets[idx]; | ||
assets.RemoveAt(idx); | ||
|
||
// Assign a fresh Id to the single model to avoid any Id collisions | ||
((ModelAsset)old.Asset).Id = AssetId.New(); // <<—— ensure unique Id | ||
|
||
var uniqueFile = MakeUniqueFileName(baseName, assets); | ||
var renamed = new AssetItem(UPath.Combine(parameters.TargetLocation, uniqueFile), old.Asset); | ||
assets.Insert(idx, renamed); | ||
} | ||
} |
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.
Shouldn't the case for splitHierarchy == false
be left untouched, what is this fixing exactly ?
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.
it was only there to seperate account for (all) model when in prefab and the legacy flow, but since thats not needed so removed
private static void TrimModelAssetToSingleMaterialByIndex(ModelAsset asset, int keepIndex) | ||
{ | ||
if (asset?.Materials == null) return; | ||
if (keepIndex < 0 || keepIndex >= asset.Materials.Count) return; | ||
|
||
var keep = asset.Materials[keepIndex]; | ||
asset.Materials.Clear(); | ||
asset.Materials.Add(keep); | ||
} |
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.
A single model should be able to have more than one material assigned to it, this is not the source of the issue obviously, something else earlier in the call chain doesn't handle it appropriately. Here an example where I simply assigned a second material to the bookend mesh:
Here's the file
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.
Fixed, by storing material to mesh mapping at entityinfo level as ground truth
sources/editor/Stride.Assets.Presentation/Templates/ModelFromFileTemplateGenerator.cs
Show resolved
Hide resolved
if (!nodeNameToIndex.TryGetValue(meshInfo.NodeName, out var nodeIndex)) | ||
continue; |
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.
Shouldn't this be an exception ? If this fail we would silently ignore one of the models ?
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.
fixed
{ | ||
if (!extraChildCountByNode.TryGetValue(nodeIndex, out var k)) | ||
k = 0; | ||
k++; |
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.
rename k
to something more explicit
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.
fixed, renamed it to counter
var firstName = firstNode.Name ?? string.Empty; | ||
bool looksLikeWrapper = | ||
string.IsNullOrWhiteSpace(firstName) | ||
|| firstName.Equals(baseName, StringComparison.OrdinalIgnoreCase) | ||
|| firstName.Equals("RootNode", StringComparison.OrdinalIgnoreCase); |
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.
Redudant firstName
, you're already testing for null in looksLikeWrapper
, change to
bool looksLikeWrapper =
string.IsNullOrWhiteSpace(firstNode.Name)
|| firstNode.Name.Equals(baseName, StringComparison.OrdinalIgnoreCase)
|| firstNode.Name.Equals("RootNode", StringComparison.OrdinalIgnoreCase);
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.
fixed
Got it. I was focusing on getting it fully functional before moving on to cleanup and optimization, since the prefab generation feature is a bit tricker than I anticipated. I’ve removed any mechnical commets. Committed the latest version with fixes and a cleaner codebase @Eideren |
Throws in |
…shConvertor Level
No more string manipulations by storing source mesh name variable in ModelAsset, checked in latest @Eideren |
Most of my comments have not been addressed, moving this to draft while you take care of those. Feel free to set it to ready once it is actually ready for review. |
I really can't see that list my side of github :) ![]() |
Ah, okay, my bad, that's on me. Give me a sec. |
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.
Well, I did mention ResetMaterialsOnPrefabItems
requiring a cleanup, it hasn't changed much, I'll leave these few comments for now
var importAnimations = parameters.Tags.Get(ImportAnimationsKey); | ||
var importSkeleton = parameters.Tags.Get(ImportSkeletonKey); | ||
var skeletonToReuse = parameters.Tags.Get(SkeletonToUseKey); | ||
var splitHierarchy = parameters.Tags.Get(SplitHierarchyKey); // <-- you read it here |
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.
Remove this comment
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.
Done!
|
||
foreach (var file in files) | ||
{ | ||
// TODO: should we allow to select the importer? |
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.
No reason to delete this comment
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.
Fixed
return new AssetItem(UPath.Combine(targetLocation, prefabUrl), prefab); | ||
} | ||
|
||
private static string SanitizePart(string s) |
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.
string?
for both parameter and return type
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.
Done!
var underlyingModel=entityInfo.Models.Where(C=>C.MeshName==underlyingMeshName).FirstOrDefault(); | ||
var nodeContainingMesh=entityInfo.Nodes.Where(c=>c.Name== underlyingModel.NodeName).FirstOrDefault(); | ||
|
||
var materialIndices=entityInfo.NodeNameToMaterialIndices?.Where(c=>c.Key== nodeContainingMesh.Name)?.FirstOrDefault().Value; |
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.
NodeNameToMaterialIndices
is never null, the first ?
is unnecessary. A Where
Linq call never returns null so the second ?
is unnecessary as well, Where(x).FirstOrDefault()
can be simplified like mentioned above but ... it is a dictionary ... you can replace the Where
with a try get, but should you even do a try get, wouldn't this being null be a bug, in which case it should just be a direct access which would throw when it doesn't find the key.
This method is very fragile, would it even work properly after the making the names unique ?
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.
Fixed, The FBX as format doesn’t enforce unique mesh names. Adding an extra identifier simply ensures uniqueness. It should work fine, and if there is edge case realted to this not taken into account and might fail, should be easy to take care of!
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.
For every mesh in model there would always be a fbx node and storing mapping directly at node level instead of mesh is to account for multiple materials on mesh
|
||
var materialIndices=entityInfo.NodeNameToMaterialIndices?.Where(c=>c.Key== nodeContainingMesh.Name)?.FirstOrDefault().Value; | ||
|
||
if(materialIndices?.Count()< 1) |
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.
Count()
is the Linq method, use the property Count
instead. Also, this will not return if materialIndices
is null, which would cause the execution to throw at line 267 in that case.
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.
Fixed
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.
only to account for edge case if theres is no material imported for mesh
return; | ||
|
||
var underlyingModel=entityInfo.Models.Where(C=>C.MeshName==asset.MeshName).FirstOrDefault(); | ||
var nodeContainingMesh=entityInfo.Nodes.Where(c=>c.Name== underlyingModel.NodeName).FirstOrDefault(); |
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.
Where(x).FirstOrDefault()
can be simplified to FirstOrDefault(x)
, but if nodeContainingMesh
or underlyingModel
are null, the line below would throw, so best use First(x)
.
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.
Fixed
List<ModelMaterial> materialsToApply = null; | ||
for (int i = 0; i < asset.Materials.Count; i++) | ||
{ | ||
if (materialIndices.Contains(i)) | ||
{ | ||
(materialsToApply??=new List<ModelMaterial>()).Add(asset.Materials[i]); | ||
} | ||
} | ||
|
||
asset.Materials.Clear(); | ||
materialsToApply?.ForEach(_mat => asset.Materials.Add(_mat)); |
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.
This is more complicated than necessary, do reverse for loop and remove from asset.Materials
when materialIndices.Contains(i) == false
instead
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.
Fixed
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.
Can you test your PR more thoroughly, started reviewing again and found another issue on the first thing I checked.
Removed the filename-based mesh selection and made it an explicit part of the asset data. The chosen mesh now comes from ModelAsset.MeshName, which is passed via ModelAssetCompiler > ImportThreeDCommand to MeshConverter.KeepOnlyMeshByName. The old DecideKeptMeshIndexFromOutput logic is removed @Eideren |
Did you test your changes ? |
Yes tested with the room model file
…On Sun 12. Oct 2025 at 20:20, Eideren ***@***.***> wrote:
*Eideren* left a comment (stride3d/stride#2898)
<#2898 (comment)>
Did you test your changes ?
—
Reply to this email directly, view it on GitHub
<#2898 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AF5T3BZKSPMEKIHDVGZGS2T3XKEXHAVCNFSM6AAAAACGW4SEGGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTGOJUHE3TCNZVGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Well, not sure what you did test, but I can still repro the naming issue I mentioned |
@Eideren can you point which naming issue? |
The one I mentioned there as I said #2898 (comment) - Changing the name of a model asset in the editor changes the actual mesh that gets imported |
My mistake, overlooked that part. I’ve checked in updates, storing mesh ID as a key pointer in ModelAsset, which is set only once at time of creation and remains unchanged on renaming etc. @Eideren |
Fixed. It still reimports the mesh upon update from source but will not add extra materials @Eideren |
If a model has multiple materials assigned it ends up being split across two models |
Can you tell which mesh to look for in structure? And you mean if material has multiple materials its only applying first one? |
There are two cushions on the bed, one of them is of the same material as the bed, while the other has a different one assigned to it, both of them are part of the bed model in the original fbx file. |
Fixed material mapping mismatch between Stride and Assimp. |
There's another fairly obvious issue you've introduced with the latest changes, didn't even have to do anything this time around, import your model and test things out properly. |
Just tested, its loading fine for me, can you tell whats the problem? @Eideren |
It's visible on your screen, half of the furniture is not on the right spot in the prefab |
I see, its going in circle, if I try to match exact materials than meshs dont position correctly if set mesh then materials dont set up correctly, ideally classes should have set up keeping multiple material in mind, i can try some more but not sure. If its acceptable to have only one material material imported like it is now, it will require only more additional stepon users part to assign the extra material on mesh in user interface? |
If it's not a bug but a limitation in the design, sure, but that's not really the case here. You'll figure it out I'm sure ! |
This PR adds an “Split hierarchy” workflow for model import: when enabled, Stride imports one Model per mesh (named after the mesh) and automatically generates a Prefab that mirrors the FBX node hierarchy. When disabled, the import produces a single combined Model (no “(All)” suffix).
What’s in this PR-
Split hierarchy checkbox in Add asset → 3D model dialog.
Remembers last value per project.
Behavior (per mode)
Split hierarchy = ON
Imports N Model assets (one per mesh) named:
"-" (sanitized; auto-uniquified with -1, -2, …).
Creates a Prefab named " Prefab" that replicates the FBX hierarchy (one Entity per node, correct parent/child relationships).
Each Entity that hosts a mesh gets a ModelComponent referencing the corresponding per-mesh Model.
Each per-mesh Model keeps only the material(s) it actually uses\
Split hierarchy = OFF
Imports a single Model named exactly ""