Change Assimp to Silk.NET.Assimp implementation #1719

Jklawreszuk · 2023-08-06T23:58:42Z

PR Details

This PR completes johang88 attempt to migrate from native C++ Assimp code to Silk.NET bindings (link to his PR).

Changes

I checked previous code and can tell it's a 1-1 recreation of the native MeshConverter. However ProcessMeshMaterial() was incomplete so I wrote missing part.
Also, my PR includes small changes that simplify MeshConverter and changes proposed by other contributors (those I could understand)

Overall, I have not noticed any regression or difference in the import of objects, but I would be happy to hear any feedback :)

Once, merged, I believe it will unlock the possibility of further development of the asset compiler for platforms other than Windows and work on glTF.

Related Issue

#1394
#360

My change requires a change to the documentation.
I have added tests to cover my changes.
Breaking change (fix or feature that would cause existing functionality to change).

* Converted Stride.Importer.Assimp to a C# library and changed assimp binding to Silk.Net.Assimp * Converted Stride.Importer.Common to a C# library as this did not contain any native code

Ethereal77

Good job. Hopefully we can also get rid of the C++ FBX importer in the near future.

I've commented some things to fix, and other things that should be good to note for future enhancements or refactorings.

Ethereal77 · 2023-08-07T10:43:57Z

sources/tools/Stride.Importer.Assimp/Material/MaterialStack.cs

@@ -0,0 +1,294 @@
+using System;


Please remember to add the copyright header to new files, the one that doesn't mention "Silicon Studio".

sources/tools/Stride.Importer.Assimp/Material/MaterialStack.cs

Ethereal77 · 2023-08-07T10:51:03Z

sources/tools/Stride.Importer.Assimp/Material/Materials.cs

