Nonuniform icosphere scaling + mesh generation questions

[henrynoyes]

Hey @brentyi ! I have a couple small things to bring up for discussion:

1. Nonuniform icosphere scaling

I've been working on a project that involves ellipsoidal geometries, and noticed that the current implementation of icosphere generation does not support nonuniform scaling. I wanted to get your thoughts on an approach to supporting this. Below is the first idea that came to mind, which adds a 3-tuple scale argument to add_icosphere() that is multiplicative with the existing radius parameter.

# _scene_api.py
def add_icosphere(
	self,
	name: str,
	radius: float = 1.0,
	color: RgbTupleOrArray = (255, 0, 0),
	*,
	subdivisions: int = 3,
+	scale: tuple[float, float, float] = (1.0, 1.0, 1.0),

# IcosphereMesh.tsx
+  // Calculate scaling values.
+  const scale = message.props.scale.map((s: number) => s * message.props.radius) as [number, number, number];

  return (
    <group ref={ref}>
      <mesh
        geometry={geometry}
-       scale={message.props.radius}
+       scale={scale}

Full diff

2. Transparent mesh depth writing

When playing around with transparent geometries, I ran into some scenarios where parts of a transparent mesh would appear opaque from certain angles (see video below). These visual artifacts were further exaggerated when the scene included multiple overlapping meshes. Observing some other material implementations, I added a flag to toggle depthWrite in the createStandardMaterial function when the mesh is transparent, which seems to have fixed the issue. Let me know if this seems right or if it is misguided.

+const isTransparent = props.opacity !== null && props.opacity < 1.0;
const standardArgs = {
  color: props.color === undefined ? 0xffffff : rgbToInt(props.color),
  wireframe: props.wireframe,
- transparent: props.opacity !== null,
+ transparent: isTransparent,
  opacity: props.opacity ?? 1.0,
+ depthWrite: !isTransparent,

mesh-depth-writing.webm

3. Argument order in add_box()

Lastly, I noticed the order of the core args in add_box() does not agree with the other add_{geom}() functions.

def add_box(
	self,
	name: str,
	color: RgbTupleOrArray = (255, 0, 0), # color before dimensions
	dimensions: tuple[float, float, float] | np.ndarray = (1.0, 1.0, 1.0),
...

def add_icosphere(
	self,
	name: str,
	radius: float = 1.0,
	color: RgbTupleOrArray = (255, 0, 0), # color after dimensions
...

Basically, just swapping the order of color and dimensions in add_box(). A bit picky, but worth changing?

I would appreciate your thoughts on these three points. I am planning on opening a PR for nonuniform-icosphere-scaling, which could either encapsulate all of these changes, or only be scoped to 1 with 2 and 3 left as separate fixes.

Thanks!

[brentyi]

Hello @henrynoyes! Thanks for the thoughts!

1) Icosphere scaling

I agree this is useful! For supporting this: it could also make sense to try and implement scale support for all scene nodes? #531

2) Don't write to depth buffer for transparent materials

No silver bullet here sadly; transparency artifacts are hard to avoid when rasterizing. Turning off depth write improves some cases, but will create different artifacts in others.

The "worst case artifacts" I can think of seem less egregious for the current behavior. Here's a simple case to illustrate:

import numpy as np
import trimesh.creation

import viser

icosphere = trimesh.creation.icosphere(subdivisions=3, radius=1.0)

server = viser.ViserServer()
sphere_handle = server.scene.add_mesh_simple(
    "/long",
    vertices=icosphere.vertices * np.array([4.0, 1.0, 1.0]),
    faces=icosphere.faces,
    opacity=0.9,
    color=(0, 0, 255),
)
sphere_handle = server.scene.add_mesh_simple(
    "/sphere",
    vertices=icosphere.vertices * 1.5,
    faces=icosphere.faces,
    opacity=0.9,
    color=(255, 0, 0),
)
server.scene.show()

This is what the current code should produce: depthWriteTrue.html

With depthWrite set to false: depthWriteFalse.html

3) add_box argument order

Sounds good! It's a breaking change but we usually expect these fields to be passed in via kwargs, so probably OK in practice. As due diligence: we could search GitHub to see if it will break any existing projects.

[henrynoyes]

1. Interesting, I suppose that would be a much more encompassing approach. I can try taking a stab at this as part of the PR. If you have any jumping-off points for the implementation feel free to let me know.
2. Ah, I see, sort of a lesser of two evils. This makes sense. Thanks for throwing together the explanation!
3. Ok, good idea. I'll poke around to see if it will cause any downstream issues before adding the change.

[brentyi]

For #1: yeah that's a lot to ask actually, maybe you can make a PR with just the icosphere change (which you already have?) and then I can think about/work on generalizing it?

[henrynoyes]

Sounds good to me! Just submitted #649. I'll close this.