I have no idea…
Are you able to start the tests locally and see if the same error pops up?
Also, the errors from the visualization tests were strange, I just restarted the failing steps. Let’s see if there are still some problems.
1 Like
It did not help…
So, are you able to test locally?
Let’s also see if @RaananW has any idea why it fails.
1 Like
looking now
I’ll have to pull it to check what happened. seems like a compilation issue.
have you merged from master lately?
1 Like
FYI - this is a runtime issue, so it won’t be detected in the build process. You can see the problem here as well:
prova | Babylon.js Playground (babylonjs.com)
Seems like the functions are not injected correctly for some reason
1 Like
Yes, and the error still occurs.
Yes, it is merged very recently (see the rebase on the PR)
It must be because I changed Vector to extend Array, which is need to achieve the same level of performance (as @Deltakosh mentioned).
Fixing this might need some changes to the build system.
In addition, Typescript is complaining about the usage of super.length which will need to be fixed to get maximum performance.
Maybe targeting a more recent ES version (e.g. es6)? Doing so could allow for proper class extension.
Sadly not going to happen any time soon, but - why is the es version important here?
1 Like
I thought maybe if the compiled code has classes, it would properly extend Array.
I was just throwing an idea out though, since the issue is very perplexing.
checking something now
1 Like
The main issue is your re-definition of the length variable. Array has length as a member (or, one might say - a getter/setter). but you have it defined as a function. Probably to maintain back-compat, but this prevents vectors from working correctly. length will stay a member and not turn into a function.
1 Like
Interesting. Keeping length as a function is needed for backward compatibility. Perhaps it could change to being a getter? If so, it would have to be delivered with 7.0 since it is a breaking change.
Is there a way to fix the other issues with Vector regardless of the issue with length?
we don’t break between versions. One of Babylon’s USPs.
What other issues are there?
1 Like
Getting all the methods to be added? Once that is done the perf of the new changes can be analyzed.
oh, the methods are there, at least when i checked locally. the main issue is length, which fails a lot of different functions. let’s solve this problem and then take it from there
It looks like when creating a new Vector2 instance, the compiled code just creates a new Array.
I looked into the built code more, and found the Vector constructor:
The function is broken though. Array.apply (_super.apply) returns an Array which gets returned by the constructor. This means that a Vector instance will not have Vector.prototype in its prototype chain. Any functions added to the Vector prototype will not exist on the instance.
If _this is removed (i.e. Array.apply is called on this but the result of the Array.apply is not returned), Array is not inherited. This also means no arguments are captured from the constructor.
If instead of returning _this, Object.assign(this, _this) is added, the arguments from the constructor are assigned as an object. Array.prototype is not in the Vector prototype chain and Vector is not a subclass of Array.
I also tried following this guide on ES5 class inheritance. With that method, both prototypes are in the chain but the arguments passed to the constructor are not captured and Vector also does not extend Array.
This amazing Stack Overflow post exposes the issue here: ES5 classes can not extend native classes. As this Stack Overflow answer to a different question points out, using super(...items) will not work.
After further testing, extending Array is not currently an option. Using numbered indexes will still be done. I’ve implemented a fix using the _length protected property.
Looks like the new changes really helped with performance. Averages across 5000 frames:
current backward compatible: 23.77ms
PR backward compatible: 23.89ms (0.12 ms slower, 0.5% perf loss)
current, aggressive: 12.83ms
PR, aggressive: 11.93ms (0.9ms faster, 7.54% perf gain)
The backward compatible test being slower could be from factors not related to the PR.
1 Like