[LukeP0WERS]

This led to some pretty extreme rendering artifacts, surprising considering its just a different tiebreak. I assume the CPU is being treated as the source of truth for the project, so this might need to be flipped if not.

[mkeeter]

Interesting! Do you have any easy ways to reproduce the rendering artifacts, e.g. a model + settings that I could run myself with fidget-cli?

I don't object to the fix – having matching CPU/GPU behavior isn't a hard requirement, but let's do it if it's easy – but having it make a significant visual difference doesn't make sense to me, so I'd like to dig a little further (could be a WGPU or driver bug)

[LukeP0WERS]

I haven't reproduced it with any model that can be shared easily. The gradients were being flipped in exactly 180 degrees right at the boundary of a box with empty space. I'm need to double check but I think the grad sample might have been exactly on the surface of the box, which might be a clue as to why things were going wrong, but I'll have to verify for sure.

[LukeP0WERS]

Yep the inversion only happens when sampling right on the surface of the box, which might explain why a tie would affect it. Sampling above or below the box gives the correct SDF pointing in the same direction, right on flips it without this fix.

[mkeeter]

Okay, here's my thoughts:

• If you are sampling at an ambiguous point, we have to pick one or the other gradient arbitrarily. There's no right solution here: this PR may fix your model, but if that particular min operation had its arguments swapped, then it would break!
• That being said, I agree that the GPU and CPU implementations should make the same choice
• However, the CPU implementation seems to match the previous GPU implementation, not the one in this PR:

  fidget/fidget-core/src/types/grad.rs

  Lines 169 to 181 in 25528e0

  	/// Minimum of two values
  	///
  	/// If either value is `NAN`, then this returns `NAN` (with empty gradients)
  	#[inline]
  	pub fn min(self, rhs: Self) -> Self {
  	if self.v.is_nan() || rhs.v.is_nan() {
  	f32::NAN.into()
  	} else if self.v < rhs.v {
  	self
  	} else {
  	rhs
  	}
  	}

So my disposition is to not merge this PR, since it would cause the CPU and GPU implementations to diverge.

[LukeP0WERS]

I should have taken a look at the other CPU functions, I was just looking at the x86_64 JIT build_min and the inconsistency is actually there, not in the shader.

[LukeP0WERS]

I dug into this a bit deeper, and I think the JIT version is the better convention. min(a, b) + max(a, b) should always be equal to a + b, so the gradient should be grad(a) + grad(b). If both sides pick rhs on a tie that becomes 2 * grad(b), and the gradient can be completely different from the distances that are tied.

[LukeP0WERS]

Whats nice is that fidget already sets up this identity to work no matter what, if you try to pass arguments in the order of min(a, b) + max(b, a) it will always be compiled to min(a, b) + max(a, b) since the arguments are sorted by node id:

fidget/fidget-core/src/context/mod.rs

Lines 215 to 224 in 25528e0

	/// Find or create a [Node] for the given commutative operation, with
	/// constant folding; deduplication is encouraged by sorting `a` and `b`.
	fn op_binary_commutative(
	&mut self,
	a: Node,
	b: Node,
	op: BinaryOpcode,
	) -> Result<Node, BadNode> {
	self.op_binary(a.min(b), a.max(b), op)
	}

[mkeeter]

Thanks, I've moved this over to #513 (rebased to remove unnecessary commits, and added a unit test early in the comit stack).