Adds m-material element to MML #181

DaniloArantesF · 2024-06-25T13:18:16Z

This PR adds a new element m-material to the mml-web package and the MML schema.
This element allows users to replace the material of any primitive (e.g. cube, cylinder, plane, sphere) with more granular control of the PBR material that is created for the parent element.
For this implementation, textures are cached and re-used across elements to minimize the performance and memory overhead.

m-material-test.mp4

Example:

<m-cube y="2" x="0" ry="45" width="2" height="2" depth="2">
  <m-material
    map="http://localhost:7079/assets/bricks/Bricks042_4K_Color.jpg"
    ao-map="http://localhost:7079/assets/bricks/Bricks042_4K_AmbientOcclusion.jpg"
    ao-map-intensity="1"
    normal-map="http://localhost:7079/assets/bricks/Bricks042_4K_NormalGL.jpg"
    displacement-map="http://localhost:7079/assets/bricks/Bricks042_4K_Displacement.jpg"
    displacement-scale=".0"
    roughness-map="http://localhost:7079/assets/bricks/Bricks042_4K_Roughness.jpg"
    metalness="0"
  ></m-material>
</m-cube>

<m-sphere radius="1" y="2" x="4">
  <m-material
    map="http://localhost:7079/assets/planks/Planks002_4K_Color.jpg"
    normal-map="http://localhost:7079/assets/planks/Planks002_4K_NormalGL.jpg"
    displacement-map="http://localhost:7079/assets/planks/Planks002_4K_Displacement.jpg"
    displacement-scale=".0"
    roughness-map="http://localhost:7079/assets/planks/Planks002_4K_Roughness.jpg"
  ></m-material>
</m-sphere>

What kind of changes does your PR introduce? (check at least one)

Does your PR introduce a breaking change? (check one)

Yes
No

If yes, please describe its impact and migration path for existing applications:

Does your PR fulfill the following requirements?

All tests are passing
The title references the corresponding issue # (if relevant)

…ll textures correctly

+ Added m-material to the MML schema

MarcusLongmuir

This is awesome and a very welcome PR. Thanks!

Aside from the specific lines I've commented on there are two issues I've noticed that might require small refactors:

