Max clones #917
Max clones #917
Conversation
Add support for Geometries to BabylonEntities Add support for cloning to BabylonEntities Insert Cloning into the Max Export workflow Change the GLTFExport to support Geometries indirection
Add support for instanciated primitive into GLTF export
Add useClone check box to the exporter form Add useClone property to the MaxExporterParameters Update Signature to accept MaxParameter intstead of ExportParameter
Max and maya 2019 do not support c# v7. Avoid using default literal
There was a problem hiding this comment.
I understand the sentiment here in how we want to try to share geometry data across meshes that arent full instances of each other, but lets make sure that our architectural changes are appropriate, and not just purpose built, and that we're considering all scenarios.
| bool isShareMat = masterMeshPair.Key.materialId == null || (meshNode.NodeMaterial != null && meshNode.NodeMaterial.MaxMaterial.GetGuid().ToString().Equals(masterMeshPair.Value.NodeMaterial.MaxMaterial.GetGuid().ToString())); | ||
|
|
||
| BabylonNode n = isShareMat? | ||
| ExportInstanceMesh(scene, meshNode, babylonScene, masterMeshPair.Key, masterMeshPair.Value) : |
There was a problem hiding this comment.
The use of a ternary operator makes this code hard to understand at first glance. Can we just use if-else for readability here?
| // If there is already an exported mesh in the scene that shares this mesh's material, then export it as an instance. | ||
| if (masterMeshPair.Key.materialId == null | ||
| || (meshNode.NodeMaterial != null && meshNode.NodeMaterial.MaxMaterial.GetGuid().ToString().Equals(masterMeshPair.Value.NodeMaterial.MaxMaterial.GetGuid().ToString()))) | ||
| bool isShareMat = masterMeshPair.Key.materialId == null || (meshNode.NodeMaterial != null && meshNode.NodeMaterial.MaxMaterial.GetGuid().ToString().Equals(masterMeshPair.Value.NodeMaterial.MaxMaterial.GetGuid().ToString())); |
There was a problem hiding this comment.
| bool isShareMat = masterMeshPair.Key.materialId == null || (meshNode.NodeMaterial != null && meshNode.NodeMaterial.MaxMaterial.GetGuid().ToString().Equals(masterMeshPair.Value.NodeMaterial.MaxMaterial.GetGuid().ToString())); | |
| bool sharesMaterial = masterMeshPair.Key.materialId == null || (meshNode.NodeMaterial != null && meshNode.NodeMaterial.MaxMaterial.GetGuid().ToString().Equals(masterMeshPair.Value.NodeMaterial.MaxMaterial.GetGuid().ToString())); |
There was a problem hiding this comment.
for readibility. Character count is not at a premium here. ;)
| babylonMesh.metadata = ExportExtraAttributes(maxNode, babylonScene); | ||
|
|
||
| // Append userData to extras | ||
| ExportUserData(maxNode, babylonMesh); |
There was a problem hiding this comment.
Will this result in us having user data from the original mesh, and the clone on the exported clone?
There was a problem hiding this comment.
yes, data are duplicated
There was a problem hiding this comment.
Are we sure thats the intention here? I was under the assumption that user data is instance-specific, I'm not sure that we want to duplicate this data as well.
There was a problem hiding this comment.
Well, in max, Instance are a "pointer" to a mesh, and then share ALL the values and behavior. Who can clarify the initial intention ?
| { | ||
|
|
||
| [DataContract] | ||
| public class BabylonVertexData : IBabylonMeshData |
There was a problem hiding this comment.
If the goal here is to abstract the geometry information from the rest of the mesh information, should we remove the geometry-related fields from BabylonMesh, and instead use a facade to the BabylonGeometry when serializing?
There was a problem hiding this comment.
Agree for an SDK, but here i stick with the original design idea which is to have Entities dedicated to serialization. Babylon format, introduce the geometries fields late into version 2 (i guess remember), and for compatibility, the file format is keeping the both. David Catuhe or other Guru will be able to tell us if this is still in use into Babylon. My concern was, related to our first conversation, was to keep the change "minimal". I will be happy to explore this if you think it's mandatory.
There was a problem hiding this comment.
I'm not sure if I'm being clear.
Imagine we have two meshes, one original and a clone of that original mesh in scene:
when serialized even though we are using the IBabylonMeshData, interface, we are not adding any additional logic to the glTF serializer to handle serialization of meshes with GeometryData only, so I'm concerned that the second mesh may not be properly serialized, as we will be using those nulled fields.
Personally I don't believe that we should limit our design for backwards compatibility here. @deltakosh, your input would be appreciated as well, but targeting the latest stable release's capabilities seems like my preferred route. Otherwise if backwards compatibility is a larger concern, we can add a flag to use a facade to serialize the old geometry format, but internally we can still use geometryData.
If we decide to go all in with GeometryData, we should go all in and use this as our storage of geometry, else we're complicating our design with additional usecases which may or may not be valid.
I'm just concerned about how we're now changing where our data "lives" but not consistently, which may cause problems in the future.
There was a problem hiding this comment.
I put all (as far i know ) the mandatory logic in place inside the GLTF part of the exporter. I asked a lot of question inside the issue but no response :-), then i move forward and also provide sample and result at the end of the issue.
I'm agree that this enhancement is quite intrusive and this is why i put it as "experimental".
So, Let's continue this interesting discussion :-).
There was a problem hiding this comment.
Cool, missed ALLL of those changes, thanks. Since you're handling both cases relatively elegantly in the glTF exporter, I'm much less worried. Lets check this in and revisit if we run into any issues in the future.
| { | ||
| [DataMember] | ||
| public string materialId { get; set; } | ||
|
|
||
| [DataMember] | ||
| public string geometryId { get; set; } |
There was a problem hiding this comment.
How are we serializing this to glTF? Now we will have mesh "clones" with no geometry data and just a geometry index.
There was a problem hiding this comment.
GLTFExporter.Mesh is in charge.
I choose to abstract the geometry throught an interface
public interface IBabylonMeshData
{
float[] positions { get; set; }
float[] normals { get; set; }
float[] tangents { get; set; }
float[] uvs { get; set; }
float[] uvs2 { get; set; }
float[] uvs3 { get; set; }
float[] uvs4 { get; set; }
float[] uvs5 { get; set; }
float[] uvs6 { get; set; }
float[] colors { get; set; }
int[] matricesIndices { get; set; }
float[] matricesWeights { get; set; }
int[] indices { get; set; }
}
An then it's kind of transparent for the GLTF serializer to work with a mesh data OR geometry.
This is also partially respond to your previous comment.
Add experimental support for Cloning Mesh when instances do not share same material as master. Introduce Geometries support into Babylon Entities for the purpose. fix #835.