Skip to content

Conversation

noa7
Copy link
Contributor

@noa7 noa7 commented Sep 17, 2025

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 ""

. 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)
@Eideren Eideren changed the title Split Heirarchy Feature Complete feat: Hierarchical Model Importer Sep 17, 2025
@Eideren Eideren linked an issue Sep 17, 2025 that may be closed by this pull request
@Eideren Eideren self-requested a review September 17, 2025 08:49
@noa7
Copy link
Contributor Author

noa7 commented Sep 17, 2025

@noa7 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@dotnet-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@dotnet-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@dotnet-policy-service agree company="Microsoft"

Contributor License Agreement

@dotnet-policy-service agree

@Eideren
Copy link
Collaborator

Eideren commented Sep 18, 2025

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 RootNode created and a weird parenting issue, see the following comparison:
image

Here's the file I used for testing - this is not CC0, do not use in projects.

@noa7
Copy link
Contributor Author

noa7 commented Sep 19, 2025

Thanks for sharing the test file. I’ve checked in updates, and both issues are now fixed:

  1. The prefab in Stride now exactly replicates the FBX hierarchy as seen in Blender- no extra "RootNode"
  2. Material assignments on individual sub-meshes remain intact after import

@Eideren

@Eideren
Copy link
Collaborator

Eideren commented Sep 19, 2025

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.

@noa7
Copy link
Contributor Author

noa7 commented Sep 19, 2025

are you on latest? it imports fine my side

image

@Eideren
Copy link
Collaborator

Eideren commented Sep 19, 2025

The vertices are at the right position in world space, but each mesh's pivot point is not right below the model, like it is in the file, it is set to the origin.
For example, if you only place the bookends model in the world, setting its entity's position to 0,0,0 should center the object around the origin
image
image
The prefab entity for a given model should have its position moved to match wherever the model is in the imported file, the model asset itself should not have any transformation applied.

@noa7
Copy link
Contributor Author

noa7 commented Sep 21, 2025

Makes sense, thanks for pointing out, This is now fixed.

With this latest-

  1. Only the material applied on mesh are visible in inspector
  2. The relative positiong of each item in prefab based of entity data in scene by storing each mesh's TRS data at its nodeinfo object that prefab is constructed with. And each mesh pivot in scene matches to that in source model
Capture Capture1

@Eideren

Copy link
Collaborator

@Eideren Eideren left a 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)
Copy link
Collaborator

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)

Copy link
Contributor Author

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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unused

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, no longer needed

Comment on lines 217 to 229
// 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
}
}
Copy link
Collaborator

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 ?

Copy link
Contributor Author

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)
Copy link
Collaborator

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment on lines 247 to 263
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);
}
}
Copy link
Collaborator

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 ?

Copy link
Contributor Author

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

Comment on lines 280 to 288
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);
}
Copy link
Collaborator

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:
image
Here's the file

Copy link
Contributor Author

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

Comment on lines 356 to 357
if (!nodeNameToIndex.TryGetValue(meshInfo.NodeName, out var nodeIndex))
continue;
Copy link
Collaborator

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 ?

Copy link
Contributor Author

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++;
Copy link
Collaborator

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

Copy link
Contributor Author

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

Comment on lines 401 to 405
var firstName = firstNode.Name ?? string.Empty;
bool looksLikeWrapper =
string.IsNullOrWhiteSpace(firstName)
|| firstName.Equals(baseName, StringComparison.OrdinalIgnoreCase)
|| firstName.Equals("RootNode", StringComparison.OrdinalIgnoreCase);
Copy link
Collaborator

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@noa7
Copy link
Contributor Author

noa7 commented Sep 23, 2025

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.

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

@Eideren
Copy link
Collaborator

Eideren commented Sep 23, 2025

Throws in ResetMaterialsOnPrefabItems when importing into a folder, that method definitely required a second cleanup pass anyway.
How about you replace those strings and pass references or ids around, this goes for other methods too. So many string manipulation and lookups, it's no wonder the whole thing throws if we so much as look at it.

