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
234 stars 16 forks source link

Proposal: rename texture.texCoordTransformMatrix to texture.transform #318

Closed vorg closed 10 months ago

vorg commented 1 year ago

We are currently using texCoordTransformMatrix, USE_BASE_COLOR_MAP_TEX_COORD_TRANSFORM and uBaseColorMapTexCoordTransform. While informative on the user side the code looks like

const material = {
  baseColorMap: {
     texture: texture,
     texCoordTransformMatrix: mat3
  }
}

in (old) ecs.Renderer i was using transform which would make the code look like this

const material = {
  baseColorMap: {
     texture: texture,
     transform: mat3
  }
}

I propose we simplify the API. The question is do we change pex-shaders too? e.g. uBaseColorMapTexCoordTransform -> uBaseColorMapTransform. The extension is called KHR_texture_transform so proposed name is more in line with gltf IMO.

vorg commented 1 year ago

Which reminds me of #310 Consider renaming maps to textures which would cause

uBaseColorMap
uBaseColorMapTexCoordTransform

uBaseColorTexture
uBaseColorTextureTransform //yay!
dmnsgn commented 1 year ago

So new components will look like that:

// basic
const material = {
  baseColorTexture: texture
};

// or with transform
const material = {
  baseColorTexture: {
     texture,
     transform: textureTransform
  }
};

Fine by me. Renaming in pex-shaders is needed as well yes, so that's a big breaking change where we need to be careful with VSCode search & replace.

vorg commented 1 year ago

Well it's actually not that simple as transform in KHR_texture_transform is an object with offset, rotation and scale and not a matrix so it could be confusing.

So it would have to baseColorTexture.transformMatrix (still shorter) and uBaseColorTextureTransformMatrix

dmnsgn commented 1 year ago

At the moment, materials texture.texCoordTransformMatrix is not set directly. Instead, the more user friendly texture.offset/rotation/scale is used and the matrix only computed on set(). So you can either provide:

The three questions:

vorg commented 1 year ago

Instead, the more user friendly texture.offset/rotation/scale is used

Is it? :D I thought all we have is texCoordTransformMatrix

is renaming texCoordTransformMatrix to transformMatrix okay

Yes

can we still allow texture.offset/rotation/scale and compute texture.transformMatrix in material system (render-pipeline?)

How to name and pack given uniforms is up to renderer. Are there any other use cases for Material System? As of now the most appropriate place is standard material.

can we animate update these properties at runtime, if so, how do you tell texture.transformMatrix to be recomputed?

Are we thinking arcade game scrolling textures here? :)

dmnsgn commented 1 year ago

Instead, the more user friendly texture.offset/rotation/scale is used

Is it? :D I thought all we have is texCoordTransformMatrix

Yep, here we compute the matrix: https://github.com/pex-gl/pex-renderer/blob/5cced079d6b6330cf42656c57200d2c49e517806/material.js#L122-L134

can we still allow texture.offset/rotation/scale and compute texture.transformMatrix in material system (render-pipeline?)

How to name and pack given uniforms is up to renderer. Are there any other use cases for Material System? As of now the most appropriate place is standard material.

No but maybe in a shared space if basic renderer or any other wants to use textures and texture transform?

can we animate update these properties at runtime, if so, how do you tell texture.transformMatrix to be recomputed?

Are we thinking arcade game scrolling textures here? :)

Thinking texture spritesheet or something ;)

vorg commented 1 year ago

Oh that's leftover code that has not been ported yet. The thing with material system is that renderers are building materials atm. And texture matrix update system would have to know all possible texture names or sniff for objects with texture property in material components, not optimal.

My plan for standard material was to support all possible 2d/3d mesh like scenarios that would involve textures and where texture transforms make sense. Other renderers like particles/lines/trails would less. Although i see usecase for spritesheets in those. Will it be full matrix with translate rotate scale though? Or rather offset, size as vec4? Just don't want to force generalization that is not there.

vorg commented 10 months ago

Update.

Given that we udate uBaseColorMap to uBaseColorTexture we would end up with uBaseColorTextureTexCoordTransform so maybe we should rename this too to uBaseColorTextureTransformMatrix and following material prop:

const material = {
  baseColorMap: {
     texture: texture,
     transformMatrix: mat3
  }
}
dmnsgn commented 10 months ago

Suggested naming:

const material = {
  baseColorTexture: {
     texture: texture,
     matrix: mat3,
     texCoord: int,
  }
}

uniform mat3 uBaseColorTextureMatrix;

Flags: USE_BASE_COLOR_TEXTURE USE_BASE_COLOR_TEXTURE_MATRIX BASE_COLOR_TEXTURE_TEX_COORD