Jump to content

Problem with VertexData.Merge


Recommended Posts

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.

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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);        }
Link to comment
Share on other sites

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?

Link to comment
Share on other sites

  • 3 weeks later...

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.

Link to comment
Share on other sites

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.
Note: Your post will require moderator approval before it will be visible.

Reply to this topic...

×   Pasted as rich text.   Paste as plain text instead

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.


  • Recently Browsing   0 members

    • No registered users viewing this page.
  • Create New...