Toggling Light visibility causes a slow frame due to octree pairing

[jamie-pate]

Godot version:

3.2.3-stable, 2882284 (3.2 branch tip at the time)

OS/device including version:

Affects all platforms
OpenGL ES 3.0 Renderer: GeForce GTX 1070 with Max-Q Design/PCIe/SSE2
Pop!_OS 20.04 LTS
Linux

Issue description:

Toggling light visibility can take several milliseconds in complex scenes, leading to dropped frames.
I'm attempting to hide rooms that are not being drawn, but when the lights are inside the room their visibility is toggled which causes a single frame to take ~35ms.

Diving into the code leads me to the octree pairing in visual serve.instance_set_visible() > instance->scenario->octree.set_pairable()
Snapshot of the octree pair_map size in my project. I assume 50,000 is a big number?

Steps to reproduce:
Toggle on and off some lights in a complex project. I'm using multiple gridmaps and toggling visibility on their ancestor when the player leaves the vicinity. The lights are toggled at the same time because they share the toggled ancestor. In the reproduction here i just toggle a container with lots of lights in it.

Minimal reproduction project:

Click the checkbox here, lose a tonne of frames. Not as dramatic as my actual project, but it hits the same codepaths and still slows down.
platformer_repro_light_pairing_slow.zip

Here is a patch to show the visibility toggle times in the profiler:
object-profiling.diff.txt

The Object value is doubled because there's multiple stats for the same codepath :o but should it shows the problem

[lawnjelly]

I'm actually getting an increase from about 30fps to 40fps when switching on the slowness checkbox .. but maybe it's because of my slow GPU running faster with the lights off, there could still be something slow going on in the octree. I'll try and profile tomorrow and see if I can see anything. I use an external profiler, callgrind.

[jamie-pate]

Ya, maybe it would be better to duplicate the light containers, and turn one off while turning the other on, then the same number of lights would be on for both phases

[lawnjelly]

I can see some slowness in the octree _cull_convex routine, however unfortunately I can't tell from this project whether this is problematic or unexpected, which deters spending further time on it (it could be a wild goose chase).

The main difficulty as far as I'm concerned is that we need a minimum reproduction project to investigate this. This project has too much going on to be able to work out what you think is a problem, determine whether it is actually a problem, and figure out what might be causing it, and whether anything can be done about it.

A minimum reproduction project might consist of say, generating 100,000 random cubes, and turning on and off a light, and determining that this is slower than would be expected. But not moving them, unless moving them was key to reproducing the problem.

[jamie-pate]

I'm fine with the solution being "just don't do that" for now.

I've got a workaround that is working quite well, and i'm satisfied if this is fixed in the changes coming in 4.0. The spikes are now down to 1ms from 35 :)

After loading the scene I walk the tree, and replace all lights with this proxy object, which will migrate the actual node to the root of the scene. I've already got the proxy set up to remove them from the scene in the case of the lowest detail setting, because some of my scripts rely on addressing them in the tree, and just turning them off doesn't offer the same performance benefit.

extends Spatial

const LodValues = preload('./LodValues.gd')

var target: Light setget _set_target
var lod: int setget _set_lod

# properties to override based on LOD
var _props := {}


func _set_lod(value: int):
	lod = value
	if target:
		var tree := get_tree()
		# remove lights completely if we set max_low_detail
		if value == LodValues.MAX_LOW_DETAIL:
			if target.get_parent():
				target.get_parent().remove_child(target)
		else:
			if target.get_parent() != tree.root:
				if target.get_parent():
					target.get_parent().remove_child(target)
				tree.root.add_child(target)
		# Just disable shadows for LOWER_DETAIL
		target.shadow_enabled = false if lod == LodValues.LOWER_DETAIL else _props.shadow_enabled


func _enter_tree():
	_set_lod(lod)

func _exit_tree():
	if target && target.get_parent():
		target.get_parent().remove_child(target)


func _set_target(value: Light):
	target = value
	_props.shadow_enabled = target.shadow_enabled
	_set_lod(lod)

func _get_property_list():
	if target:
		return target.get_property_list()
	else:
		return get_property_list()

# https://docs.godotengine.org/en/stable/classes/class_object.html?highlight=Object#class-object-method-get
func _get(property: String):
	if property == 'target':
		return target
	elif property == 'lod':
		return lod
	elif property == 'name':
		return name
	elif target:
		if property in _props:
			return _props[property]
		else:
			return target.get(property)
	# worrying that the return value for 'property doesn't exist' is null
	return null

func _set(property: String, value):
	var result = true
	if property == 'target':
		_set_target(value)
	elif property == 'lod':
		_set_lod(value)
	elif property == 'name':
		name = value
	elif target:
		if property in _props:
			_props[property] = value
		target.set(property, value)
		_set_lod(lod)
	else:
		result = false
	return result

[jamie-pate]

Ok, i went and made it a plugin: https://github.com/jamie-pate/godot-virtual-light

[akien-mga]

Might be fixed by #44901 (will be available in 3.2.4 beta 6 to test).

[akien-mga]

Assuming fixed by #44901, please comment if you can still reproduce the issue in 3.3.