@noa7
Copy link
Contributor Author

noa7 commented Sep 26, 2025

No more string manipulations by storing source mesh name variable in ModelAsset, checked in latest @Eideren

@Eideren
Copy link
Collaborator

Eideren commented Sep 27, 2025

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.

@noa7
Copy link
Contributor Author

noa7 commented Sep 27, 2025

Yes, I got that, but you haven't gone through the review I made below the message I linked here #2898 (comment) This stuff:

I really can't see that list my side of github :)

image

@Eideren
Copy link
Collaborator

Eideren commented Sep 27, 2025

Ah, okay, my bad, that's on me. Give me a sec.

Copy link
Collaborator

@Eideren Eideren left a 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this comment

Copy link
Contributor Author

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?
Copy link
Collaborator

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

Copy link
Contributor Author

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)
Copy link
Collaborator

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

Copy link
Contributor Author

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;
Copy link
Collaborator

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 ?

Copy link
Contributor Author

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!

Copy link
Contributor Author

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)
Copy link
Collaborator

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor Author

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();
Copy link
Collaborator

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines 259 to 269
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));
Copy link
Collaborator

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@noa7 noa7 marked this pull request as ready for review September 28, 2025 13:14
Copy link
Collaborator

@Eideren Eideren left a 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.

@noa7
Copy link
Contributor Author

noa7 commented Oct 12, 2025

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

@Eideren
Copy link
Collaborator

Eideren commented Oct 12, 2025

Did you test your changes ?

@noa7
Copy link
Contributor Author

noa7 commented Oct 12, 2025 via email

@Eideren
Copy link
Collaborator

Eideren commented Oct 12, 2025

Well, not sure what you did test, but I can still repro the naming issue I mentioned

@noa7
Copy link
Contributor Author

noa7 commented Oct 12, 2025

@Eideren can you point which naming issue?

@Eideren
Copy link
Collaborator

Eideren commented Oct 13, 2025

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

@noa7
Copy link
Contributor Author

noa7 commented Oct 13, 2025

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

@Eideren
Copy link
Collaborator

Eideren commented Oct 13, 2025

If I select this option from the context menu, the model has a bunch of new entries created for its materials instead of just the ones used
image

@noa7
Copy link
Contributor Author

noa7 commented Oct 14, 2025

Fixed. It still reimports the mesh upon update from source but will not add extra materials @Eideren

@Eideren
Copy link
Collaborator

Eideren commented Oct 14, 2025

If a model has multiple materials assigned it ends up being split across two models
Untitled.zip - assigned a different material for one of the cushion on the bed.
See what I mean by saying you should test your PR more thoroughly, we shouldn't be having this much back and forth.

@noa7
Copy link
Contributor Author

noa7 commented Oct 14, 2025

Can you tell which mesh to look for in structure? And you mean if material has multiple materials its only applying first one?

@Eideren
Copy link
Collaborator

Eideren commented Oct 14, 2025

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.
When imported as a split, the bed gets separated into two model asset, one for the bed and cushion of material A and another with only the other cushion with material B

@noa7
Copy link
Contributor Author

noa7 commented Oct 17, 2025

Fixed material mapping mismatch between Stride and Assimp.
Stride importer has one material per mesh, while Assimp exports multiple submeshes for multi-material meshes is what makes this tricky. Updated the mesh conversion logic to merge these correctly so materials now display correctly. @Eideren

@Eideren
Copy link
Collaborator

Eideren commented Oct 18, 2025

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.

@noa7
Copy link
Contributor Author

noa7 commented Oct 18, 2025

Just tested, its loading fine for me, can you tell whats the problem? @Eideren
image

@Eideren
Copy link
Collaborator

Eideren commented Oct 18, 2025

It's visible on your screen, half of the furniture is not on the right spot in the prefab

@noa7
Copy link
Contributor Author

noa7 commented Oct 18, 2025

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?

@Eideren
Copy link
Collaborator

Eideren commented Oct 18, 2025

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 !

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hierarchical FBX Importer

2 participants