+        public static readonly Operation[] ConvertAssimpStackOperationCppToCs = new Operation[]
+        {
+            Operation.Add,			//aiStackOperation_Add
+		    Operation.Add3ds,		//aiStackOperation_Add3ds


Weird indentation.

Indeed weird. I guess github wrongly displays this file because when you open file everything is correct

Maybe the indentation is not normalized between tabs and spaces? I tend to favor spaces because this very reason.

Yes this file had mixed tabs, Thanks!

sources/tools/Stride.Importer.Assimp/MeshConverter.cs

Ethereal77 · 2023-08-07T12:08:47Z

sources/tools/Stride.Importer.Assimp/MeshConverter.cs

+                //else if (hasBaseValue)
+                //{
+                //	computeColorNode = gcnew MaterialFloatComputeNode(baseValue);
+                //}


Why is this commented out?

I think that's commented C++/CLI code, since C# doesn't have a gcnew keyword. gcnew is specific to C++/CLI and C++/CX.

gcnew in C++/CLI is just the new keyword in C#, i.e. instantiating a new instance managed by the Garbage Collector.

Yes, but this is a C# file, so there's no use for it. Maybe used as some sort of reference?

Right now I'm not very versed in the material system. Maybe @tebjan knows better, but if there is a MaterialFloatComputeNode (or there were one) that maybe is a IComputeValue (does that type still exist?) it is/was useful somehow.
Or maybe it is merged into ComputeColor at this point and we should just create a regular ComputeColor with a single scalar value?
Anyway, that else if (hasBaseValue) indicates there was an use for it.

Commented code is from cpp MeshConverter. Out of curiosity, I cloned the xenko 1.4 source code and the code was still commented out

Then must be a half-implemented feature, or a remnant from some old code. As I said, I'm not very knowledgeable about the material system, so don't know what would be the uses of that even if it sounds useful somehow.

I found original commit. Maybe It will be usefull for someone in the future: https://github.com/SiliconStudio/xenko//commit/d5ee29ea8efeceaa6eaf777ff903e4514f28c6f7

Ethereal77 · 2023-08-07T12:11:44Z

sources/tools/Stride.Importer.Assimp/MeshConverter.cs

+                // TODO TEMP: Set a default diffuse model
+                finalMaterial.Attributes.DiffuseModel = new MaterialDiffuseLambertModelFeature();


Why a default fixed Lambertian diffuse model?

Ethereal77 · 2023-08-07T12:12:12Z

sources/tools/Stride.Importer.Assimp/MeshConverter.cs

+                // TODO TEMP: Set a default specular model
+                var specularModel = new MaterialSpecularMicrofacetModelFeature
+                {
+                    Fresnel = new MaterialSpecularMicrofacetFresnelSchlick(),
+                    Visibility = new MaterialSpecularMicrofacetVisibilityImplicit(),
+                    NormalDistribution = new MaterialSpecularMicrofacetNormalDistributionBlinnPhong()
+                };
+                finalMaterial.Attributes.SpecularModel = specularModel;


Why a default fixed Blinn-Phong specular model?

Ethereal77 · 2023-08-07T12:13:12Z

sources/tools/Stride.Importer.Assimp/MeshConverter.cs

+            }
+            else if (textureType == TextureType.TextureTypeEmissive)
+            {
+                // TODO: Add support


Would be nice to support all these texture map types, as they are pretty much standard today for PBR workflows.

Yes, fully agree, importing textures with the model would be great.

It looks like there is already an issue for this: #473

NicusorN5 · 2023-08-07T12:55:58Z

Question: should this be marked as a draft? There's lots of TO-DOs.

Ethereal77 · 2023-08-07T13:03:09Z

I think if you add [WIP] to the title it is enough. But don't know.

Jklawreszuk · 2023-08-07T13:06:58Z

Thanks for all the feedback ^_^. Yea, I'll set to WIP PR for a while, but I hope to fix all the issues soon™

VaclavElias · 2023-08-07T13:20:45Z

Regarding the spaces, https://github.com/stride3d/stride/blob/master/.editorconfig is present, but not sure if it is in the right location?

Ethereal77 · 2023-08-07T13:39:14Z

To avoid this kind of problems with tabs and spaces I like to use this extension from Microsoft Labs. Highly recommended.

tebjan · 2023-08-07T13:59:37Z

There is a link on top of this page that says "Convert to draft". But I would split the PR, first establish feature parity with the existing importer and only after that is merged, add additional features in a second PR.

But the workflow decision is of course up to the dev.

Ethereal77 · 2023-08-07T14:01:58Z

Yeah, of course. My review is only suggestions to point to typos, indentation, comments, etc. Then comments about what we should aim for in a future PR.

Doprez · 2023-08-07T14:03:09Z

Question: should this be marked as a draft? There's lots of TO-DOs.

Although I havent cross referenced anything myself, there were a bunch of TO-DOs in the C++ files as well.

Ethereal77 · 2023-08-07T19:20:45Z

Why have you closed the PR?

Jklawreszuk · 2023-08-07T19:55:04Z

I am sorry, I force pushed the wrong branch on the remote by mistake and PR was automaticaly closed 😢
At least I have a lesson to avoid that.

Jklawreszuk · 2023-08-07T22:13:40Z

Alright, so I refactored and moved the classes to separate files and applied all the review suggestions I could.
I think my changes are ready to be reviewed again.

Ethereal77

Ok. Some more comments. I hope you don't hate me too much for some of these ones 😁.
The indentation one about tabs vs spaces is just a hassle. I favor spaces because render the same in every editor (unlike tabs, which render depending on editor/user config). But I understand it is a matter of preference.
As Stride has no coding style guidelines defined formally anywhere apart from imitating what you can find in other files or the rules of .editorconfig, it's up to you.
I don't want to be an indentation nazi 😀.

Ethereal77 · 2023-08-08T10:23:05Z

sources/tools/Stride.Importer.Assimp/Material/Flags.cs

+    /// <summary>
+    /// Enumeration of the new Assimp's flags.
+    /// </summary>
+


Don't hate me, but... blank line between XMLdoc and type definition 😁

Ethereal77 · 2023-08-08T10:25:46Z

sources/tools/Stride.Importer.Assimp/Material/Flags.cs

@@ -0,0 +1,19 @@
+// Copyright (c) .NET Foundation and Contributors (https://dotnetfoundation.org/ & https://stride3d.net) and Silicon Studio Corp. (https://www.siliconstudio.co.jp)


If these are new files, the header should be the one that doesn't mention Silicon Studio.
Look at this file for example.

Hmm...I am not sure about that. Because I originaly separated classes/enums from this file:
https://github.com/stride3d/stride/blob/master/sources/tools/Stride.Assimp/MaterialStack.cs

If it comes from another file with a license header, forget about my comment then. For other files with no header or for new files, however, use the "simple" one.

Ethereal77 · 2023-08-08T10:26:05Z

sources/tools/Stride.Importer.Assimp/Material/MappingMode.cs

@@ -0,0 +1,16 @@
+// Copyright (c) .NET Foundation and Contributors (https://dotnetfoundation.org/ & https://stride3d.net) and Silicon Studio Corp. (https://www.siliconstudio.co.jp)


Header. See above.

sources/tools/Stride.Importer.Assimp/Material/MaterialStack.cs

Ethereal77 · 2023-08-08T10:31:38Z

sources/tools/Stride.Importer.Assimp/Material/StackType.cs

+    /// <summary>
+    /// Enumeration of the different types of node in the new Assimp's material stack.
+    /// </summary>
+    public enum StackType


Is not a more appropriate name for this StackElementType? It defines not the type of the material stack, but the type of an element in such stack.

Ethereal77 · 2023-08-08T10:42:02Z

sources/tools/Stride.Importer.Common/TextureLayerGenerator.cs

+            return currentComposition;
+        }
+
+        public static ComputeTextureColor GenerateMaterialTextureNode(string vfsOutputPath, string sourceTextureFile, uint textureUVSetIndex, Vector2 textureUVscaling, TextureAddressMode addressModeU, TextureAddressMode addressModeV, Logger logger)


