pex-gl / pex-renderer

Physically based renderer (PBR) and scene graph for PEX.
https://pex-gl.github.io/pex-renderer/examples/index.html
MIT License
237 stars 16 forks source link

Where to keep default values #307

Open vorg opened 2 years ago

vorg commented 2 years ago

There are few variable here and there that if not set might create unexpected or black renders:

cachedUniforms.uClearCoatRoughness = material.clearCoatRoughness || 0.04

In the current 2.0 (pre-ECS) implementation those values are scattered across material and renderer with various value || default checks. That becomes a problem even more with ECS where material could be empty object. I wonder what could be unified approach to handling those.

dmnsgn commented 2 years ago

Philosophically, it sounds more like something a system (render) should worry about. But matching ALL material properties and if/def flags in pex-shaders should also be an objective and could potentially allow for conditionally building shader strings (if we ever tired of all these if/def preprocessor that aren't supported in WebGPU anyway).

dmnsgn commented 2 years ago

Practically, not having a fully formed component before the first system pass is an issue.

vorg commented 2 years ago

As mentioned in https://github.com/pex-gl/pex-renderer/issues/309 if we remove

entity.material = renderer.material({ baseColor: [1, 0, 0, 1] })
//in favour of
entity.material = { baseColor: [1, 0, 0, 1]} 

how does one discovers a) material b) other props like emissive

docs? system api comments?

vorg commented 2 years ago

Maybe the problem is not the component constructors but the name spacing of them. Maybe needing a reference to renderer to call renderer.material is the problem.

Standalone systems work well (if you know which ones needs ctx reference)

module.exports = (node, graph) => {
  const { systems } = require("pex-renderer");

  const triggerIn = node.triggerIn("in");
  const triggerOut = node.triggerOut("out");

  const renderSystem = systems.render({ ctx: graph.ctx });

  triggerIn.onTrigger = (props) => {
    renderSystem.update(props.entities, props.deltaTime);

    node.comment = props.entities.length;

    triggerOut.trigger(props);
  };
};

Maybe the same could work with components

import { components as cmp, entity as createEntity } from "pex-renderer";

const entity = createEntity({ //assigns unique id based on built-in entity counter   
   //assigns identity modelMatrix and empty worldBounds
   transform: cmp.transform({ position: [0, 0, 1] }),

   //assigns default roughness, metallic and sets all textures to null
   material: cmp.material({ baseColor: [1, 1, 1, 1] }) 
})

It gets a bit more problematic with components that need ctx for some of it's data but at least having null properties creates interface for how should they be called

import { components as cmd, entity as createEntity } from "pex-renderer";

const entity = createEntity({
  //assignes reflectionOctMapAtlas to null
  reflectionProbe: cmp.reflectionProbe() 
})

So we know it's reflectionOctMapAtlas not _ reflectionMap or reflectionMap.

Why still pushing for constructors / factory functions.

With e.g. material component it's the renderer that poly-fills it and uses it but with e.g. transform and transform.worldBounds the poly-filling / if null checks would need to happen in transform, render, lightProbe, shadow mapping and possible other systems.