See Standardize Vectors and Colors by dr-vortex · Pull Request #13699 · BabylonJS/Babylon.js · GitHub, Help with lighting issue, and the screenshot report.
If anyone has in-depth knowledge of the rendering pipeline, please help me with fixing the issue(s) with rendering. I have no experience with or knowledge about the rendering pipeline so any help is greatly appreciated.
The problem is not with rendering here but one of the function you changed is not behaving the same way. The rendering is just a symptom and we should mostly dig for the root cause.
I guess you need to test the functions themselves as anyway the change is really large and having tests would help getting confidence it won’t introduce any regressions. The rendering is only a symptom here.
Also the tests would be helpful to validate the perf impact as we could use them with some kind of load tests.
I agree. The reason I’m looking for a rendering pipeline expert is that there are around a hundred different functions. Testing each of them with all the values that could cause different results (e.g. 0, -1, 1) and a couple of other real numbers would result in 500+ tests. I think doing that many tests would be a waste of time since we could find the function(s) causing the issue by reviewing the code calling it. For now, I will start doing some manual tests.
I want to underline that this is my MAIN concern. i find the code you provided elegant but we already tried several attempts like this one in the past and we always had to rollback as the hit on the perf side was significant.
That being said, we need to test first of course. Maybe your approach is smarter and thus it will be a superb contribution.
Thanks!
@Vortex, because I’m all love and compassion I looked at your rendering issue 
The problem is right there:
Standardize Vectors and Colors by dr-vortex · Pull Request #13699 · BabylonJS/Babylon.js (github.com)
You cannot assume that if len === 1 then you can return result because result will be zero most of the time. So the easy fix is to only return directly when len === 0 only.
…
I just finished working on unit tests for every Vector3 method/static. Thank you so much for finding that. I will add tests to the PR for future vector changes.
Also, Something seems to have broken background colors… Jest Screenshot Report
@Vortex before you fix it all and add lots of tests, you should validate that perf won t be an issue. It would be a shame to throw away all this great work if the perf were not recoverable. I agree the latest fixes could impact perf but I d guess the majority of the changes are in to validate your assumptions that it will be at least as fast.
Even if the standardization doesn’t work, the tests will still be useful. I’m writing them for the classes separately since Vector doesn’t care about the dimension of operands for many methods.
#13973 should help with debugging.
@sebavan @Deltakosh Any thoughts on the background color issue?
cc @RaananW as it seems the PGs do render correctly in the playground:
https://babylonsnapshots.z22.web.core.windows.net/refs/pull/13699/merge/index.html#0YYQ3N#0
The background is black indeed, as in the reference image (https://playground.babylonjs.com/#0YYQ3N#0).
This is related to transparency. The background color of the canvas in the vis-test was recently set to greenyellow to force seeing changes when a transparent element is rendered. Feels like there is an issue with the way colors are parsed in these situations.
Is it possible that there is some conversion from color3 to color4 that sets the 4th value to 0 instead of 1?
Indeed, there’s a problem with Color4. I took a Spector snapshot of the PG in the context of the PR:
@Vortex The alpha value is wrong, it should be 1. So there must be a bug with the Color4 class.
[…] It’s probably because the clearColor in the .babylon file has only 3 components:
It currently works with the existing code, because the loader is doing:
scene.clearColor = Color4.FromArray(parsedData.clearColor);
and Color4.FromArray is:
public static FromArray(array: DeepImmutable<ArrayLike<number>>, offset: number = 0): Color4 {
return new Color4(array[offset], array[offset + 1], array[offset + 2],
array[offset + 3]);
}
array[offset + 3] is undefined because the array has only 3 elements.
The constructor of Color4 is:
constructor(
public r: number = 0,
public g: number = 0,
public b: number = 0,
public a: number = 1
) {}
So alpha ends up being initialized with 1 in this case.
vis-tests doing their job
Thank you! 432a116 should fix it.
Now the real question is REALLY perf because even if I like your work a lot I will not allow it to merge if this is detrimental to perf
Here are some PG you can use to evaluate the before/after regarding perfs:
The sun PG consistently stays at 60 FPS. The other PG is a little slower. Here are the averages across 5000 frames:
current backward compatible: 26.36ms
PR backward compatible: 28.54ms (2.18 ms slower)
current aggressive: 16.08ms
PR aggressive: 17.18ms (1.1ms slower)
Is this acceptable performance? If not, I can take a look at more optimization.
Unfortunately it is not acceptable. We do not commit code that regress perf (and here it is relevant with 10% slower code)
But if you can figure out how to get back to same values, then it will be excellent!
I’ve changed Vector from having a vector property to instead extending Array and having the properties directly on the object. Using vector[i] instead of vector.vector[i] should speed up performance.
It is currently not working though, and fails on every test.
@Evgeni_Popov @Deltakosh
The tests are saying that the methods on Vector do not exist. I can’t think of any reason why these errors are occuring. What do you think?