Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Where to keep default values #307

Open
Tracked by #322
vorg opened this issue Jun 30, 2022 · 4 comments
Open
Tracked by #322

Where to keep default values #307

vorg opened this issue Jun 30, 2022 · 4 comments

Comments

@vorg
Copy link
Member

vorg commented Jun 30, 2022

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

  • default roughness: 1 and metallic: 1 are necessary evil in case you provide metallic / roughness texture (ideally they would be roughness=1 and metallic=1

  • emissiveIntensity has to be set (default to at least 1)

  • clearCoatRoughness should be at least 0.04

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.

  • how much should be in renderer.material()
  • how much should be internal in the render-system's entity._material or maybe even some entity._material.uniformCache
  • how much can be offloaded to pex-shaders and ifdef everything if not explicitly set in the entity.material e.g. clearCoatRoughness would require USE_CLEAR_COAT_ROUGHNESS or emissiveIntensity would need USE_EMISSIVE_INTENSITY instead of defaulting to forgotten uEmissiveIntensity=0.
@dmnsgn
Copy link
Member

dmnsgn commented Jun 30, 2022

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
Copy link
Member

dmnsgn commented Jun 30, 2022

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

@vorg
Copy link
Member Author

vorg commented Jul 5, 2022

As mentioned in #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
Copy link
Member Author

vorg commented Jul 8, 2022

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.

@dmnsgn dmnsgn added this to the 4.0.0 milestone Oct 19, 2022
This was referenced May 5, 2023
Draft
@github-project-automation github-project-automation bot moved this to In progress in 4.0.0 Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In progress
Development

No branches or pull requests

2 participants