Initial pass of fix for inconsistent behavior of punctual lights when round-tripped via glTF. Made fix to light orientation when exported from a right hand scene via proper calculation of node yaw angle, added code pathway to allow for lights to "reuse" parent node if it is the sole child (indicative of glTF round trip, since a parent node is generated on import), modified glTF extension contract to allow for returning a null node via promise, and allowing that null node to propagate to extension handler caller function.

serializers/src/glTF/2.0/Extensions/KHR_lights_punctual.ts

@@ -1,5 +1,7 @@
 import { SpotLight } from "babylonjs/Lights/spotLight";
-import { Vector3, Color3, Quaternion } from "babylonjs/Maths/math";
+
+import { Nullable } from "babylonjs/types";
+import { Vector3, Color3, Quaternion, Matrix, TmpVectors } from "babylonjs/Maths/math";
 import { Light } from "babylonjs/Lights/light";
 import { Node } from "babylonjs/node";
 import { ShadowLight } from "babylonjs/Lights/shadowLight";
@@ -9,6 +11,7 @@ import { IGLTFExporterExtensionV2 } from "../glTFExporterExtension";
 import { _Exporter } from "../glTFExporter";
 import { Logger } from "babylonjs/Misc/logger";
 import { _GLTFUtilities } from "../glTFUtilities";
+import { DirectionalLight } from 'babylonjs';
 
 const NAME = "KHR_lights_punctual";
 
