Set WebGLRenderer's useLegacyLights = false by default

[mrdoob]

Related issue: #24975

Description

Testing how many examples break with this change.

[LeviPesin]

About half of examples break.

[mrdoob]

@LeviPesin Where can we see the diff images?

[LeviPesin]

After the run will be finished you will be able to see them in the Output screenshots artefact.

[mrdoob]

Found them. Thanks!

[mrdoob]

@donmccurdy regarding #24975 (comment)...

I still think this is too big of a breaking change and it'll be better to use the transition to WebGPU for this (ie. WebGPURenderer only has physically lights shaders and if someone wants to match WebGLRenderer and WebGPURender they'll have to set useLegacyLights = false in their project.

[gkjohnson]

I still think this is too big of a breaking change and it'll be better to use the transition to WebGPU for this

Can I ask what the concern with the change is? That people will ask why the lighting changed?

There will be adjustments like this that have to be made on WebGPU when it's released so it would be nice if the project could find a way to feel comfortable making changes like this especially since it's a one line change to "undo". Colors and lights haven't really been displayed correctly by default ever afaik and it feels like it's constantly being deferred. Some of this progress feels a little too glacial, imo.

One thing that might help improve the user visibility and understanding of impact of these types of changes is to make the "migration" page more prominent or understandable in the README - ie renaming it to something like "Migrating Breaking Changes" and moving the link location so it's more findable. Likewise the breaking changes could also be listed or at least more clearly linked at the top of the latest release notes. I've been surprised to see how many people don't know about the migration guide.

[mrdoob]

That's true... We could be better at making it more clear which changes are breaking and which ones are not.

[mrdoob]

I've spent some idle brain cycle on this today and it may indeed be a good idea do this change now so by the time WebGPURenderer is more usable there will be less things to think about.