The way that the m-material attaches to the parent currently uses a property check for mesh which is difficult to discover and make work across different affected elements. It could instead us the same mechanism of elements like m-attr-anim and use the addSideEffectChild of the parent MElement which can then choose to use this child m-material.
The primitive elements also modify the material that their mesh uses (e.g. Cube modifies this.material.color and this.material.opacity directly. This means that the ordering and precedence isn't predictable. I think we should make the behaviour that if an element has an overriding material then the color and opacity can't be modified directly.

There are also two wider concepts related to materials that are worth noting so we can design now for their future implementation:

Reusing materials across elements by creating an m-material with an id that can then be referenced by multiple elements. This in particular means that you don't want elements to be able to modify the material.
The ability to override specific materials of an m-model.

I don't think it's necessary to get all of that in a single addition, but we'd prefer to avoid changing the behaviour to accommodate those features.

MarcusLongmuir · 2024-06-25T20:39:17Z

packages/mml-web/src/elements/Material.ts

+      envMapRotation: this.props.envMapRotation,
+      envMapIntensity: this.props.envMapIntensity,
+      wireframe: this.props.wireframe,
+      wireframeLinewidth: this.props.wireframeLinewidth,


wireframeLinewidth is limited on most platforms to 1 so we probably shouldn't include it as the behaviour can't be implemented consistently.

MarcusLongmuir · 2024-06-25T21:24:06Z

packages/mml-web/src/elements/Material.ts

+        instance.materialManager
+          .loadTexture(instance.props.displacementMap, instance)
+          .then((texture) => {
+            instance.material!.displacementMap = texture;


The displacementMap is an interesting one because it allows rendering the vertices of the element visually outside the detectable bounds which violates the m-frame min-/max- attributes. I'd propose omitting it.

MarcusLongmuir · 2024-06-25T21:36:54Z

packages/mml-web/src/elements/Material.ts

+        if (value) {
+          const texture = await this.materialManager.loadTexture(value.toString(), this);
+          if (texture && this.material && key in this.material) {
+            (this.material[key] as unknown as THREE.Texture) = texture;


This is prone to race conditions if the loading of a first texture (A) takes long enough that a new texture (B) has been set and loaded.

The first texture (A) will finish loading and overwrite B.

Does fixing the issue with populating the cache after load also fix this issue? Now element A will kick off the promise, and element B will wait on it.

MarcusLongmuir · 2024-06-25T21:47:12Z

packages/mml-web/src/elements/MaterialManager.ts

+      return cacheItem.texture;
+    }
+    const texture = await this.textureLoader.loadAsync(src);
+    this.textureCache.set(src, {


Populating the cache only after loading means that two or more materials using the same map that are created at the same time will each load the texture and then all populate the cache. The alternative is to populate the cache with the promise.

GwilymIO · 2024-06-26T13:55:08Z

This is awesome!! We have been on about something like this for a while <3

…ctly + Primitive elements can't modify their child materials anymore though color and opacity + Cache is set to promise when loading new texture to avoid race condition + Properly catches error when loading textures and returns null + Removed the wireframeLinewidth, displacementMap, displacementScale and displacementBias attributes

+ Adds m-material-priority-test

…ing a shared material on connection + The loaded event is dispatched on the material now and the disconnected event is dispatched on each registered user element

+ Replaced primitives-test assets

…ements

DaniloArantesF · 2024-07-01T12:06:25Z

@MarcusLongmuir Thank you again for the feedback. I think I have addressed most of your concerns so far. The attributes were removed and the textureCache now stores a promise. Can I assume this also fixes the other issue you brought up here?

This is prone to race conditions if the loading of a first texture (A) takes long enough that a new texture (B) has been set and loaded.

The first texture (A) will finish loading and overwrite B.

I've also switched to using addSideEffectChild and I'm preventing the attributeHandlers from modifying the material if it is from a material element.

Your first suggestion seemed simple enough, so I decided to implement it. Now you can define a shared material by creating a m-material element with an id. It will automatically be registered in the MaterialManager and call addSideEffectChild on any elements with the attribute material-id set to that id.
To keep the execution deterministic I had to establish a priority between the two ways of attaching the material. Currently, it prioritizes a direct child over a material-id attribute. I also had to handle the case of non-unique IDs for shared materials. Currently, the order of the elements in the page is respected and further calls to register a shared material are ignored. The call to register is set before textures are loaded to avoid a race condition like in one of the issues you raised before.
I also moved the material logic from the MElements into a separate MaterialElementHelper class, so it should be easier and faster to debug and implement changes since it is all better encapsulated now.

<m-plane width="20" height="20" rx="-90" material-id="my-material"></m-plane>
<m-cube y="2" x="0" material-id="my-material"></m-cube>
<m-material id="my-material" map="http://localhost:7079/assets/test-image.jpg"></m-material>

Finally, are you able to provide more details on how you would like to override m-model materials? I'm confident I can get that done as well once we figure out the best way to specify which materials should be replaced.

DaniloArantesF · 2024-07-01T12:14:58Z

@GwilymIO Happy to help! I would love to get more involved and work with the MML team to bring more of these features to life.

MarcusLongmuir · 2024-07-01T22:24:14Z

Great work. I need to read the code that you've added, but there are a few points that I've noted whilst using the behaviour and thinking about this:

The material-id is currently global, but independent documents (e.g. those loaded from within different m-frames or just by being different mml documents loaded by the library) should not share ids and documents should be able to assume their contained material is the one that will apply (not the first element in the whole window with the id)
I haven't fully reasoned about the mesh material replacement, but the challenge is that in a given gLTF scene there are an arbitrary number of meshes with arbitrary hierarchy and then there is also the possibility that different model formats (not gLTF) allow multiple materials per primitive mesh (requiring a ThreeJS materialIndex equivalent). Related: https://discourse.threejs.org/t/some-issues-when-export-a-single-mesh-with-multiple-materials-using-gltf-exporter/50395. The problem is planning what the attributes for the m-material and m-model element are going to look like to avoid creating future issues, even if we don't include this behaviour in this PR.
Independent of the caching of the Promise the loadTexture call is still race-condition prone because for a given call to loadTexture the function chained on the then can execute after a subsequent call for the same map. E.g.
```
instance.materialManager.loadTexture(instance.props.map, instance).then((texture) => {
  instance.material!.map = texture;
  instance.material!.needsUpdate = true;
});
```
In this usage the setting of instance.material.map inside the function can be being done by an old call to loadTexture that took a long time and is now overwriting the latest texture.

DaniloArantesF · 2024-07-11T19:13:29Z

@MarcusLongmuir That's a good point. Shared materials should be scoped to their documents/frames so that they can't affect/be affected by elements outside of it. I haven't looked into this issue yet, but I'm assuming that I can check if an element is a child of a m-frame like so:

    const remoteDocument = this.getRemoteDocument();
    if (remoteDocument?.parentElement instanceof Frame) {
      // scoped to this m-frame
    } else {
      // scoped to root
    }

My first thought is to use the frame/document pointer as a key to map elements to their scope. If you have other considerations or a better approach please let me know.

MarcusLongmuir · 2024-07-15T08:26:32Z

I think you can just use the getRemoteDocument() function to give you the document to use as the key in the scoping map. The value will be null if the element is just on a page without having a remote document.

+ The element's remote document is used to scope the shared material to that document + Elements will not be able to use shared materials if they are not in the same remote document

+ Adds checks before overwriting the material texture to ensure the value has not changed since the load started

DaniloArantesF · 2024-07-24T18:53:06Z

@MarcusLongmuir Finally got some free time this week to implement the changes we discussed. I went with a scoped key instead of using the m-frame pointer. I create a key by appending the id of the material to its remote address (e.g. ws:localhost:7079/document.html/#my-shared-id) and keep using one map for everything. Lastly, I fixed the race condition when loading textures. I was able to create a test to verify the issue and it is working perfectly for me now.
Please let me know if you see other issues with the new code, and I'll do my best to get them fixed as soon as I can.

MarcusLongmuir

It seems that there are quite a few places that are responsible for applying the logic of which material should be applied to an element given the mutation being handled.

Whilst the cues to do this check and modification can come from multiple places I think this would be far easier to comprehend/avoid regressions/ensure correctness if the logic for applying order of precedence and changing materials was in one function.

That might be easier to do as a simplification pass though after we get the correctness in this PR.

I'm not confident that we know how we'll handle multiple materials applied to an m-model. Until we have a design for that it's a bit of a risk to land this as-is because we may realise the material-id approach is incompatible. Have you had any thoughts about that?

MarcusLongmuir · 2024-07-26T14:29:46Z

packages/mml-web/src/elements/MaterialManager.ts

+    let sharedMaterial = this.sharedMaterials.get(scopedId);
+
+    if (sharedMaterial && sharedMaterial.material?.getMaterial()) {
+      const conflictingNodes = sharedMaterial.remoteDocument?.querySelectorAll(


Using querySelectorAll here could result in finding m-material nodes that aren't owned by this particular remote document and are instead inside m-frames within the remote document.

MarcusLongmuir · 2024-07-26T14:37:02Z

packages/mml-web/src/elements/MaterialManager.ts

+  }
+
+  registerSharedMaterial(remoteAddress: string, id: string, material: Material) {
+    const scopedId = MaterialManager.getScopedMaterialKey(remoteAddress, id);


Using the address of the document avoids most conflicts, but it's not 100% correct because you can include the same document url in multiple m-frames and then the materials collide and it's possible that those two connections to the same address present inconsistent m-materials to the client.

DaniloArantesF added 5 commits June 18, 2024 16:57

adds a basic m-material element

a348538

Added MaterialManager to cache textures

acabc11

Improves performance and implements child materials to other MElements

ba26707

Properly disconnecting m-material from parent element and handling nu…

d3a0f8f

…ll textures correctly

Added e2e and mml-web tests

e1cdfd3

+ Added m-material to the MML schema

MarcusLongmuir reviewed Jun 25, 2024

View reviewed changes

DaniloArantesF added 7 commits June 26, 2024 16:31

Implements shared materials for cubes

e3c7ebc

Added shared material support to other primitives and updated schema

b826a61

Establishes priority when both material-id and child material exist

604e812

+ Adds m-material-priority-test

Handles duplicate shared material ids and fixes the issue with replac…

b7c9794

…ing a shared material on connection + The loaded event is dispatched on the material now and the disconnected event is dispatched on each registered user element

Updated e2e-tests

ea859e5

+ Replaced primitives-test assets

Adds MaterialHelper class to abstract material element logic from MEl…

354a273

…ements

DaniloArantesF added 4 commits July 23, 2024 14:24

Implements shared material isolation by frame

b1b3613

+ The element's remote document is used to scope the shared material to that document + Elements will not be able to use shared materials if they are not in the same remote document

Adds m-material frame tests

f36a6a6

Updates other primitives with call to materialHelper connectedCallback

996e84a

Fixes race condition when loading textures for m-material

ac832b1

+ Adds checks before overwriting the material texture to ensure the value has not changed since the load started

MarcusLongmuir reviewed Jul 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds m-material element to MML #181

Adds m-material element to MML #181

DaniloArantesF commented Jun 25, 2024

MarcusLongmuir left a comment

MarcusLongmuir Jun 25, 2024

MarcusLongmuir Jun 25, 2024

MarcusLongmuir Jun 25, 2024

DaniloArantesF Jun 26, 2024

MarcusLongmuir Jun 25, 2024

GwilymIO commented Jun 26, 2024

DaniloArantesF commented Jul 1, 2024

DaniloArantesF commented Jul 1, 2024

MarcusLongmuir commented Jul 1, 2024

DaniloArantesF commented Jul 11, 2024

MarcusLongmuir commented Jul 15, 2024

DaniloArantesF commented Jul 24, 2024

MarcusLongmuir left a comment •

edited

Loading

MarcusLongmuir Jul 26, 2024

MarcusLongmuir Jul 26, 2024

Adds m-material element to MML #181

Are you sure you want to change the base?

Adds m-material element to MML #181

Conversation

DaniloArantesF commented Jun 25, 2024

MarcusLongmuir left a comment

Choose a reason for hiding this comment

MarcusLongmuir Jun 25, 2024

Choose a reason for hiding this comment

MarcusLongmuir Jun 25, 2024

Choose a reason for hiding this comment

MarcusLongmuir Jun 25, 2024

Choose a reason for hiding this comment

DaniloArantesF Jun 26, 2024

Choose a reason for hiding this comment

MarcusLongmuir Jun 25, 2024

Choose a reason for hiding this comment

GwilymIO commented Jun 26, 2024

DaniloArantesF commented Jul 1, 2024

DaniloArantesF commented Jul 1, 2024

MarcusLongmuir commented Jul 1, 2024

DaniloArantesF commented Jul 11, 2024

MarcusLongmuir commented Jul 15, 2024

DaniloArantesF commented Jul 24, 2024

MarcusLongmuir left a comment • edited Loading

Choose a reason for hiding this comment

MarcusLongmuir Jul 26, 2024

Choose a reason for hiding this comment

MarcusLongmuir Jul 26, 2024

Choose a reason for hiding this comment

MarcusLongmuir left a comment •

edited

Loading