Long line of arguments. See above.

Ethereal77 · 2023-08-08T10:45:58Z

sources/tools/Stride.Importer.Assimp/MeshConverter.cs

+
+namespace Stride.Importer.Assimp
+{
+	public class MeshConverter


Is it possible that you have normalized spaces to tabs? I ask because it seems Stride has favored the C# standard of using always indentation of 4 spaces.

Ethereal77 · 2023-08-08T10:47:53Z

sources/tools/Stride.Importer.Assimp/MeshConverter.cs

+			}
+		}
+
+		public unsafe Model Convert(string inptuFilename, string outputFilename, bool deduplicateMaterials)


Still typo in inptuFilename.

Ethereal77 · 2023-08-08T10:50:00Z

sources/tools/Stride.Importer.Assimp/MeshConverter.cs

+			Logger = logger ?? GlobalLogger.GetLogger("Import Assimp");
+		}
+
+		private void ResetConvertionData()


Shouldn't this be named ResetConversionData?

Ethereal77 · 2023-08-08T10:55:48Z

sources/tools/Stride.Importer.Assimp/MeshConverter.cs

+		{
+			var animationCurve = new AnimationCurve<Vector3>();
+
+				// Switch to cubic implicit interpolation mode for Vector3


Weird indentation.

Jklawreszuk · 2023-08-15T18:32:38Z

Ok. Some more comments. I hope you don't hate me too much for some of these ones 😁.

@Ethereal77 Not at all. That's the purpose of code review, to make sure everything is alright 😅
Thanks again for all suggestions. I think that's the good point to have identation consistency, so I changed for every file to space instead of tabs. Would be also nice to take care of .editorconfig file in future. Other than that I think my changes can be reviewed again (I hope didn't miss anything 😂)

VaclavElias · 2023-08-15T21:05:22Z

Nice attitude 🙂

Ethereal77

LGTM.
This is a first good step towards a more developed content pipeline for Stride.

Ethereal77 · 2023-08-16T08:18:33Z

sources/tools/Stride.Importer.Assimp/Material/Flags.cs

