Add generic Assert function

[ryantrem]

This adds a generic Assert function using the asserts keyword introduced in TS 3.7 (we don't have any other uses of asserts in the codebase currently). This generic Assert function just asserts a truthy value. If the assertion fails, it throws a generic error. Since it uses the asserts keyword it can affect control flow.

Some example use cases:

• Inserting a value into a map, and then later in the code retrieving it. Instead of using a non-null assertion, you could call Assert, which can make debugging easier since the exception is thrown at the time of retrieval rather than when the value is actually used (e.g. it could be passed through a bunch of functions and eventually causes an exception trying to operate on an undefined value).

map.set("foo", foo);
// a bunch of other code, where you could possibly have made a programming error
const foo = map.get("foo");
Assert(foo);
// Foo is now guaranteed to be defined
foo.doSomething();

• Other truthy cases

const value = null! as A | B;
Assert(value instanceof A);
// value is now guaranteed to be an A
value.doAnAThing();

I did not add a message param to this Assert function because those strings would end up in the final bundle and increase its size. Since assertions are supposed to be for programming errors (not usage errors), they are really intended to contributors, not consumers, and I don't think consumers should pay the extra cost.

[bjsplat]

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

[bjsplat]

Snapshot stored with reference name:
refs/pull/16039/merge

Test environment:
https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16039/merge/index.html

To test a playground add it to the URL, for example:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16039/merge/index.html#WGZLGJ#4600

Links to test babylon tools with this snapshot:

https://playground.babylonjs.com/?snapshot=refs/pull/16039/merge
https://sandbox.babylonjs.com/?snapshot=refs/pull/16039/merge
https://gui.babylonjs.com/?snapshot=refs/pull/16039/merge
https://nme.babylonjs.com/?snapshot=refs/pull/16039/merge

To test the snapshot in the playground with a playground ID add it after the snapshot query string:

https://playground.babylonjs.com/?snapshot=refs/pull/16039/merge#BCU1XR#0

[bjsplat]

WebGL2 visualization test reporter:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16039/merge/testResults/webgl2playwright/index.html

[bjsplat]

Visualization tests for WebGPU (Experimental)

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16039/merge/testResults/webgpuplaywright/index.html

[sebavan]

I like it and as it is still a bit more costly than "Non-null assertion" and as it will end up in an exception as well I would probably not replace in the hottest path but totally good in lower frequency usage.

[noname0310]

I like it and as it is still a bit more costly than "Non-null assertion" and as it will end up in an exception as well I would probably not replace in the hottest path but totally good in lower frequency usage.

A runtime check proves that the value is valid after the assertion, so the JIT compiler can do a better job of optimizing when generating code.
therefore, it seems like it could be helpful if utilized well in hot paths.

[ryantrem]

I like it and as it is still a bit more costly than "Non-null assertion" and as it will end up in an exception as well I would probably not replace in the hottest path but totally good in lower frequency usage.

A runtime check proves that the value is valid after the assertion, so the JIT compiler can do a better job of optimizing when generating code.
therefore, it seems like it could be helpful if utilized well in hot paths.

Oh that's quite interesting. Maybe I'll try to find some places in the code to test.

[RaananW]

I'm still digesting :-)

The real benefit here is typescript. In javascript an exception will be thrown if the object is not available (and will actually provide the right stacktrace). I agree with the "not on the hot path", but would be interesting to see if what's @noname0310 is correct, because then there is no reason not to use it (where needed, of course).

[ryantrem]

I'm still digesting :-)

The real benefit here is typescript. In javascript an exception will be thrown if the object is not available (and will actually provide the right stacktrace). I agree with the "not on the hot path", but would be interesting to see if what's @noname0310 is correct, because then there is no reason not to use it (where needed, of course).

I don't think that is always the case. For example, if you use a non-null assertion, and the variable is returned through multiple stack frames, and eventually you try to call a function on it, the error is at the final call site and you have to do some digging to find the place in the code where the bad non-null assertion happened. If you assert at the same point in the code where there otherwise would have been a non-null assertion operator, then you will immediately see exactly in the code where the programming error was.

[RaananW]

I don't think that is always the case. For example, if you use a non-null assertion, and the variable is returned through multiple stack frames, and eventually you try to call a function on it, the error is at the final call site and you have to do some digging to find the place in the code where the bad non-null assertion happened. If you assert at the same point in the code where there otherwise would have been a non-null assertion operator, then you will immediately see exactly in the code where the programming error was.

I wouldn't recommend using non-null assertion. There is always a better way. I would use optional chaining where possible, but that could also lead to different issues.
This assertion is technically checking if the value is there and if it isn't, it throws an error. similar to simply adding

if(!variable) {
    throw...
}

which is something we do in certain cases. Typescript is also perfectly fine with this check. In many cases, throwing is not the right thing. it is very much use-case dependent. if we have an onError callback, it would be best not to throw but to call onError. If we are in the render loop it is better not to throw (maybe?).

Sorry, it sounds like I am against it :-) . I'm not, i just try to understand what are the benefits, apart from reusable assertion function

[sebavan]

Would be perfect if we had debug/release builds

[Popov72]

My two cents is that "assert" is normally used to find bugs during development (debug mode), but that this code should be removed in release mode. If the code remains in release mode, then it's no longer "assert" but normal error handling code, which we should handle in the right way (log, provide a clear error message, ...).

A runtime check proves that the value is valid after the assertion, so the JIT compiler can do a better job of optimizing when generating code. therefore, it seems like it could be helpful if utilized well in hot paths.

I don't think we should rely on what a compiler would or wouldn't do under the hood, because not all compilers do the same thing / their behavior can change over time.

[RaananW]

Would be perfect if we had debug/release builds

Removing it for release would fail production build in certain cases. But yes, I like the general idea :-)

[ryantrem]

i just try to understand what are the benefits, apart from reusable assertion function

An assertion is intended to detect programming errors (not "expected" runtime errors from user input). I think @Popov72 stated it well, and I did wonder about having custom webpack step (or something like that) to remove calls to Assert. The only downside is it would only help us identify programming errors in local dev builds and not "production" builds. But maybe if they were not removed for tooling like Playground it would be the best of both?

[bghgary]

This is what I did in my side project to make code disappear in production build vs dev build.

https://github.com/bghgary/tank3d/blob/main/deploy/webpack.dev.config.js#L20
https://github.com/bghgary/tank3d/blob/main/deploy/webpack.build.config.js#L33
https://github.com/bghgary/tank3d/blob/c1b6001fea7e99bb58a13f4f9da0dd74e696f298/app/src/worlds/world.ts#L24
https://github.com/bghgary/tank3d/blob/c1b6001fea7e99bb58a13f4f9da0dd74e696f298/app/src/worlds/world.ts#L67-L84

The bundler will remove the if (false) block. We can configure the defines how we want for various scenarios.

[sebavan]

yup we discussed it a few times with @Popov72 as well for rendering tooling :-)

[RaananW]

This will need to happen as a post-step of the build, otherwise this code:

const perBoneCorners = skinnedMeshCorners.get(mesh.uniqueId);
Assert(perBoneCorners);
perBoneCorners.forEach((corners, boneIndex) => {

will fail compilation because it is possible perBoneCorners is null.

I have not seen a good plugin that removes debug code correctly and without issues. There is also a slight difference between debug code (like Gary's example), and assertion as part of a prod function IMO.

There is no issue running a post-step during the build removing all calls to Assert, it just needs to be configured correctly, and will need to be well tested, as these kind of changes tend to cause issues in the future.