Per-point sizes and colors

[brisvag]

Trying my hand at a first contribution. I'm a bit confused by how buffers are bound and accessed.

Right now, this doesn't work (sizes are not read at all, it seems).

[almarklein]

Seems like you've found your way around the code!

A note about that index. In quite a few shaders we don't have vertex buffers but use storage buffers (i.e. random access buffers). This allows us doing geometry-shader like stuff. In the point shader the in.vertex_index runs until 6x the number of points. In the shader we use the modulo operation to map these to the 4 corner positions, forming 2 triangles.

[almarklein]

BTW we use black . for autoformatting and flake8 . for style checks.

[brisvag]

I think it's working. Not sure if all those hasattr and $$ if ... are clean. I assume it's better for performance to minimize the amount of stuff handled by the shader in the first place?

[Korijn]

This looks great! Thank you! Looks very clean to me.

[almarklein]

I suggested two tweaks that will allow putting these geometry attributes to None. Other than that this looks great!

There's indeed quite a few $$ if now. It would also be possible to always pass the color and size as a varying, so there'd be only one triage in the vertex shader, making the code a bit easier to read. However, the current version is probably more performant (no idea how much it matters in practice).

[almarklein]

Another small request: we should update the docstring of the width and color props on the material to say that if the geometry has these attributes, these are used instead.

[Korijn]

Thanks to the discussion in #176 I realized this change is (in a very small way) inconsistent with the rest of the API. The Material type and its configuration should always determine what the rendering strategy will be. The geometry object provides the structured data, but it does not determine what rendering strategy is utilized. So to resolve this, and be consistent with many other options we have, boolean flags should be added PointMaterial to enable the use of per-vertex colors, and sizes. This is how it works in threejs and it seems very sensible to me: Geometry provides data, Material determines the rendering strategy.

So as a concrete proposal, adding point_sizes, point_colors as two boolean properties on PointMaterial to independently enable the use of the vertex buffers on the geometry would be sufficient. Then you don't need to check for the existence of geometry buffers; if these flags are toggled on, you can expect them to be available and otherwise the render function may raise an exception.

I was sure we had also implemented vertex colors this way already but it looks like we don't have any examples in pygfx yet.

[almarklein]

I thought ThreeJS had similar flags somewhere, but I can't find them right now.

So as a concrete proposal, adding point_sizes, point_colors

This is the first time we have this situation, but we'll have more cases in the future (like mesh per-vertex and per-face colors). So let's bikeshed a little.

What about instead of a separate property, we define a special value, e.g. PointMaterial(color="pervertex")

[Korijn]

I thought ThreeJS had similar flags somewhere, but I can't find them right now.

So as a concrete proposal, adding point_sizes, point_colors

This is the first time we have this situation, but we'll have more cases in the future (like mesh per-vertex and per-face colors). So let's bikeshed a little.

What about instead of a separate property, we define a special value, e.g. PointMaterial(color="pervertex")

That is also fine with me.

I also tried looking it up in threejs but had some trouble finding it in the docs for some reason. However it is still the case, for example: https://threejs.org/examples/?q=color#webgl_geometry_colors

You can see the materials used there have flags to enable vertex colors, flat shading, wireframe and transparency:

const material = new THREE.MeshPhongMaterial( {
  color: 0xffffff,
  flatShading: true,
  vertexColors: true,
  shininess: 0
} );

const wireframeMaterial = new THREE.MeshBasicMaterial( { color: 0x000000, wireframe: true, transparent: true } );

[brisvag]

What about instead of a separate property, we define a special value, e.g. PointMaterial(color="pervertex")

I like this more: no conflicting arguments. One step further: does this have to be hardcoded to use a specific name in the geometry?

geometry = gfx.Geometry(positions=positions, arbitrary_name=some_data)
material = gfx.PointsMaterial(color='arbitrary_name')

[Korijn]

I think it is good to expect a fixed name (e.g. colors and sizes). It makes code consistent and is just less configuration for users to worry about.

I'm not sure but is the color attribute name not standardized even in gltf etc?

[brisvag]

To exapnd a bit on it, the use case I was imagining is using the same geometry data for different purposes, and possibly the ability to provide a quick conversion:

positions.shape  # (100, 3)
some_data.shape  # (100,)

to_red = """
// shader func that takes a float and returns a vec4(float, 0, 0, 1)
"""

