Migrate engine Constants from a static class to const enums and normal exports

[james-pre]

This PR migrates Constants from a class to const enums and normal exports. It is backward-compatible, and the backward-compatible exports are marked as deprecated.

This should result in a sizable jump in runtime performance, since const enums' values are inlined at compile time. It will also reduce the bundle size, especially if the backward-compatible Constants object is removed. If not, it should still reduce the bundle size due to the inlining.

[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]

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]

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.

[deltakosh]

I do not think we can touch that one as it is unrolled by the build system and replaced directly into the code

@RaananW ?

[james-pre]

@deltakosh,

Perhaps this could be a good opportunity to reduce the workload of the build system, and just use Typescript's inlining of const enums? I'm not as familiar with BJS as you and the team are, so I'm not sure if using const enums would be better or worse.

If that's not doable, we can at least change Constants to an object instead of a static class.

I've put in at least 15 hours so far, so I'd prefer if it didn't go to waste, but I understand if const enums and normal constants aren't doable.

As for why the builds are failing, it looks like a lot of Generic type 'Observable<T, T>' requires 2 type argument(s). and Type 'T' is not assignable to type 'BABYLON.T'. (T being AbstractMesh, Mesh, BaseTexture, etc.)

I'll stop working on this for now in case you would like me to just make it an object or do something else.

[RaananW]

We are already using const enum throughout the repository. However, becasue of the way typescript is dealing with const enums, we have to both export the js part of it AND roll it back to enums in the final release. There are reasons behind it, but I won't get into that here.

About this change - this is something we have considered, but it requires a bit mroe thought and careful implementation. I will go over this PR and give some feedback, but maybe next time before working for 15 hours and submitting a PR you can discuss that on the forum and save you a lot of time.

[james-pre]

About this change - this is something we have considered, but it requires a bit mroe thought and careful implementation. I will go over this PR and give some feedback, but maybe next time before working for 15 hours and submitting a PR you can discuss that on the forum and save you a lot of time.

Yeah sometimes I think I'll just submit the PR when I'm done working, and it almost always backfires. I'm glad I submitted a (draft) PR after 15 hours before I put in another 10 or something.

Please don't waste to much time on this PR if you think its not a good idea to switch to const enums, I totally understand.

[RaananW]

Please don't waste to much time on this PR if you think its not a good idea to switch to const enums, I totally understand.

This is actually something we have considered - changing the engine's constants to enum(s), very similar to what you have done. The thing is that since we are extremely careful about back-compat, we try to make these kind of changes gradually. Const enums, for example, were only introduces a month or two ago, even though we could have done that a long time ago. We want to be 100% sure everything is working and nothing is broken before we do something like that.

I haven't fully reviewed it yet, but I wouldn't invest more time in this for now.

[james-pre]

Sounds good.

The thing is that since we are extremely careful about back-compat, we try to make these kind of changes gradually.

I'm preserving the export of a Constants object so it should be backward compatible, but I totally understand the hesitation. I would too if I were reviewing this PR- its not a tiny change, and should be reviewed carefully due to the impact.

As I already mentioned, If you don't want to make the change, I can just make it into an object for now, then we can take a look a const enums and const variables later. What do you think?

[RaananW]

we can take a look a const enums and const variables later.

We are already using const enums, so I am not sure what you are referring to exactly here, but - let me first review this. It will take some time, asking for your patience.

I love your enthusiasm, and I appreciate all of the contributions. Just asking - from now on, if you do want to make any major change like this, please - have a discussion with the core team on the forum. It is an open source project, and we really do love contributions. However, every contribution will eventually need to be maintained by the team as well, and we love knowing what we are getting into.

[james-pre]

We are already using const enums, so I am not sure what you are referring to exactly here, but - let me first review this. It will take some time, asking for your patience.

I mean specifically for the engine Constants.

I love your enthusiasm, and I appreciate all of the contributions. Just asking - from now on, if you do want to make any major change like this, please - have a discussion with the core team on the forum. It is an open source project, and we really do love contributions. However, every contribution will eventually need to be maintained by the team as well, and we love knowing what we are getting into.

Yeah, as a creator of an open-source project, I understand. I've rejected some pull requests simply because the code was so unreadable I wouldn't be able to maintain it. Thanks for your understanding. I'll try to take things slower.

FYI this PR is not finished. I don't expect to make changes to what I've already done, so it should be safe to review.

[ryantrem]

From a quick glance, this looks similar to #14076. If so, that PR discussion may have some useful context.

[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.

[james-pre]

@ryantrem, Thanks for linking your PR.

The types having the non-existent const enum when preserveConstEnum is false and back-mapping when preserveConstEnum is true is a problem.

However, I think we have a unique opportunity with Constants, because Constants already has custom build system behavior (since its values are inlined). If we use the build system to fix the reverse mapping of the const enums by using an object instead of inlines Constants, we could keep a similar amount of maintainer workload with const enums.

Now I'm wondering, how does webgpuConstants.ts handle runtime behavior? @RaananW Do you have any insight?

[github-actions]

This pull request has been marked as stale because it has been inactive for more than 14 days. Please update to "unstale".

[james-pre]

@RaananW Do you have any insight on how we could handle constant inlining with ES6 and how webgpuConstants.ts handles things at runtime?

[deltakosh]

cc @RaananW :) If no activity I suggest we close this PR