If we're going to change this...
Should we also do the outputEncoding to outputColoSpace change (#23936) now and set it to sRGBColorSpace by default while we're at it?

Anything else?

/cc @donmccurdy @Mugen87 @WestLangley

[WestLangley]

Anything else?

texture.encoding -> texture.colorSpace

Discussed in 2017 in this lengthy thread.

[donmccurdy]

Summary of current vs. proposed defaults:

https://docs.google.com/spreadsheets/d/1Gv2SToWQSmsGfsp4V-Kh59nxAHMZqIJQ3QYMXvHhZ7s/edit#gid=0

All else being equal, making these changes cleanly in a single release would probably be best. Some changes offset the migration cost of others. Pairing ColorManagement.enabled = true with renderer.outputColorSpace = SRGBColorSpace means a codebase assigning colors with color.setHex( 0xABC ) does not have to change their code at all.

However, physical vs. legacy lights is less intertwined with the other color space-related properties. If we want to split it off I'm fine with that, too.

---

and set it to sRGBColorSpace by default...

Note that it's SRGBColorSpace, and not sRGBColorSpace. I've been tempted to use the string color space names from the Canvas 2D and WebGPU APIs, just to avoid that confusion. 😇

[mrdoob]

I'm going to commit the new screenshots as a first step before fixing the examples.

[mrdoob]

List of examples fixed:

• css2d_label
• games_fps
• misc_boxselection
• misc_controls_drag
• misc_controls_fly
• misc_controls_map
• misc_controls_orbit
• misc_controls_trackball
• misc_controls_transform
• misc_exporter_collada
• misc_exporter_draco
• misc_exporter_gltf
• misc_exporter_obj
• misc_exporter_ply
• misc_exporter_stl
• misc_exporter_usdz
• physics_ammo_break
• physics_ammo_cloth
• physics_ammo_instancing
• physics_ammo_rope
• physics_ammo_terrain
• physics_ammo_volume
• physics_oimo_instancing
• webaudio_orientation
• webaudio_sandbox
• webaudio_timing
• webgl2_multisampled_renderbuffers
• webgl_animation_keyframes
• webgl_animation_multiple
• webgl_animation_skinning_additive_blending
• webgl_animation_skinning_morph
• webgl_buffergeometry
• webgl_buffergeometry_compression
• webgl_buffergeometry_indexed
• webgl_buffergeometry_uint
• webgl_camera_array
• webgl_camera_cinematic
• webgl_clipping
• webgl_clipping_intersection
• webgl_clipping_stencil
• webgl_decals
• webgl_geometries
• webgl_geometries_parametric
• webgl_geometry_colors
• webgl_geometry_colors_lookuptable
• webgl_geometry_convex
• webgl_geometry_csg
• webgl_geometry_extrude_shapes
• webgl_geometry_extrude_shapes2
• webgl_geometry_extrude_splines
• webgl_geometry_minecraft
• webgl_geometry_nurbs
• webgl_geometry_shapes
• webgl_geometry_teapot
• webgl_geometry_text
• webgl_gpgpu_birds_gltf
• webgl_gpgpu_water
• webgl_helpers
• webgl_instancing_raycast
• webgl_instancing_scatter
• webgl_interactive_buffergeometry
• webgl_interactive_cubes
• webgl_interactive_cubes_gpu
• webgl_interactive_cubes_ortho
• webgl_layers
• webgl_lightningstrike
• webgl_lights_hemisphere
• webgl_lights_pointlights
• webgl_lights_spotlight
• webgl_loader_3dm
• webgl_loader_3ds
• webgl_loader_3mf
• webgl_loader_3mf_materials
• webgl_loader_amf
• webgl_loader_collada
• webgl_loader_collada_kinematics
• webgl_loader_collada_skinning
• webgl_loader_draco
• webgl_loader_fbx
• webgl_loader_gltf_compressed
• webgl_loader_gltf_sheen
• webgl_loader_ifc
• webgl_loader_kmz
• webgl_loader_ldraw
• webgl_loader_lwo
• webgl_loader_md2
• webgl_loader_md2_control
• webgl_loader_mmd
• webgl_loader_mmd_audio
• webgl_loader_mmd_pose
• webgl_loader_nrrd
• webgl_loader_obj
• webgl_loader_obj_mtl
• webgl_loader_ply
• webgl_loader_prwm
• webgl_loader_stl
• webgl_loader_texture_tga
• webgl_loader_ttf
• webgl_loader_usdz
• webgl_loader_vox
• webgl_loader_vrml
• webgl_loader_vtk
• webgl_lod
• webgl_marchingcubes
• webgl_materials
• webgl_materials_bumpmap
• webgl_materials_cubemap
• webgl_materials_cubemap_refraction
• webgl_materials_displacementmap
• webgl_materials_lightmap
• webgl_materials_normalmap
• webgl_materials_normalmap_object_space
• webgl_materials_physical_clearcoat
• webgl_materials_physical_reflectivity
• webgl_materials_subsurface_scattering
• webgl_materials_texture_anisotropy
• webgl_materials_variations_lambert
• webgl_materials_variations_phong
• webgl_materials_variations_physical
• webgl_materials_variations_standard
• webgl_materials_variations_toon
• webgl_materials_video
• webgl_math_obb
• webgl_modifier_curve
• webgl_modifier_curve_instanced
• webgl_modifier_edgesplit
• webgl_modifier_simplifier
• webgl_modifier_subdivision
• webgl_morphtargets Examples: Update webgl_morphtargets_*, use physically-based lights #25537
• webgl_morphtargets_horse Examples: Update webgl_morphtargets_*, use physically-based lights #25537
• webgl_morphtargets_sphere Examples: Update webgl_morphtargets_*, use physically-based lights #25537
• webgl_multiple_canvases_complex
• webgl_multiple_canvases_grid
• webgl_multiple_elements
• webgl_multiple_renderers
• webgl_multiple_scenes_comparison
• webgl_multiple_views
• webgl_nodes_loader_gltf_sheen
• webgl_nodes_materials_instance_uniform
• webgl_nodes_materials_physical_clearcoat
• webgl_nodes_materialx_noise
• webgl_portal
• webgl_postprocessing
• webgl_postprocessing_advanced
• webgl_postprocessing_backgrounds
• webgl_postprocessing_fxaa
• webgl_postprocessing_glitch
• webgl_postprocessing_outline
• webgl_postprocessing_pixel
• webgl_postprocessing_rgb_halftone
• webgl_postprocessing_sao
• webgl_postprocessing_sobel
• webgl_postprocessing_ssaa
• webgl_postprocessing_ssao
• webgl_postprocessing_ssr
• webgl_postprocessing_unreal_bloom
• webgl_raycaster_bvh
• webgl_read_float_buffer
• webgl_refraction
• webgl_rtt
• webgl_shadowmap_csm
• webgl_shadowmap_pcss
• webgl_shadowmap_performance
• webgl_shadowmap_pointlight
• webgl_shadowmap_viewer
• webgl_shadowmap_vsm
• webgl_shadowmesh
• webgl_skinning_simple
• webgl_water
• webxr_ar_dragging
• webxr_vr_ballshooter
• webxr_vr_cubes
• webxr_vr_dragging
• webxr_vr_handinput
• webxr_vr_handinput_cubes
• webxr_vr_handinput_pointerclick
• webxr_vr_handinput_pointerdrag
• webxr_vr_handinput_pressbutton
• webxr_vr_handinput_profiles
• webxr_vr_haptics
• webxr_vr_paint
• webxr_vr_panorama_depth
• webxr_vr_rollercoaster
• webxr_vr_sculpt

[Mugen87]

Based on the discussion in #25537, we should clarify how the examples should be updated.

The obvious and most pragmatic way is to just toggle the useLegacyLights flag and update the light properties so the previous visuals are restored.

The examples could also be updated in a more extensive manner: Making the scale of the scenes more realistic (assuming 1 world unit = 1 m) which ensures proper scaling of objects and consequently more reasonable lighting properties (position/distance and intensity/candela values). Besides, some examples might be overexposed or too dark or the contribution of certain lights to the overall lighting is worthy of improvement (e.g. #25537 (comment)).

Assuming the project wants to favor physically correct renderings, I vote for b) although this will make the updating process more time consuming.

[gkjohnson]

Assuming the project wants to favor physically correct renderings, I vote for b) although this will make the updating process more time consuming.