geometry = gfx.Geometry(positions=positions, arbitrary_name=some_data)
material = gfx.PointsMaterial(color='arbitrary_name', color_func=to_red, some_other_property='arbitrary_name', some_other_property_func=...)

As I type this out, I realize it;s probably way out of scope, but that's where my head went :P

[almarklein]

I think it is good to expect a fixed name

Also see #68.

I realize it;s probably way out of scope, but that's where my head went :P

Interesting. IIRC Bokeh does something similar. Also e.g. Mesh(geometry=..., Material(color="normals")).

That said, perhaps a bit too much for now. We could revisit the idea when we're more clear on what geometry is ;P

[Korijn]

I just checked; it doesn't look like there are any standards for geometry attribute names. So we just need to be internally consistent, as Almar also indicated, reference #68.

For now, I would recommend just sticking to sizes and colors as geometry attribute names. We may do a mass rename later.

[brisvag]

So, to recap:

• geometry should use sizes and colors as attribute names
• material should either:
  • have boolean flags like point_sizes to enable using the geomtry-provided values, or
  • allow a special value for size such as pervertex

[Korijn]

That's right.

[almarklein]

material should either:

• have boolean flags like point_sizes to enable using the geomtry-provided values, or
• allow a special value for size such as pervertex

I'm not sure which I like better. I dislike having tons of flags in the future for every property that can also be set per-vertex. But the fact that a color property can be something different than a color is also not great.

If we go with the boolean flag, let's use something that looks more like a boolean. E.g. use_vertex_color?

Ha, @Korijn do you remember that in Arbiter 1.0 we had props that could be set to e.g. ":x" to select column x? Memories :) Also, here's a link to Bokeh docs showing a similar pattern to use string names to select columns from a datasource: https://docs.bokeh.org/en/latest/docs/user_guide/data.html

[Korijn]

material should either:

• have boolean flags like point_sizes to enable using the geomtry-provided values, or
• allow a special value for size such as pervertex

I'm not sure which I like better. I dislike having tons of flags in the future for every property that can also be set per-vertex. But the fact that a color property can be something different than a color is also not great.

If we go with the boolean flag, let's use something that looks more like a boolean. E.g. use_vertex_color?

Ha, @Korijn do you remember that in Arbiter 1.0 we had props that could be set to e.g. ":x" to select column x? Memories :) Also, here's a link to Bokeh docs showing a similar pattern to use string names to select columns from a datasource: https://docs.bokeh.org/en/latest/docs/user_guide/data.html

Let's do the flag!

[Korijn]

We just figured out while reviewing some code with Almar and Berend that we do have an example of vertex coloring in the repo already, and its on LineMaterial, see here:

pygfx/pygfx/materials/_line.py

Lines 39 to 47 in a11f250

	@property
	def vertex_colors(self):
	return self._vertex_colors
	@vertex_colors.setter
	def vertex_colors(self, value):
	if value != self._vertex_colors:
	self._vertex_colors = value
	self._bump_rev()

So you can reference how that works as an example :)

[brisvag]

Updated. It all seems to work, except colors are only shades of gray. I assume I did something wrong with types somewhere in the shaders, but I can't find it.

[almarklein]

Sorry for opening the discussion once more ... but one problem with vertex_colors, is that at some point we'll probably also support per-face colors (#152), and having both vertex_colors and face_colors wouldn't be great :)

---

edit

Taking a step back, a user may want to apply color in different ways (this applies to Mesh, Line, and Points:

• Just one color for the whole mesh -> material.color .
• Applying per vertex (arbitrary) values and colormapping these -> geometry.texcoords.
• Applying per vertex colors directly.
• Applying per face (arbitrary) values and colormapping these.
• Applying per face colors directly.

(It's good to know that we already do support per-vertex colors before this PR, because a user can set the texcoords to the colors, and then use a unit 3D colormap. This approach is far from obvious, which is why we decided to also support explicit per-vertex colors.)

What about a color_mode enum, that can be set to "plain", "texcoords", "vertex", "face", or "auto". In "auto" mode the behavior is to use the texcoords + colormap if material.map is given, and use material.color otherwise.

[brisvag]

Good point... More and more I lean towards the bokeh approach.

[Korijn]

Can we tackle that when we get to implement face colors?

[almarklein]

Can we tackle that when we get to implement face colors?

I edited my post above. But yeah, we can also just go with what we have now and fix it later. Seems like that needs more thought/discussion anyaway.

[almarklein]

It now works correctly, right @brisvag ?

[brisvag]

Yes! This is the updated example: