Async material constructor

[mzschwartz5]

Description

There's currently no way to wait for a material to load its required resources, or to know when they're loaded. Currently, the Material constructor only creates functions that will load resources, but those functions are only called on subsequent Update calls.

One consequence of this is that our unit tests rely on polling the internal _imagesLoaded list to determine when its okay to make assertions. This is verbose and flaky.

For ease of testing and consistency with other APIs (like Model.fromGltfAsync), we'd like an analogous Material.fromTypeAsync factory function. Because of the current image-loading polling model, and the complexity of the Material creation logic, creating such an async method is a challenge. For more context, see this comment and following discussion.

The solution this PR takes is to extract image-loading logic into callable, async functions. The Update loop calls those functions indirectly (similar to before), but now we also call those functions on Material construction and keep a list of the resulting promises. The fromTypeAsync constructor can then await all the initialization promises, and the caller of fromTypeAsync can await the result of that.

See my comment below for a commit-by-commit breakdown of this PR.

Issue number and link

#10566

Testing plan

Localhost sandcastle

Also tested with the materials sandcastle and everything looks good to me. Feel free to suggest / test with more materials-type sandcastles.

Author checklist

• I have submitted a Contributor License Agreement
• I have added my name to CONTRIBUTORS.md
• I have updated CHANGES.md with a short summary of my change
• I have added or updated unit tests to ensure consistent code coverage
• I have updated the inline documentation, and included code examples where relevant
• I have performed a self-review of my code

[github-actions]

Thank you for the pull request, @mzschwartz5!

✅ We can confirm we have a CLA on file for you.

[mzschwartz5]

Adding some context for this PR on a commit-by-commit basis:

Commit 1: Refactors conditional logic of material image loading

The ultimate goal is to be able to extract image loading into its own, callable function. The complex conditional statement in the original code makes this quite difficult. This commit simplifies the conditional to facilitate the end goal.

The behavior should be the same, just less convoluted logic. There IS one, temporary, behavioral difference - I completely removed this check:

!defined(material._texturePaths[uniformId])

This statement exists for the first time this code runs - after which, this property will always be defined. Since I intend to explicitly load the images on Material construction in the next commit, this property will always be defined in this code path, and the check will no longer be necessary.

Commit 2: Refactors material texture 2D image loading into own function.

Now that the conditional logic is simpler (last commit), this commit extracts the image loading logic into its own function, and calls it immediately after using createTexture2DUpdateFunction to register an updater.

The image loading logic returns a promise, which is pushed into a list of initialization promises. Nothing is done with that list in this commit, but it will be used in the future commits for the async factory constructor.

Commit 3: Refactors cubemap image loading into own function

Similarly to what I did for the 2D texture image loading logic, this commit extracts the cubemap image loading into a callable function which returns a promise. The createUniforms function now immediately calls this image loader and pushes this promise onto the initialization promises list.

And the updater function returned by createCubeMapUpdateFunction now also uses this image loader function, but doesn't do anything with the promise - instead, as before, it polls the status of the image loading in the update loop. The promise is just for the async factory constructor coming in the next commit.

Also: had to change one unit test. It would hang forever waiting for _loadedImages.length to be not 0 on the second material, but because of the refactor, this never happens because both images get handled in the first render.

This is why you don't rely on internal details of a class for unit tests :)

Commit 4: Adds async factory method for Material class

We finally get to the main purpose of this PR - making an async factory constructor. Behaves very similarly to fromType, but waits on the initialization promises (including those from submaterials), and returns a promise itself that can be awaited.

Note: there are two (I think?) bugs in getInitializationPromises in this commit. Fixed in later commit.

Commit 5 - just adds JS docs

Commit 6: Adds unit tests for async material constructor

Note: while writing unit tests, I learned that fromType / the material cache does not support submaterials! This commit also adds support for it by making two small adjustments:

1. Create uniforms and submaterials after retrieving the cached material
2. Populate the material .fabrics member from the options.fabrics passed into the constructor.

I'm a little nervous that this may introduce some bugs, but the unit tests and my own testing didn't reveal any.

Commit 7 - Material unit test update and CHANGES.md

Updates the material unit tests to use the new async constructor. Ironically, only one test can actually make use of it. The others that test images are all testing "what if we change an image...", which out of scope of what the async constructor does.

Commit 8 - Adds Material constructor options to fromType and fromTypeAsync methods

As title says, adds the options object to these methods (for min/mag filters, strict, and translucent options). Updates unit test to use these instead of setting internal fields.

[mzschwartz5]

Part of the goal of this PR was to make the Material unit tests more concise and less reliant on internal fields. Using fromTypeAsync definitely makes this unit test more readable, but it still relies on the internal filter fields, because these properties are not copied over when constructing "by type", and thus have to be set manually.

[javagl]

I had a look at the resulting code state and the individual commits, and the changes look reasonable for me. I have not thoroughly and deeply checked whether the changes from pulling out the 'loader' functions really do not affect the behavior, but from what I've seen (and the descriptions), there wasn't anything that would be obvious to break in any way.

Two inlined comments about the specs. I'm not sure whether something like that fromOptionsAsync could make sense...

[javagl]

Should this call (and the one in the test below) check the rendererd color using a callback? Maybe this one is only about the basic creation, but the one below could probably do a check for 'blue'.

