Material Components (uv-scroll, video-texture-target, video-texture-source) #5911

netpro2k · 2023-01-25T21:41:43Z

This PR adds support for "material components". We allow adding hubs components to materials in GLTF files. So far we only used this for video-texture-source (and intended to go back and use it for uv-scroll). Previously we had no good way of implementing this, since components could only be on AFRAME entities, and entities could only be Object3Ds. This PR turns Materials into entities as well, so components can be added to them.

I am still slightly uneasy about the concept of making materials entitles (and the number of entities this adds), but it feels nice to actually be able to implement this correctly. If we discover we really do not end up creating more material components then we can consider special casing these instead, similar to what we did before.

Part of implementing video-texture-source was factoring out the code for updating render targets used in the camera tool system. Ideally we should probably refactor that to actually use video-texture-target and video-texture-source components. The code in this function was also simplified a bit resolving some (but not all) of the TODOs.

This also incidentally fixes issues that were present resolving node references in components (these are the first components we have implemented that use them).

part of #5899

johnshaughnessy

Nice work! Left some comments. I'll be curious if/when we ever bump into an invalidated assumption about entities always being associated with object3Ds. Even if down the road we decide again material entities, it's encouraging to me to see that we can "just" have entities and component data and it's fine

johnshaughnessy · 2023-01-25T23:59:37Z

src/app.ts

