Idea: Experimental feature flags for CesiumJS

[ptrgags]

Thinking about the future of the Model.js refactor (a large part of #9520), one concern is migration will prove difficult, as there are many, many intertwined features there.

To help avoid creating staging branches for many months on end, it would be nice if we had some standard mechanism for enabling/disabling experimental features. This way, users can try out new features before they are fully finished.

For example, for 3D Tiles it would be very helpful if the contents could do something along the lines of:

var model;
if (ExperimentalFeatures.enable3DTilesNextModel) {
    // Experimental version of Model. Basic rendering may work, 
    // but other features like animations and glTF 1.0 support are not yet implemented 
    // and may break.
    model = new Model2(...); 
} else {
    // Original Model.js implementation
    model = new Model(...);
}

Then applications only have to specify Cesium.ExperimentalFeatures.enable3DTilesNextModel = true to try the new version of Model.

This could be used for other large features in the future as well. The important thing would be to clearly mention that experimental features are subject to change from release to release, use at your own risk.

What are your thoughts, @sanjeetsuhag, @lilleyse, @ebogo1? Is ExperimentalFeatures something that would benefit CesiumJS in the long run? Are there any potential pitfalls to doing this?

[lilleyse]

I like this idea. It reminds me of chrome://flags/ or about:config to access experimental browser features.

Just to make sure everyone agrees, code that's merged into master, while experimental, should meet code quality standards and have unit test coverage, otherwise something like npm run coverage could show very misleading results, or worse it could break existing code. This would be similar how we've been treating the GltfLoader code.

Since this is mainly for developers I would guess that even ExperimentalFeatures itself is @private.

I think it's worth trying and I'm curious what others think.

[ptrgags]

Ah yes, I totally agree that we would still want good test coverage for any experimental features. ExperimentalFeatures would be intended for good quality code, just not yet ready to be exposed to the public API (due to lack of feature parity or other reasons)

[ebogo1]

👍 for an ExperimentalFeatures flag in CesiumJS - this could have been useful for KTX2 and might also be useful for meshopt soon. Any thoughts about how to organize different categories of features? Having options related to tilesets, texture loading, models, etc. all in one file might get out of hand... maybe there won't be enough concurrent experimental features for this to be an issue?

[ptrgags]

@ebogo1 That's a good point, though I think we should keep this simple, just a flat list of flags for now.

ExperimentalFeatures.enable3DTilesNextModel;
ExperimentalFeatures.enableMeshopt;

I plan to make a PR for this early next week. In the code I'll include a block comment so we can describe best practices for this from the start. Some ideas:

• experimental features must have solid unit test coverage. This is not a replacement for a thorough PR review!
• experimental features are intended for large work-in-progress features when there's a benefit to merging the code sooner (e.g. to avoid keeping staging branches around).
• experimental feature flags should ideally be short-lived - make it clear in the PR what needs to happen for the experimental feature to be promoted to a regular feature.

[ptrgags]

Today this came up when talking with @mramato, he was on board with the idea, but he had one big suggestion as far as best practices go:

• When checking for the flag, this should be done in as few places as possible, ideally one place. So for example, Model.js would check the flag once, and then return either the Model class or the Model2 class depending on the setting.

[ptrgags]

One note about this that @sanjeetsuhag and I discovered while designing the new Model: Sometimes checking the flag when you construct an object is the simplest way that effectively works. This is because:

1. You can't conditionally import classes (at least not without ES6 import() but then things become async...)
2. While you can conditionally export classes, the problem is this would be checked too early. The if block would be checked when the page is loading, well before the user sets the ExperimentalFeatures.enableXxxx flag.

One alternate is to make a function to get

function getModelClass() {
   if (experimentalFeatures.enableExperimentalModel)
     return ModelExperimental;
   }
   
   return Model;
}

// usage 
var Model = getModelClass();
var model = new Model(options);

Another is to use a factory function:

function makeModel(options) {
  if (experimentalFeatures.enableExperimentalModel)
     return new ModelExperimental(options);
   }
   
   return new Model(options)
}

In our case I'd prefer the former, since we have both new Model() and Model.fromGltf() constructors, but either could work.

[ptrgags]

Closing this as ExperimentalFeatures is now available in the latest release.