-
-
Notifications
You must be signed in to change notification settings - Fork 931
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
Change Assimp to Silk.NET.Assimp implementation #1719
Conversation
* 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
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.
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.
@@ -0,0 +1,294 @@ | |||
using System; |
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.
Please remember to add the copyright header to new files, the one that doesn't mention "Silicon Studio".
public static readonly Operation[] ConvertAssimpStackOperationCppToCs = new Operation[] | ||
{ | ||
Operation.Add, //aiStackOperation_Add | ||
Operation.Add3ds, //aiStackOperation_Add3ds |
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.
Weird indentation.
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.
Indeed weird. I guess github wrongly displays this file because when you open file everything is correct
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.
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.
Maybe the indentation is not normalized between tabs and spaces? I tend to favor spaces because this very reason.
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.
Yes this file had mixed tabs, Thanks!
//else if (hasBaseValue) | ||
//{ | ||
// computeColorNode = gcnew MaterialFloatComputeNode(baseValue); | ||
//} |
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.
Why is this commented out?
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.
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.
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.
gcnew
in C++/CLI is just the new
keyword in C#, i.e. instantiating a new instance managed by the Garbage Collector.
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.
Yes, but this is a C# file, so there's no use for it. Maybe used as some sort of reference?
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.
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.
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.
Commented code is from cpp MeshConverter. Out of curiosity, I cloned the xenko 1.4 source code and the code was still commented out
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.
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.
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.
I found original commit. Maybe It will be usefull for someone in the future: https://github.com/SiliconStudio/xenko//commit/d5ee29ea8efeceaa6eaf777ff903e4514f28c6f7
// TODO TEMP: Set a default diffuse model | ||
finalMaterial.Attributes.DiffuseModel = new MaterialDiffuseLambertModelFeature(); |
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.
Why a default fixed Lambertian diffuse model?
// TODO TEMP: Set a default specular model | ||
var specularModel = new MaterialSpecularMicrofacetModelFeature | ||
{ | ||
Fresnel = new MaterialSpecularMicrofacetFresnelSchlick(), | ||
Visibility = new MaterialSpecularMicrofacetVisibilityImplicit(), | ||
NormalDistribution = new MaterialSpecularMicrofacetNormalDistributionBlinnPhong() | ||
}; | ||
finalMaterial.Attributes.SpecularModel = specularModel; |
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.
Why a default fixed Blinn-Phong specular model?
} | ||
else if (textureType == TextureType.TextureTypeEmissive) | ||
{ | ||
// TODO: Add support |
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.
Would be nice to support all these texture map types, as they are pretty much standard today for PBR workflows.
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.
Yes, fully agree, importing textures with the model would be great.
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 looks like there is already an issue for this: #473
Question: should this be marked as a draft? There's lots of TO-DOs. |
I think if you add [WIP] to the title it is enough. But don't know. |
Thanks for all the feedback ^_^. Yea, I'll set to WIP PR for a while, but I hope to fix all the issues soon™ |
Regarding the spaces, https://github.com/stride3d/stride/blob/master/.editorconfig is present, but not sure if it is in the right location? |
To avoid this kind of problems with tabs and spaces I like to use this extension from Microsoft Labs. Highly recommended. |
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. |
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. |
Although I havent cross referenced anything myself, there were a bunch of TO-DOs in the C++ files as well. |
Why have you closed the PR? |
I am sorry, I force pushed the wrong branch on the remote by mistake and PR was automaticaly closed 😢 |
Alright, so I refactored and moved the classes to separate files and applied all the review suggestions I could. |
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.
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 😀.
/// <summary> | ||
/// Enumeration of the new Assimp's flags. | ||
/// </summary> | ||
|
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.
Don't hate me, but... blank line between XMLdoc and type definition 😁
@@ -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) |
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.
If these are new files, the header should be the one that doesn't mention Silicon Studio.
Look at this file for example.
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.
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
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.
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.
@@ -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) |
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.
Header. See above.
/// <summary> | ||
/// Enumeration of the different types of node in the new Assimp's material stack. | ||
/// </summary> | ||
public enum StackType |
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.
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.
return currentComposition; | ||
} | ||
|
||
public static ComputeTextureColor GenerateMaterialTextureNode(string vfsOutputPath, string sourceTextureFile, uint textureUVSetIndex, Vector2 textureUVscaling, TextureAddressMode addressModeU, TextureAddressMode addressModeV, Logger logger) |
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.
Long line of arguments. See above.
|
||
namespace Stride.Importer.Assimp | ||
{ | ||
public class MeshConverter |
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.
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.
} | ||
} | ||
|
||
public unsafe Model Convert(string inptuFilename, string outputFilename, bool deduplicateMaterials) |
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.
Still typo in inptuFilename
.
Logger = logger ?? GlobalLogger.GetLogger("Import Assimp"); | ||
} | ||
|
||
private void ResetConvertionData() |
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 named ResetConversionData
?
{ | ||
var animationCurve = new AnimationCurve<Vector3>(); | ||
|
||
// Switch to cubic implicit interpolation mode for Vector3 |
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.
Weird indentation.
1cb106c
to
97d66be
Compare
97d66be
to
84280b7
Compare
@Ethereal77 Not at all. That's the purpose of code review, to make sure everything is alright 😅 |
Nice attitude 🙂 |
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.
LGTM.
This is a first good step towards a more developed content pipeline for Stride.
namespace Stride.Importer.Assimp.Material | ||
{ | ||
/// <summary> | ||
/// Enumeration of the new Assimp's flags. |
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 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.
If there are no objections, I think changes are ready to merge 😊 |
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 ? |
And of course, thank you for going through this stuff @Jklawreszuk |
Also, we should thank @johang88 for his big effort 🫡🫡 . I'm impressed.
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. |
PR Details
This PR completes johang88 attempt to migrate from native C++ Assimp code to Silk.NET bindings (link to his PR).
Changes
ProcessMeshMaterial()
was incomplete so I wrote missing part.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