@@ -113,6 +115,7 @@ export class App {
    this.store = store;
    // TODO: Create accessor / update methods for these maps / set
    this.world.eid2obj = new Map();
+    this.world.eid2mat = new Map();


I (wrongly) assumed that the networking system would be making use of eid2obj, but this doesn't seem to be the case. I'd love to see a NetworkedUVScroll.

I think the way I would network sync a lot of the animation type stuff is just using serverTime instead. But your point stands it should be completely trivial to network stuff about material entities too, they are not special.

Network IDs would need to be assigned (deterministically) to materials in setNetworkedData...

Ah yeah that makes sense. That makes things a bit more tricky but still seems doable.

src/bit-components.js

johnshaughnessy · 2023-01-26T00:08:23Z

src/systems/remove-object3D-system.js

-  exitedObject3DQuery(world).forEach(removeFromMap);
+  entities.forEach(removeObjFromMap);
+  exitedObject3DQuery(world).forEach(removeObjFromMap);
+  exitedMaterialQuery(world).forEach(removeFromMatMap);


(Checking my understanding...)
It is OK not to call o.material ? o.material.eid ? removeEntity(world, o.material.eid) in the traverseSome above, because the various cleanup functions are going to call disposeMaterial as needed. We don't risk missing any material eids. Is that correct?

That's the idea yeah. I don't think it would be appropriate to remove a material entity just because an Mesh pointing at that material was removed. We specifically dispose of materials when we are done with them through various different means. All of them should always call disposeMaterial, which will call removeEntity, and then it will be removed from the map here.

johnshaughnessy · 2023-01-26T00:13:05Z

src/bit-systems/uv-scroll.ts

+  migrateLegacyComponents(world);
+  uvScrollQuery(world).forEach(function (eid) {
+    const map = (world.eid2mat.get(eid)! as MeshBasicMaterial).map;
+    if (!map) return; // This would not exactly be expected to happen but is not a "bug" either. There is just no work to do in this case.


I'm confused why this wouldn't be a bug. Is it valid to attach a UVScroll (material) component to something that isn't a material? Alternatively, is it valid to have a material eid that is not in eid2mat?

No. It must be on a material, and the material will be in the map (hence the !). What is not guaranteed, and I don't think inherently a bug is that a material can choose to not have a map. In practice you would not do this in a GLTF file, but I could see cases where we would be UV scrolling something and changing its map. Maybe turning it off? Not sure. Maybe that is not a real usecase.

johnshaughnessy · 2023-01-26T00:18:47Z

src/components/gltf-model-plus.js

-          }
+          mapMaterials(obj, function (m) {
+            disposables.add(m);
+          });


Yeah, in making sure we were calling disposeMaterial here it became clear that we didn't need to be so verbose. Also it was actually a bug before because we weren't calling dispose on the material itself.

johnshaughnessy · 2023-01-26T00:27:40Z

src/inflators/model.tsx

+  components: { [componentName: string]: any },
+  idx2eid: Map<number, EntityID>
+) {
+  Object.keys(components).forEach(name => {


Object.entries(components).forEach(([name, props]) => { ?

src/inflators/model.tsx

johnshaughnessy · 2023-01-26T00:33:38Z

src/inflators/model.tsx

+    const components = obj.userData.gltfExtensions?.MOZ_hubs_components;
+    if (components) inflateComponents(world, eid, components, idx2eid);
+
+    mapMaterials(obj, function (mat: Material) {


Should this check if the object was replaced by an inflator?
mapMaterials( world.eid2obj.get(eid) || obj,

The materials in the GLTF file certainly would not be on an object created by an inflator. If obj is a Mesh (only thing that can have materials from the GLTFLoader) and swapped, that's a bug, and we throw an exception below.

johnshaughnessy · 2023-01-26T00:39:02Z

src/bit-systems/video-texture.ts

+    const data = sourceDataMap.get(eid);
+    if (data && world.time.elapsed > data.lastUpdated + 1000 / VideoTextureSource.fps[eid]) {
+      updateRenderTarget(world, data.renderTarget, data.camera);
+      data.lastUpdated = world.time.elapsed;


Why does video-texture.ts set renderTarget.needsUpdate = false after updateRenderTarget whereas that does not happen here?

That was just an additional property I sloppily stuck on renderTarget in the camera-tool system. This system does not use that, though the purpose of that in camera-tool system was to only update the render target when the material preview is visible, which we should actually do here too...

johnshaughnessy · 2023-01-26T00:39:21Z

src/bit-systems/video-texture.ts

+
+function noop() {}
+
+export function updateRenderTarget(world: HubsWorld, renderTarget: WebGLRenderTarget, camera: EntityID) {


This function is tricky. (Not new)

Yeah I think it will become less tricky if we schedule it in the right spot relative to normal rendering. It might actually already be the case that the bone visibility stuff is no longer needed but I didn't want to revisit that now.

johnshaughnessy · 2023-01-27T04:45:58Z

src/bit-systems/video-texture.ts

+    } else if (source && entityExists(world, source)) {
+      bindMaterial(world, eid);
+    }
+  });


What happens if the source entity goes away, and this VideoTextureTarget.source[eid] pointed to it? Do we need to check for that and set VideoTextureTarget.source[eid] = 0, is this handled already, or is this not possible / unnecessary?

This should be handled just above if (!source || !entityExists(world, source)) {

netpro2k added 6 commits January 25, 2023 13:22

Add Material components and fix resolving noderefs

0a70f7a

video-texture-target & video-texture-source

0824568

Implement uv-scroll

07c1156

Add Material entities to ECS sidebar

d90f9ec

Fix legacy noderefs

ebcbae2

Fix some issues in videoTextureSystem

e062620

netpro2k had a problem deploying to hc-bio January 25, 2023 23:16 — with GitHub Actions Failure

johnshaughnessy approved these changes Jan 26, 2023

View reviewed changes

netpro2k added 2 commits January 26, 2023 00:13

Fix ref resolving in renderAsEntity

17683e1

srcNode -> source

ec78d8b

netpro2k had a problem deploying to hc-bio January 26, 2023 08:19 — with GitHub Actions Failure

Fix changing sources in videoTextureSystem

0f464de

netpro2k had a problem deploying to hc-bio January 26, 2023 22:21 — with GitHub Actions Failure

Delint

1d5ebd7

netpro2k had a problem deploying to hc-bio January 26, 2023 22:29 — with GitHub Actions Failure

johnshaughnessy reviewed Jan 27, 2023

View reviewed changes

netpro2k added 2 commits January 30, 2023 12:53

Merge branch 'master' into material-components

3e2aa35

Fix merge

ffb27af

netpro2k had a problem deploying to hc-bio January 30, 2023 21:06 — with GitHub Actions Failure

netpro2k merged commit e5f7385 into master Jan 30, 2023

netpro2k deleted the material-components branch January 30, 2023 21:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Material Components (uv-scroll, video-texture-target, video-texture-source) #5911

Material Components (uv-scroll, video-texture-target, video-texture-source) #5911

netpro2k commented Jan 25, 2023 •

edited by johnshaughnessy

Loading

johnshaughnessy left a comment

johnshaughnessy Jan 25, 2023

netpro2k Jan 26, 2023

johnshaughnessy Jan 26, 2023

netpro2k Jan 27, 2023

johnshaughnessy Jan 26, 2023

netpro2k Jan 26, 2023

johnshaughnessy Jan 26, 2023

netpro2k Jan 26, 2023

johnshaughnessy Jan 26, 2023

netpro2k Jan 26, 2023

johnshaughnessy Jan 26, 2023

johnshaughnessy Jan 26, 2023

netpro2k Jan 26, 2023

johnshaughnessy Jan 26, 2023

netpro2k Jan 26, 2023

johnshaughnessy Jan 26, 2023

netpro2k Jan 26, 2023

johnshaughnessy Jan 27, 2023

netpro2k Jan 30, 2023


		function noop() {}

		export function updateRenderTarget(world: HubsWorld, renderTarget: WebGLRenderTarget, camera: EntityID) {

Material Components (uv-scroll, video-texture-target, video-texture-source) #5911

Material Components (uv-scroll, video-texture-target, video-texture-source) #5911

Conversation

netpro2k commented Jan 25, 2023 • edited by johnshaughnessy Loading

johnshaughnessy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netpro2k commented Jan 25, 2023 •

edited by johnshaughnessy

Loading