Open ryantrem opened 2 weeks ago
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.
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.
I do like it! And yes we protect backward compat for now! (as we wont remove side effects)
This is a great idea!
A side-note - I find the plugin configuration a bit complicated (syntax-wise). Adding it to importMesh would make more sense TBH, but we will probably have an issue with back-compat.
I initially thought about passing options directly to importMesh, but there are a number of complications. Things like how do we route the options through to the right plugin/loader/extension in a type safe and non-error prone way, and what if someone passes through options for a plugin/loader/extension they have not opted in to use?
Configuring the plugin/loader/extensions as part of opting in to using them doesn't face these same problems. Since SceneLoader doesn't actually hold much state, and instance is basically (as you said) a specific configuration of loading logic, so it is quite light weight and creating multiple instances shouldn't be a problem. It also has the advantage that once you setup your desired configuration, you can just make import calls on that instance many times without having to pass through a complex configuration for every call.
Currently configuring loader options is cumbersome and error prone. If you wanted to adjust a glTF loader option as well as the
MSFT_lod
extension options for example, your code would look something like this:This is somewhat difficult, especially the deeper the options you want to set (such as glTF 2.0 extension options). It is also not super type safe as it requires casting plugins and extensions to known types. Also the configuration requires modifying global/static state, which means it is possible to inadvertently configure other imports besides your own.
While thinking about solutions to this problem, it occurred to me that a solution to this problem is more obvious when we think about it in the context of another problem, which is eliminating our dependency on side effects. Currently loader plugins are registered via side effects, glTF version loaders are registered via side effects, and glTF 2.0 extensions are registered via side effects. If we were not relying on side effects, then the user would do something explicit to opt in to the loaders, glTF versions, and glTF extensions they actually want to support. If they are already doing that, then it would present and obvious place to put configuration for those plugin/loader/extensions. Given this, my proposed changes in this PR work towards solving both of these problems by making providing the option of explicitly choosing what plugin/loader/extensions you want, and configuring them while you are at it. The replacement code for the above would then be:
The advantages of this are:
SceneLoader
instance (theConfigure
calls just return factories that can instantiate the plugin/loader/extension with options).This approach is also fully backward compatible, as it simply introduces the option to instantiate a
SceneLoader
and configure it exactly the way you want (leveraging the existing factory function logic). I think this approach is inline with a futureCoreScene
/CoreSceneLoader
/CoreSceneLoaderPlugin
as well, so we could likely rework it to be built on top of that layer when it is created.High level notes on the changes:
SceneLoader
was mostly moved to instance state, with a private default instance that the static contract calls into. The default instance behaves the same as the original static API.OptionsBase
class was introduced to maintain back compat (options directly on plugin/loader) while avoiding duplicating the list of options across multiple types.Completing this approach will require:
SceneLoader
s.