+namespace Stride.Importer.Assimp.Material
+{
+    /// <summary>
+    /// Enumeration of the new Assimp's flags.


A better explanation of this types would be nice.
It's ok however if you are not very familiar with graphics and/or meshes/Assimp. That is something that can be later expanded in a PR to improve documentation I think.

Jklawreszuk · 2023-08-18T16:13:59Z

If there are no objections, I think changes are ready to merge 😊

Eideren · 2023-08-18T23:02:02Z

Thanks a bunch for taking the time to look at this @Ethereal77, and @NicusorN5 for providing additional insight !

I'm guessing this one will be taken care of in another issue/pr @Jklawreszuk ?
https://github.com/stride3d/stride/pull/1719/files#r1285751424

Eideren · 2023-08-18T23:03:12Z

And of course, thank you for going through this stuff @Jklawreszuk

Jklawreszuk · 2023-08-18T23:49:09Z

Thanks a bunch for taking the time to look at this @Ethereal77, and @NicusorN5 for providing additional insight !

Also, we should thank @johang88 for his big effort 🫡🫡 . I'm impressed.

I'm guessing this one will be taken care of in another issue/pr @Jklawreszuk ?

Definitely, we need now to discuss what to do next with FBX importer, because having cpp compiler as requirement during building is pain and I think potential PR would cover additional changes to assimp importer.

johang88 and others added 9 commits July 27, 2023 20:29

Changed Assimp binding to Silk.Net.Assimp

19f80f3

* Converted Stride.Importer.Assimp to a C# library and changed assimp binding to Silk.Net.Assimp * Converted Stride.Importer.Common to a C# library as this did not contain any native code

Fixed dependencies in Stride.Assets.Models.csproj

2cf85c5

Fixed GetNumUVChannels

4311025

Assimp material stack

0b4f865

Fixed animation info

156c3ad

Fixed matrix conversion and other things

8656f7c

"Fixed" animation import of quaternions

73dbabf

Fixed broken Stride.Assets.Models.csproj

d4dc63f

Add missing logic in ProcessMeshMaterial, apply review changes

04bd38c

Ethereal77 reviewed Aug 7, 2023

View reviewed changes

Jklawreszuk changed the title ~~Change Assimp to Silk.NET.Assimp implementation~~ [WIP] Change Assimp to Silk.NET.Assimp implementation Aug 7, 2023

[Assimp] Changes after code-review, refactoring

77a15f8

Jklawreszuk closed this Aug 7, 2023

Jklawreszuk force-pushed the SilkAssimp branch from c6d467e to f33a0ee Compare August 7, 2023 19:12

Jklawreszuk changed the title ~~[WIP] Change Assimp to Silk.NET.Assimp implementation~~ Change Assimp to Silk.NET.Assimp implementation Aug 7, 2023

Jklawreszuk reopened this Aug 7, 2023

Ethereal77 reviewed Aug 8, 2023

View reviewed changes

Jklawreszuk force-pushed the SilkAssimp branch from 1cb106c to 97d66be Compare August 15, 2023 18:14

[Assimp] Fix formating

84280b7

Jklawreszuk force-pushed the SilkAssimp branch from 97d66be to 84280b7 Compare August 15, 2023 18:22

Jklawreszuk requested a review from Ethereal77 August 15, 2023 18:34

Ethereal77 reviewed Aug 16, 2023

View reviewed changes

Fix spacing

0227b98

Eideren merged commit 7b84fed into stride3d:master Aug 18, 2023

Jklawreszuk deleted the SilkAssimp branch August 18, 2023 23:29

Eideren added this to the 4.2 milestone Sep 26, 2023

gamingrobot mentioned this pull request Feb 10, 2024

Replace FBX C++/CLI importer with a cross-platform FBX library #1923

Closed

		// TODO TEMP: Set a default diffuse model
		finalMaterial.Attributes.DiffuseModel = new MaterialDiffuseLambertModelFeature();

		@@ -0,0 +1,19 @@
		// Copyright (c) .NET Foundation and Contributors (https://dotnetfoundation.org/ & https://stride3d.net) and Silicon Studio Corp. (https://www.siliconstudio.co.jp)

		@@ -0,0 +1,16 @@
		// Copyright (c) .NET Foundation and Contributors (https://dotnetfoundation.org/ & https://stride3d.net) and Silicon Studio Corp. (https://www.siliconstudio.co.jp)

Change Assimp to Silk.NET.Assimp implementation #1719

Change Assimp to Silk.NET.Assimp implementation #1719

Conversation

Jklawreszuk commented Aug 6, 2023

PR Details

Changes

Related Issue

Ethereal77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jklawreszuk Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicusorN5 commented Aug 7, 2023

Ethereal77 commented Aug 7, 2023

Jklawreszuk commented Aug 7, 2023

VaclavElias commented Aug 7, 2023

Ethereal77 commented Aug 7, 2023

tebjan commented Aug 7, 2023 • edited Loading

Ethereal77 commented Aug 7, 2023

Doprez commented Aug 7, 2023

Ethereal77 commented Aug 7, 2023

Jklawreszuk commented Aug 7, 2023

Jklawreszuk commented Aug 7, 2023

Ethereal77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jklawreszuk commented Aug 15, 2023 • edited Loading

VaclavElias commented Aug 15, 2023

Ethereal77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jklawreszuk commented Aug 18, 2023

Eideren commented Aug 18, 2023

Eideren commented Aug 18, 2023

Jklawreszuk commented Aug 18, 2023

Jklawreszuk Aug 7, 2023 •

edited

Loading

tebjan commented Aug 7, 2023 •

edited

Loading

Jklawreszuk commented Aug 15, 2023 •

edited

Loading