I think setting useLegacyLights to "true" for the examples is a good solution for the short term and the examples can be updated to more correct light intensity values over time and future releases. I don't think this should be blocked by trying to update all the examples at once.

[LeviPesin]

I also think b is better -- it is more time consuming, but we get in return that almost all examples will be physically-realistic.

[gkjohnson]

While testing I've noticed that almost all examples have an unacceptable visual appearance after setting useLegacyLights to true. Updating them to some extent will be required in any event.

Code-wise this PR is a one line change that swaps the default of useLegacyLights from true to false. Setting useLegacyLights to true in the examples must make them look just like this did before this change otherwise the flag is not behaving as expected. Are you sure there's not something else going on? Perhaps this is a bug?

[Mugen87]

@gkjohnson Sorry, I have misread your comment. I've also deleted my answer since it was misleading.

Yes, just adding renderer.useLegacyLights=true to the examples would be a third option and not require any more updates.

[gkjohnson]

Yes, just adding renderer.useLegacyLights=true to the examples would be a third option and not require any more updates.

To reiterate I think this should happen in steps and we should not block changing the default of this flag on updating all the light intensities in the examples. I think we do the following:

1. merge a PR that sets renderer.useLegacyLights = true in all examples possibly with a note in a comment.
2. merge this PR so the default of the field is changed to false.
3. update lighting in examples over time as availability permits and with some community help if possible.

[donmccurdy]

@gkjohnson I'd be concerned if we'd be turning on the option for users before we are comfortable using it ourselves. See the line of comments in #25537 for example. So I think it's necessary to at least go through the exercise of updating a number (TBD, not necessarily all ~200) of our existing examples before turning this on by default.

@WestLangley @mrdoob Given the feedback in #25537 and above, would you prefer one of these options?

• (A) We update all 200 examples, trying to achieve a similar visual result with useLegacyLights = false, but without non-blocking issues of scale and units.
• (B) We update 4-8 examples, with a detailed review and discussion.

I'm not personally eager to do the full overhaul on all examples linked above... 😇

Moreover, I'm not sure that all users need to be thinking in term of absolute units, even in useLegacyLights = false mode, so perhaps not all examples need to be written that way.

[mrdoob]

I was going to do (A) but the correctness discussion in #25537 scared me away 🙃

[gkjohnson]

@gkjohnson I'd be concerned if we'd be turning on the option for users before we are comfortable using it ourselves.

I agree - I just intended to suggest that it should not be necessary to block on every example having been updated in order to flip the flag default.

[WestLangley]

I have had extensive discussion with @donmccurdy offline. Here is a summary of what I think:

1. All we will do by setting useLegacyLights to false is change how point and spot lights decay. That means users will have to set different intensity values for those lights.
2. If users want to set SI units for their intensities, they must use meters for distance.
3. We already have webgl_lights_physical.html. I'd keep that example, but rename it. I think the examples that set SI units should all be named webgl_units_physical.
4. I do not think we need many such examples (4, or so). Each physical units example should have a specific purpose (to be discussed).
5. The remaining examples can be "unitless". (Even if they load a glTF model.)
6. The "physically-based lights" term will go away. We will use the terms "physical units" or "unitless".

[donmccurdy]

Perhaps webgl_lights_physical_units rather than webgl_units_physical? Just to keep the terms in an order that sorts categorically.

I was going to do (A) but the correctness discussion in #25537 scared me away 🙃

@mrdoob I think the implication of @WestLangley's comment above is that we only want to have the correctness discussion for a few (~4) examples. The remainder can be converted by eye, without worrying much about units. This still leaves us a choice of (A) or (B).

[Mugen87]

After a series of PRs, it should be possible to change the default value of useLegacyLights without breaking the examples, editor and docs. The examples were updated base on option (A) from #25479 (comment). Meaning I've made sure the previous looks were mostly restored (with some minor tweaks) when disabling legacy lighting. Additionally, I have updated the scale of many scenes to a more real world standard.

When changing the default we should consider to directly deprecate the legacy lighting code path so it can be removed in ten releases.

While migrating the examples, I've noticed it is not always obvious how to restore the lighting in the scene especially when spot and point lights are used. Next to a note in the migration guide, it would be good to highlight the change in the forum with a separate topic (similar when color management was turned on by default).

All we will do by setting useLegacyLights to false is change how point and spot lights decay.

The light intensities also change since the "artist-friendly light intensity scaling factor" Math.PI is not used anymore. So next to point and spot lights, all ambient, hemisphere and directional lights as well as light maps also change and have to be updated on app level in order to restore previous looks.

[Mugen87]

Let's make a fresh PR that changes the default of useLegacyLights and deprecates it at the same time. I suggest to target r155 so we have enough time to verify this change in dev. r154 is already too close, imo.

[mrdoob]

Let's make a fresh PR that changes the default of useLegacyLights and deprecates it at the same time. I suggest to target r155 so we have enough time to verify this change in dev.

Sounds good!

[Mugen87]

Okay, I'll try to file a PR in the next days!