Eliminates SpringArm update lag when changing SpringArm length

[Nanotross]

Desciption
If the SpringArm's length is updated, its children won't update until the next frame because the process_spring() function won't be called again until the next frame. Any code that needs the position of its children to update in the same frame as the SpringArm length is updated won't get the values based on the updated length until the next frame.

The fix makes the set_length() function only change the SpringArm length when the new length is different than the current length and calls the process_spring() function to update the SpringArm and its children with the new length, which eliminates the lag.

Before/After Example
The zip file contains 1 second videos of where the bug occurs, before and after the fix (the videos are from 3.2, but affected code is also present in 3.1, 3.2, and 4.0).

The update lag is most apparent when the camera is rotating while the length changes. In the videos, when the camera rotates to its maximum rotation above the character, the SpringArm zooms out the camera a little after the rotation has stopped because the SpringArm's update lags doesn't update its children until 1 frame after the length is updated.

The effect of the update lag can be seen by looking at the shadow to the left of the character in both videos and comparing them.

The update lag can also be seen in the output. The top value is the new length, and the next two values are two calculations of the length to a child object of the SpringArm. Because the SpringArm didn't update in the same frame the length was changed, the second and third values lag the first (in the before video).

SpringArm_Update_Bug_Before_After_Fix.zip

Affected Versions
3.1, 3.2, and 4.0

[Nanotross]

I've updated it to stop unnecessary gizmo updates.

The start of the method is now

if (p_length == spring_length) {
	return;
}

because there is nothing to update if the new length is the same as the current length.

[akien-mga]

The implementation seems good now, but I'm not sure if the bug described is actually valid.

Like all physics code, process_spring() is meant to run in the physics frame, so any changes made from your code will only be applied but taken into account only the next frame:

- physics frame
- your code changes
- physics frame
- your code now sees changes done

So forcing a recalculation here might actually go against Godot's design.

[Nanotross]

The issue is that any code dependent on the position of a child of the SpringArm node won't be updated until the frame after the SpringArm length updates.

Example:
If the SpringArm's length is set based on an angle of a Camera, and that Camera is a child of the SpringArm, then the Camera's angle will update and be seen a frame before the Camera's position.

- physics frame
- process_spring()
- camera angle changes
- set length based on new camera angle
- physics frame
- process_spring()
  - camera position updates
- physics frame

This leads to the camera position lagging the angle by one frame, when they should be updated during the same frame (at least for it to look good).

The other option is to make the process_spring() function public instead of private and let the developer call it when needed. Though, this adds more burden onto the developer to identify and avoid this issue. It also "might actually go against Godot's design" because it would allow process_spring() to be called outside of the physics frame.

If the priority is to adhere to "Godot's design" over responsive implementation, then I'll just have to create a custom SpringArm object that isn't bound by the physics frame.

[akien-mga]

This might no longer be needed after #37911. SpringArm was running process_spring() in PROCESS instead of PHYSICS_PROCESS, hence the extra lag. CC @madmiraal

[Nanotross]

I've taken an older version of my project (where the issue still existed) and removed everything except what's necessary to reproduce the issue.

I tested the simplified project using a custom build with the changes from #37911 and #37964, but the issue still remained.

I've attached the project if you want to check it out for yourself.

SpringArm_Issue_Check.zip

Important notes:

• The frame rate is capped at 15 fps to accentuate the issue (the issue is less noticeable at higher frame rates because it's only a 1 frame update delay). Feel free to test other frame rates.

• The 'W' and 'S' keys move the camera up/down respectively at a fixed rate. This is done to make it easier to identify the issue as the issue occurs when the camera stops moving (such as when the camera reaches the maximum vertical rotation).

• I've positioned the player such that the shadow to the left will be near the top left of the screen when the camera is rotated to be fully over the player. The issue is seen when the camera reaches the maximum vertical rotation and stops.

• Looking at the shadow that is now at left of the screen is the clearest place to see the issue. It make take repeatedly moving the camera up and down using 'W' and 'S' to see the issue clearly.

• To reiterate on earlier comments, the issue is that the children of the SpringArm don't update until the frame after changes to the length of the SpringArm are made. This results in the camera "jumping" when it stops moving. It's most noticeable when the camera reaches the maximum vertical rotation because the difference in length is greatest there.

[madmiraal]

This change will result in the same crash seen in #32876 and fixed by #37911 and #37964. Unfortunately, to be able to check for collisions, process_spring() must run within physics. Therefore process_spring() cannot be run from within set_length(), because set_length() can be (and is) called from outside of physics.

With regards the example project provided, the reason the spring length adjustment is seen in the frame after the camera angle is changed, is because the spring length is being adjusted in the frame after the camera angle is changed. This is due to the way the scripts are constructed in the example project. The camera angles are adjusted in the _process() loop and the spring length is adjusted in the _physics_process() loop. However, the spring length is dependent on the the angles set in the _process() loop. Since the _physics_process() loop runs before each frame's _process() loop, the spring length is being adjusted before the angles. Therefore, the spring length is adjusted in the frame after the angles are set, regardless of whether or not the SpringArm::process_spring() runs in the same physics loop.

I've used the following single script on the Rotation_Helper_Horizontal node, which calculates the both the angle adjustment and the spring length in the _physics_process() loop. This results in both changes seen happening in the same frame:

extends Spatial
export var H_CAMERA_JOYSTICK_SENSITIVITY = 80
onready var vertical = $Rotation_Helper_Vertical
onready var spring_arm = $Rotation_Helper_Vertical/SpringArm
var camera_x = 0
func _ready():
	self.rotation_degrees.y = 185
func _physics_process(delta):
	vertical.rotate_x(deg2rad(camera_x * H_CAMERA_JOYSTICK_SENSITIVITY * delta))
	var camera_rot = vertical.rotation_degrees
	camera_rot.x = clamp(camera_rot.x, -85, 85)
	vertical.rotation_degrees = camera_rot
	camera_x = 0
func length_from_angle():
	var camera_direction_vector = spring_arm.get_global_transform().basis.z
	var angle = camera_direction_vector.angle_to(Vector3(0,-1,0))
	var length = pow(angle/2 + 1, 2) - 0.1
	return length
func _input(event):
	if Input.is_action_pressed("ui_up"):
		camera_x += 0.5
	if Input.is_action_pressed("ui_down"):
		camera_x -= 0.5

[akien-mga]

Closing as this change is incorrect, as described in the previous comment.