Problem with VertexData.Merge

[JCPalmer]

If Positions, Normals, etc can now be Float32Arrays, VertexData.merge() fails.  This is because receiving mesh's array does not have a push function.  This is one of the only differences between typed arrays and Javascript arrays:  typed arrays are of fixed size at the time of construction.

 

Think a merge that will always work is one that:

• computes the new size of the combination,
• instances a new array of the final size
• references receiving array by index

Think we should be creating a new method like, _mergeElement, that does this and returns the new array which should replace the old one.  The current structure of Merge() is quite long,  adding this extra stuff, would increase that.  having a sub method might actually have less code.

 

I double checked for references to push().  They are contained to the canned geometry / ribbon methods, so it looks like this is the only instance of this issue.  I have so many unfinished things that I cannot address this myself right now.  Wanted to bring it up, so anyone else who encountered it would know it was a known issue.

[RaananW]

That's an interesting issue.

I find your suggestion to be the best - return a new vertex data object that can be assigned to a new our existing geometry. Unless someone else jumps in I'll try finding the time later to correct this.

[GameMonetize]

Agree with this! If no one can do it, I'll do it

[JCPalmer]

Here is a candidate for the new method.  Functionality should be the same.  Please review:

        private _mergeElement(source : number[] | Float32Array, other : number[] | Float32Array) : number[] | Float32Array{            if (!other) return source;            if (!source) return other;                        var len = other.length + source.length;            var ret = (source instanceof Float32Array) ? new Float32Array(len) : new Array<number>(len);            var z = 0;            for (var i = 0, len = source.length; i < len; i++){                ret[z++] = source[i];            }            for (var i = 0, len = other.length; i < len; i++){                ret[z++] = other[i];            }            return ret;                    }

I have a test using Float32Array already to go once this in in.

[GameMonetize]

green light for me!

[JCPalmer]

I changed merge(), compiled & tested.  GitHub will not allow me to merge.  Here is the other part:

        public merge(other: VertexData): void {            if (other.indices) {                if (!this.indices) {                    this.indices = [];                }                var offset = this.positions ? this.positions.length / 3 : 0;                for (var index = 0; index < other.indices.length; index++) {                    this.indices.push(other.indices[index] + offset);                }            }            this.positions = this._mergeElement(this.positions, other.positions);            this.normals   = this._mergeElement(this.normals  , other.normals  );            this.uvs       = this._mergeElement(this.uvs      , other.uvs      );            this.uvs2      = this._mergeElement(this.uvs2     , other.uvs2     );            this.uvs3      = this._mergeElement(this.uvs3     , other.uvs3     );            this.uvs4      = this._mergeElement(this.uvs4     , other.uvs4     );            this.uvs5      = this._mergeElement(this.uvs5     , other.uvs5     );            this.uvs6      = this._mergeElement(this.uvs6     , other.uvs6     );            this.colors    = this._mergeElement(this.colors   , other.colors   );            this.matricesIndices      = this._mergeElement(this.matricesIndices     , other.matricesIndices     );            this.matricesWeights      = this._mergeElement(this.matricesWeights     , other.matricesWeights     );            this.matricesIndicesExtra = this._mergeElement(this.matricesIndicesExtra, other.matricesIndicesExtra);            this.matricesWeightsExtra = this._mergeElement(this.matricesWeightsExtra, other.matricesWeightsExtra);        }

[RaananW]

typed arrays have a set method (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray/set) , so maybe something like this:
 

if(source instanceof Float32Array) {   var ret = new Float32Array(len);   res.set(source);   res.set(other);   return res;} else {   return source.concat(other)}

[JCPalmer]

I have not used set, or concat for that matter.  Using commands should written in a compile language, should be faster than looping in an interpreter.  Think you need to specify an offset for the 2nd set, though

 

Time to re-clone my local repo is just not available right now.  I am pretty sure no one has even figured out the option that gets them in trouble even exists.  Should not be a high priority to fix.  They could trigger it using cpu skinning, but who is likely to merge once rendering has started, and is also cpu skinning?

[JCPalmer]

I'm baaaaaaaaack (well almost, gulp not on my new re-cloned repo yet).  I have made the changes shown above to Merge.  For _mergeElement(), I did some of the stuff Raanan brought up, here:

        private _mergeElement(source : number[] | Float32Array, other : number[] | Float32Array) : number[] | Float32Array{            if (!other ) return source;            if (!source) return other;                        var len = other.length + source.length;            var isSrcTypedArray = source instanceof Float32Array;            var isOthTypedArray = other  instanceof Float32Array;                        // use non-loop method when the source is Float32Array            if(isSrcTypedArray) {               var ret32 = new Float32Array(len);               ret32.set(source);               ret32.set(other, source.length);               return ret;                            }else{                 var ret = [];                ret.concat(<number[]> source);                // can only use non-loop, concat, method when other is also number[]                 if (!isOthTypedArray) {                    return ret.concat(<number[]> other);                                    } else {                    for (var i = 0, len = other.length; i < len; i++){                        ret.push(other[i]);                    }                    return ret;                }            }             }

When the source is already Float32Array, it is no problem for set() if other is either Float32Array or number[].  The same is not true for number.concat().  Other must also be number[].

 

Also, the original code created new arrays.  I thought a new array should also be created no matter what.  I am thinking a bunch of clones might be being merged, and not sure what would happen.  Not a problem for Float32Array, since a new array must be made.  Raanan's code for number[] seemed too aggressive writing without creating a new array.

 

I also changed the new serialize() method.  It did not have the matrice elements to do influencers > 4.  Also reminds me, might not Mesh.parse() create Float32Arrays right off the bat?  No looping is required, just new Float32Array([1,2,3,4]).  The next version of tower of Babel generates code like this.

[JCPalmer]

Actually, I changed Mesh.merge() to handle clones, so I should know know what happens, but its Sunday.  I do not like to make important decisions on Sunday.