mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
102.71k stars 35.38k forks source link

`Line2<LineGeometry,LineMaterial>` rendering issue after updating 164->165 #29863

Closed nyan-left closed 1 day ago

nyan-left commented 1 day ago

Description

After updating 164->165 Line2<LineGeometry,LineMaterial> rendering seems to be broken.

The migration guide does not mention any steps.

I suspect this is related: https://github.com/mrdoob/three.js/pull/28343/files

Reproduction steps

Install r164

  1. Create a Line2.
  2. Add it to your scene.
  3. Observe that the Line2 is visible and works as intended.
  4. Install r165.
  5. Observe that the line is barely visible and flickering.

Before:

Screenshot 2024-11-12 at 12 36 32

After:

Screenshot 2024-11-12 at 12 36 13

Note that the difference in rotation is unrelated as it's a screenshot of a rotating line.

Tested on Macos Chrome Version 130.0.6723.117 (Official Build) (arm64)

Code

import * as THREE from "three";
import "./styles.css";
import { Line2 } from "three/examples/jsm/lines/Line2";
import { LineMaterial } from "three/examples/jsm/lines/LineMaterial";
import { LineGeometry } from "three/examples/jsm/lines/LineGeometry";

function init() {
  const canvas = document.getElementById("canvas") as HTMLCanvasElement;
  const size = { width: window.innerWidth, height: window.innerHeight };
  const scene = new THREE.Scene();
  const camera = new THREE.PerspectiveCamera(
    45,
    size.width / size.height,
    0.1,
    1000
  );
  const renderer = new THREE.WebGLRenderer({ canvas });
  renderer.setSize(size.width, size.height);

  camera.position.z = 5;
  const directionalLight = new THREE.DirectionalLight("gray");
  directionalLight.position.set(1, 1, 1);
  scene.add(directionalLight);
  const ambientLight = new THREE.AmbientLight("gray");
  scene.add(ambientLight);

  const lineGeometry = new LineGeometry();
  lineGeometry.setPositions([0, 0, 0, 1, 0, 0]);
  lineGeometry.setColors([1, 0, 0, 1, 0, 0]);

  const lineMaterial = new LineMaterial({
    color: 0xff0000,
    linewidth: 0.01,
  });

  const line = new Line2(lineGeometry, lineMaterial);
  scene.add(line);

  const animate = () => {
    requestAnimationFrame(animate);
    line.rotation.x += 0.01;
    line.rotation.y += 0.01;
    renderer.render(scene, camera);
  };
  animate();
}

init();

Live example

r164 (expected) sandbox version below. In order to observe the issue, tweak the package.json to:

    "three": "0.165.0"

Screenshots

No response

Version

r165

Device

Desktop

Browser

Chrome

OS

MacOS

nyan-left commented 1 day ago

This is likely due to not calling lineMaterial.resolution.set in the first place, and I'm able to restore the width of the line in r165 by setting linewidth to a much, much higher value. I think this should be documented in the migration guide as it's a breaking change.

gkjohnson commented 1 day ago
 linewidth: 0.01,

In your example 0.01 is not a valid line width. This equates to 1/100th of a pixel in size. The only reason this might have worked in r164 is because resolution wasn't being set and otherwise defaults to 1, 1. This technically isn't a breaking change because the class wasn't being used correctly in the first place.

nyan-left commented 1 day ago

@gkjohnson

I understand the perspective that this isn't technically a breaking change, but I think there's a reasonable case that it is. The documentation's description of linewidth is quite minimal - just "Controls line thickness. Default is 1" - and while pixel units are implied through the worldUnits property, it's not explicitly stated anywhere.

My code was working in r164 with small linewidth values, and while the new behavior in r165 might be more technically correct, the change impacts existing applications without any heads-up in the migration guide or deprecation warnings.

I think it would be helpful to add this to the migration guide and make the units of measurement more explicit in the docs, just to help other developers who might run into the same issue. What do you think?

gkjohnson commented 1 day ago

I'll let someone else chime in on whether this should be added to the migration guide (cc @mrdoob) but there were no docs for Line2 back in r164 - if you weren't setting "resolution" then you weren't following the examples provided for the class. I understand it can be hard to follow the right way to use every example when docs aren't provided but at the same time I don't think we can be expected to document every incorrect use of a class that might break from a change. The documentation for linewidth could be updated to make more clear the the units are in pixels, if you'd like to make a PR for that.