Adds m-material element to MML

DaniloArantesF commented 4 months ago

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.

https://github.com/mml-io/mml/assets/33551518/b1a6dec4-530e-40f1-bc41-5cad35729301

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)

[ ] Bugfix
[X] Feature
[ ] Refactor
[ ] Tests
[ ] Other, please describe:

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

[ ] Yes
[X] No

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

Does your PR fulfill the following requirements?

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

GwilymIO commented 4 months ago

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

DaniloArantesF commented 4 months ago

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

chrome_szgifVhzL5

<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 commented 4 months ago

@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 commented 4 months ago

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 commented 3 months ago

@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 commented 3 months ago

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.

DaniloArantesF commented 3 months ago

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

mml-io / mml

Adds m-material element to MML #181