@@ -81,9 +84,9 @@ export class KHR_lights_punctual implements IGLTFExporterExtensionV2 {
      * @param babylonNode BabylonJS node
      * @returns nullable INode promise
      */
-    public postExportNodeAsync(context: string, node: INode, babylonNode: Node): Promise<INode> {
+    public postExportNodeAsync(context: string, node: Nullable<INode>, babylonNode: Node, nodeMap?: {[key: number]: number}): Promise<Nullable<INode>> {
         return new Promise((resolve, reject) => {
-            if (babylonNode instanceof ShadowLight) {
+            if ( node && babylonNode instanceof ShadowLight) {
                 const babylonLight: ShadowLight = babylonNode;
                 let light: ILight;
 
@@ -106,7 +109,7 @@ export class KHR_lights_punctual implements IGLTFExporterExtensionV2 {
                     }
                     if (lightType !== LightType.POINT) {
                         const localAxis = babylonLight.direction;
-                        const yaw = -Math.atan2(localAxis.z, localAxis.x) + Math.PI / 2;
+                        const yaw = -Math.atan2(localAxis.z * (this._exporter._babylonScene.useRightHandedSystem ? -1 : 1), localAxis.x) + Math.PI / 2;
                         const len = Math.sqrt(localAxis.x * localAxis.x + localAxis.z * localAxis.z);
                         const pitch = -Math.atan2(localAxis.y, len);
                         const lightRotationQuaternion = Quaternion.RotationYawPitchRoll(yaw, pitch, 0);
@@ -158,13 +161,64 @@ export class KHR_lights_punctual implements IGLTFExporterExtensionV2 {
 
                     this._lights.lights.push(light);
 
-                    if (node.extensions == null) {
-                        node.extensions = {};
-                    }
                     const lightReference: ILightReference = {
                         light: this._lights.lights.length - 1
                     };
 
+                    // Avoid duplicating the Light's parent node if possible.
+                    let parentBabylonNode = babylonNode.parent;
+                    if (parentBabylonNode && parentBabylonNode.getChildren().length == 1) {
+                        let parentNode = this._exporter._nodes[nodeMap![parentBabylonNode.uniqueId]];
+                        if (parentNode){
+                            let parentNodeLocalMatrix = TmpVectors.Matrix[0];
+                            let parentInvertNodeLocalMatrix = TmpVectors.Matrix[1];
+                            let parentNodeLocalTranslation = parentNode.translation ? new Vector3(parentNode.translation[0], parentNode.translation[1], parentNode.translation[2]) : Vector3.Zero();
+                            let parentNodeLocalRotation = parentNode.rotation ? new Quaternion(parentNode.rotation[0], parentNode.rotation[1], parentNode.rotation[2], parentNode.rotation[3]) : Quaternion.Identity();
+                            let parentNodeLocalScale = parentNode.scale ? new Vector3(parentNode.scale[0], parentNode.scale[1], parentNode.scale[2]) : Vector3.One();
+
+                            Matrix.ComposeToRef(parentNodeLocalScale, parentNodeLocalRotation, parentNodeLocalTranslation, parentNodeLocalMatrix);
+                            parentNodeLocalMatrix.invertToRef(parentInvertNodeLocalMatrix);
+
+                            // Convert light local matrix to local matrix relative to grandparent, facing -Z
+                            let lightLocalMatrix = TmpVectors.Matrix[2];
+                            let nodeLocalTranslation = node.translation ? new Vector3(node.translation[0], node.translation[1], node.translation[2]) : Vector3.Zero();
+
+                            // Undo directional light positional offset
+                            if (babylonLight instanceof DirectionalLight){
+                                nodeLocalTranslation.subtractInPlace(this._exporter._babylonScene.useRightHandedSystem ? babylonLight.direction : _GLTFUtilities._GetRightHandedPositionVector3(babylonLight.direction));
+                            }
+                            let nodeLocalRotation = this._exporter._babylonScene.useRightHandedSystem ? Quaternion.Identity() : new Quaternion(0, 1, 0, 0);
+                            if (node.rotation){
+                                nodeLocalRotation.multiplyInPlace(new Quaternion(node.rotation[0], node.rotation[1], node.rotation[2], node.rotation[3]));
+                            }
+                            let nodeLocalScale = node.scale ? new Vector3(node.scale[0], node.scale[1], node.scale[2]) : Vector3.One();
+
+                            Matrix.ComposeToRef(nodeLocalScale, nodeLocalRotation, nodeLocalTranslation, lightLocalMatrix);
+                            lightLocalMatrix.multiplyToRef(parentInvertNodeLocalMatrix, lightLocalMatrix);
+                            let parentNewScale = TmpVectors.Vector3[0];
+                            let parentNewRotationQuaternion = TmpVectors.Quaternion[0];
+                            let parentNewTranslation = TmpVectors.Vector3[1];
+
+                            lightLocalMatrix.decompose(parentNewScale, parentNewRotationQuaternion, parentNewTranslation);
+                            parentNode.scale = parentNewScale.asArray();
+                            parentNode.rotation = parentNewRotationQuaternion.asArray();
+                            parentNode.translation = parentNewTranslation.asArray();
+
+                            if (parentNode.extensions == null) {
+                                parentNode.extensions = {};
+                            }
+                            parentNode.extensions[NAME] = lightReference;
+
+                            // Do not export the original node
+                            resolve(undefined);
+                            return;
+                        }
+                    }
+
+                    if (node.extensions == null) {
+                        node.extensions = {};
+                    }
+
                     node.extensions[NAME] = lightReference;
                 }
             }

serializers/src/glTF/2.0/glTFExporter.ts

@@ -67,7 +67,7 @@ export class _Exporter {
     /**
      * Stores all the generated nodes, which contains transform and/or mesh information per node
      */
-    private _nodes: INode[];
+    public _nodes: INode[];
     /**
      * Stores all the generated glTF scenes, which stores multiple node hierarchies
      */
@@ -119,7 +119,7 @@ export class _Exporter {
     /**
      * Stores a map of the unique id of a node to its index in the node array
      */
-    private _nodeMap: { [key: number]: number };
+    public _nodeMap: { [key: number]: number };
 
     /**
      * Specifies if the source Babylon scene was left handed, and needed conversion.
@@ -152,7 +152,7 @@ export class _Exporter {
     private static _ExtensionNames = new Array<string>();
     private static _ExtensionFactories: { [name: string]: (exporter: _Exporter) => IGLTFExporterExtensionV2 } = {};
 
-    private _applyExtension<T>(node: T, extensions: IGLTFExporterExtensionV2[], index: number, actionAsync: (extension: IGLTFExporterExtensionV2, node: T) => Promise<Nullable<T>> | undefined): Promise<Nullable<T>> {
+    private _applyExtension<T>(node: Nullable<T>, extensions: IGLTFExporterExtensionV2[], index: number, actionAsync: (extension: IGLTFExporterExtensionV2, node: Nullable<T>) => Promise<Nullable<T>> | undefined): Promise<Nullable<T>> {
         if (index >= extensions.length) {
             return Promise.resolve(node);
         }
@@ -163,10 +163,10 @@ export class _Exporter {
             return this._applyExtension(node, extensions, index + 1, actionAsync);
         }
 
-        return currentPromise.then((newNode) => this._applyExtension(newNode || node, extensions, index + 1, actionAsync));
+        return currentPromise.then((newNode) => this._applyExtension(newNode, extensions, index + 1, actionAsync));
     }
 
-    private _applyExtensions<T>(node: T, actionAsync: (extension: IGLTFExporterExtensionV2, node: T) => Promise<Nullable<T>> | undefined): Promise<Nullable<T>> {
+    private _applyExtensions<T>(node: Nullable<T>, actionAsync: (extension: IGLTFExporterExtensionV2, node: Nullable<T>) => Promise<Nullable<T>> | undefined): Promise<Nullable<T>> {
         var extensions: IGLTFExporterExtensionV2[] = [];
         for (const name of _Exporter._ExtensionNames) {
             extensions.push(this._extensions[name]);
@@ -175,7 +175,7 @@ export class _Exporter {
         return this._applyExtension(node, extensions, 0, actionAsync);
     }
 
-    public _extensionsPreExportTextureAsync(context: string, babylonTexture: Texture, mimeType: ImageMimeType): Promise<Nullable<BaseTexture>> {
+    public _extensionsPreExportTextureAsync(context: string, babylonTexture: Nullable<Texture>, mimeType: ImageMimeType): Promise<Nullable<BaseTexture>> {
         return this._applyExtensions(babylonTexture, (extension, node) => extension.preExportTextureAsync && extension.preExportTextureAsync(context, node, mimeType));
     }
 
@@ -183,11 +183,11 @@ export class _Exporter {
         return this._applyExtensions(meshPrimitive, (extension, node) => extension.postExportMeshPrimitiveAsync && extension.postExportMeshPrimitiveAsync(context, node, babylonSubMesh, binaryWriter));
     }
 
-    public _extensionsPostExportNodeAsync(context: string, node: INode, babylonNode: Node): Promise<Nullable<INode>> {
-        return this._applyExtensions(node, (extension, node) => extension.postExportNodeAsync && extension.postExportNodeAsync(context, node, babylonNode));
+    public _extensionsPostExportNodeAsync(context: string, node: Nullable<INode>, babylonNode: Node, nodeMap?: {[key: number]: number}): Promise<Nullable<INode>> {
+        return this._applyExtensions(node, (extension, node) => extension.postExportNodeAsync && extension.postExportNodeAsync(context, node, babylonNode, nodeMap));
     }
 
-    public _extensionsPostExportMaterialAsync(context: string, material: IMaterial, babylonMaterial: Material): Promise<Nullable<IMaterial>> {
+    public _extensionsPostExportMaterialAsync(context: string, material: Nullable<IMaterial>, babylonMaterial: Material): Promise<Nullable<IMaterial>> {
         return this._applyExtensions(material, (extension, node) => extension.postExportMaterialAsync && extension.postExportMaterialAsync(context, node, babylonMaterial));
     }
 
@@ -1477,8 +1477,8 @@ export class _Exporter {
             if (!this._options.shouldExportNode || this._options.shouldExportNode(babylonNode)) {
                 promiseChain = promiseChain.then(() => {
                     let convertToRightHandedSystem = this._convertToRightHandedSystemMap[babylonNode.uniqueId];
-                    return this.createNodeAsync(babylonNode, binaryWriter, convertToRightHandedSystem).then((node) => {
-                        const promise = this._extensionsPostExportNodeAsync("createNodeAsync", node, babylonNode);
+                    return this.createNodeAsync(babylonNode, binaryWriter, convertToRightHandedSystem, nodeMap).then((node) => {
+                        const promise = this._extensionsPostExportNodeAsync("createNodeAsync", node, babylonNode, nodeMap);
                         if (promise == null) {
                             Tools.Warn(`Not exporting node ${babylonNode.name}`);
                             return Promise.resolve();
@@ -1530,7 +1530,7 @@ export class _Exporter {
      * @param convertToRightHandedSystem Converts the values to right-handed
      * @returns glTF node
      */
-    private createNodeAsync(babylonNode: Node, binaryWriter: _BinaryWriter, convertToRightHandedSystem: boolean): Promise<INode> {
+    private createNodeAsync(babylonNode: Node, binaryWriter: _BinaryWriter, convertToRightHandedSystem: boolean, nodeMap?: {[key: number]: number}): Promise<INode> {
         return Promise.resolve().then(() => {
             // create node to hold translation/rotation/scale and the mesh
             const node: INode = {};

serializers/src/glTF/2.0/glTFExporterExtension.ts

@@ -1,5 +1,6 @@
 import { ImageMimeType, IMeshPrimitive, INode, IMaterial, ITextureInfo } from "babylonjs-gltf2interface";
 import { Node } from "babylonjs/node";
+import { Nullable } from "babylonjs/types"
 
 import { Texture } from "babylonjs/Materials/Textures/texture";
 import { SubMesh } from "babylonjs/Meshes/subMesh";
@@ -25,7 +26,7 @@ export interface IGLTFExporterExtensionV2 extends IGLTFExporterExtension, IDispo
      * @param mimeType The mime-type of the generated image
      * @returns A promise that resolves with the exported texture
      */
-    preExportTextureAsync?(context: string, babylonTexture: Texture, mimeType: ImageMimeType): Promise<Texture>;
+    preExportTextureAsync?(context: string, babylonTexture: Nullable<Texture>, mimeType: ImageMimeType): Promise<Texture>;
 
     /**
      * Define this method to get notified when a texture info is created
@@ -43,7 +44,7 @@ export interface IGLTFExporterExtensionV2 extends IGLTFExporterExtension, IDispo
      * @param binaryWriter glTF serializer binary writer instance
      * @returns nullable IMeshPrimitive promise
      */
-    postExportMeshPrimitiveAsync?(context: string, meshPrimitive: IMeshPrimitive, babylonSubMesh: SubMesh, binaryWriter: _BinaryWriter): Promise<IMeshPrimitive>;
+    postExportMeshPrimitiveAsync?(context: string, meshPrimitive: Nullable<IMeshPrimitive>, babylonSubMesh: SubMesh, binaryWriter: _BinaryWriter): Promise<IMeshPrimitive>;
 
     /**
      * Define this method to modify the default behavior when exporting a node
@@ -52,7 +53,7 @@ export interface IGLTFExporterExtensionV2 extends IGLTFExporterExtension, IDispo
      * @param babylonNode BabylonJS node
      * @returns nullable INode promise
      */
-    postExportNodeAsync?(context: string, node: INode, babylonNode: Node): Promise<INode>;
+    postExportNodeAsync?(context: string, node: Nullable<INode>, babylonNode: Node, nodeMap?:{[key: number]: number}): Promise<Nullable<INode>>;
 
     /**
      * Define this method to modify the default behavior when exporting a material
@@ -60,7 +61,7 @@ export interface IGLTFExporterExtensionV2 extends IGLTFExporterExtension, IDispo
      * @param babylonMaterial BabylonJS material
      * @returns nullable IMaterial promise
      */
-    postExportMaterialAsync?(context: string, node: IMaterial, babylonMaterial: Material): Promise<IMaterial>;
+    postExportMaterialAsync?(context: string, node: Nullable<IMaterial>, babylonMaterial: Material): Promise<IMaterial>;
 
     /**
      * Define this method to return additional textures to export from a material