Replace colors with tint and vertexColors with colors

[vorg]

It would make it more clear what's happening as instanced color is multiplied by base color. I call it tint in glsl anyway https://github.com/pex-gl/pex-renderer/blob/master/glsl/PBR.frag#L1411

[vorg]

currently we have

renderer.geometry({
  positions: [],
  normals: [],
  vertexColors: []
  offsets:  { data: [], divisor: 1 },
  colors: { data: [], divisor: 1 }
})

renderer.geometry({
  positions: [],
  normals: [],
  colors: []
  offsets:  { data: [], divisor: 1 },
  tints: { data: [], divisor: 1 }
})

[vorg]

Another reason to use colors not vertexColors is glTF vertex color attribute is named COLOR_0 https://github.com/KhronosGroup/glTF/tree/master/specification/2.0#meshes

[vorg]

Added this to 3.0.0 milestone as it's breaking and we change it now or never..

Still undecided on the tints name though. Probably better than instanceColors though.

[dmnsgn]

Yes, that's probably going to be formalised in glTF 3.0. Meanwhile I am also undecided. instanceColors is more explicit but that means than anything that is instanced should follow the convention.

[vorg]

@dmnsgn any updates on that? e.g. progress of gltf discussion or maybe we should do research how babylon or three.js are naming things

[vorg]

ThreeJS is actually vertexColors and instanced colors and offsets https://github.com/mrdoob/three.js/blob/master/examples/webgl_buffergeometry_instancing.html#L153

BabylonJS seem not to have instanced color support? https://doc.babylonjs.com/how_to/how_to_use_instances

[vorg]

I would propose that unless we are willing to use instance* prefix we should close those issue and wait for GLTF to clarify.

geometry.set({
   positions: [],
   normals: [],
   vertexColors: [],
   offsets: [],
   scales: [],
   rotations: [],
   colors: []
})

vs 

geometry.set({
   positions: [],
   normals: [],
   colors: [],
   instanceOffsets: [],
   instanceScales: [],
   instanceRotations: [],
   instanceColors: []
})

[dmnsgn]

I agree. What's your gut feeling about instance* prefix? I am leaning toward this option as it makes it really explicit.

[vorg]

One thing about instance* prefix is that it would make it clear what's supported and what's instanced. We could then drop the requirement for { divisor: 1 } as it would be redundant.

The question is though: do we want to support custom (instanced or not) attributes as we have custom shaders? There are generally 3 options:

• you add way to add arbitrary attribute
• you hack your data into know attributes (if you have custom shader you can write custom handling)
• we add data_0, data_1, data_2, data_3 attributes

Writing this here just for reference because I think we should strive to do minimum work and not implement any of the above yet.

[vorg]

@dmnsgn @simonharrisco if we go ahead with this:

geometry.set({
   positions: [],
   normals: [],
   colors: [],
   instanceOffsets: [],
   instanceScales: [],
   instanceRotations: [],
   instanceColors: []
})

Then is it still this:

geometry.set({
 instanceOffsets: { data: [], divisor: 1 }
})

or is this now also (or only) valid:

geometry.set({
 instanceOffsets: [] // divisor is assumed as this is instanced attribute and defaults to 1
})

https://github.com/pex-gl/pex-renderer/blob/master/geometry.js#L106

this is similar to allowing both:

geometry.set({
 positions: { buffer: {}, offset: N, string: M }
})
geometry.set({
 positions: []
})

[vorg]

Good thing is all old examples will throw "unknown attribute" if we try to set e.g. offset https://github.com/pex-gl/pex-renderer/blob/master/geometry.js#L111

That will be easy to spot and fix instead of silent errors.

[simonharrisco]

geometry.set({
 instanceOffsets: [] // divisor is assumed as this is instanced attribute and defaults to 1
})

Seems more user friendly and feels nice with the other properties

[vorg]

To complicate things glTF uses TRANSLATION, ROTATION, and SCALE in EXT_mesh_gpu_instancing and doesn't support color or material properties as of today.

[vorg]

Note: one benefit of aInstanceColor and instanceColors would be automatic instanced attribute detection and auto divisor: 1 insert.

I would not change anything for now to avoid breaking all existing code.