[mzschwartz5]

That's a good point- the one below should probably at least check the color.

[javagl]

I've seen the comment below. But this is accessing a private variable. While this is can, of course, be OK for a spec, it raises the question of how users could create such a material with these filter options asynchronously.

It may be a bit of a stretch, but ... could it make sense to have some fromOptionsAsync that receives the full options that can be passed to the material constructor?

[mzschwartz5]

could it make sense to have some fromOptionsAsync that...

I don't see why not, honestly. I was mostly following the pattern set out by Material.fromType, which has the same shortcomings. But maybe both should have an options field (it wouldn't be a breaking change if we added a new optional field to .fromType, right?)

I mean, ideally, the .fromType func should have just accepted the full options, including fabric, from the get-go, but this is how we'd add the behavior without breaking the interface.

[mzschwartz5]

The additional arg would basically be the Material options object, minus the fabric part which comes from the type (and optional uniforms) passed in.

[ggetz]

Hm, I'm not a fan of having to call Material.fromType(type, undefined, options) if there are no uniforms.

But, given we're in here and doing some major cleanup, I think it makes sense to re-evaluate the existing API from first principles. For instance,

• We can create new static constructor functions as needed, synchronous or asynchronous
• We can expose some options via getter/setters if it makes sense
• We could compare the Material APIs of ThreeJS and BablyonJS as a reference point

(I'm taking a review pass now, and will add other thoughts here if they come up once I look more holistically at the code.)

[ggetz]

Overall, the biggest complication I'm seeing is that we need to pass a context option to create a Texture, meaning we have some limitations on when we can actually create a texture.

Going through some other parts of the API, I noticed TextureUniform seems to be handling the aggregation of options needed for creating a texture. Maybe we could make use of that and allow it to be passed as part of the existing uniforms object? In the case where things are synchronous, we can resolve the resource in the update function. In the case of asynchronous loading, we can resolve or throw before construction.

[mzschwartz5]

I noticed TextureUniform seems to be handling the aggregation of options needed for creating a texture. Maybe we could make use of that and allow it to be passed as part of the existing uniforms object?

I'm not exactly sure what the suggestion is here, but it seems strange to me to cram unrelated properties into the uniforms object. It would mismatch the top-level options schema, but more importantly, we'd then have to parse out "which of the fields in this object are uniforms and which are other options" - if I'm understanding correctly.

[mzschwartz5]

I just pushed two commits, here's what I've done to address the concerns in this thread and some offline comments:

1. I removed the options field both from fromType and fromTypeAsync. Instead, I added setters to the Material API for min and mag filters. We can consider adding setters for other options as well (some, like strict, only apply at construction, so it's pointless to have a setter).
2. I added tracking of errors on initialization and now rethrow an error if one exists, in the fromTypeAsync method.

And here's something else to consider, based on offline discussion:

As it stands, a Material allows you to pass in options for its texture sampler (namely: min/mag filter). However, a Material can have multiple textures - but only one sampler. The sampler should really be a property of the material.

After reading up on the Material fabric json schema, I feel that the most sensible way to support this would be to add a property on the schema, "samplers", which would map each texture name to an object of sampler settings.

Then, in the Material API, if that mapping is defined in the fabric, use it. Otherwise, default to the Material-level setting.

If we add to the fabric schema in this way, it wouldn't require any changes to the signatures of fromType or fromTypeAsync. So if we can agree on that path forward, we're good to push these changes without further consideration.

[ggetz]

I pushed up a some small test cleanup. It was not strictly related to changes from this PR, but I noticed some code re-use that was not following jasmine's recommended best practices.

[mzschwartz5]

If we ever target ES2021 or higher, we could use an AggregateError here.

[javagl]

There are some structural changes in the tests that look like cleanups and generalizations to make them more concise.

There are also considerable restructurings in the material itself, beyond the introduction of the async factory. It can be difficult to "track" and "match" these changes in the GitHub side-by-side view. But I assume that...

1. these restructurings are necessary (e.g. to pull ~"the stuff that loads the image" into a function that can sensibly awaited)
2. these restructurings did not introduce non-obvious semantic changes

Yeah. The second one is always a big 🤞 . The point is that I see that certain empty objects are now cloned, or certain calls have been moved before that "caching check"...

and I could try to figure out: What exactly is happening there, and what is the effect (or possible side-effect) of doing this before the caching check...?

But iff it changed the behavior in any way, then one could say that the tests are still passing, so whatever the change of the behavior may be: It was (certainly not 'specified' to begin with, and) apparently not important enough to be tested explicitly.

On a higher level:

The overall approach looks sensible. On the API level, the consistency of the fromType and fromTypeAsync should make it easy to use, without users having to learn something new (and it is a pure addition to what was already there). The process of collecting all relevant promises recursively and awaiting them looks like a clean solution for the task at hand.

(My baseline complaints (lack of documentation, unused parameters, things like createTexture2DUpdateFunction that should be broken into 4 smaller functions etc) do not apply to this PR, but to the code that was already there...)

One thing that is specific for this PR:

The minificationFilter/magnificationFilter setter/getter are an addition to the public API, and I think that it could make sense to mention that it's now possible to set these filters on existing materials. (Not crucially imporant, but maybe as a short note in the existing 'Additions' point...)