ramokz / phantom-camera

👻🎥 Control the movement and dynamically tween 2D & 3D cameras. Built for Godot 4. Inspired by Cinemachine.
https://phantom-camera.dev/
MIT License
1.97k stars 65 forks source link

Fixed jitter for none physics object #179

Closed P5ina closed 6 months ago

P5ina commented 6 months ago

Camera is not a physics body, it should be animated in _process. There is example of my fix:

Before

https://github.com/ramokz/phantom-camera/assets/44378225/2fd6f936-536f-4d7c-bcba-693adcb39da6

After

https://github.com/ramokz/phantom-camera/assets/44378225/f46ecd17-3b6d-4301-9788-e8c89bf0abda

The cube is animated through tweens, but they're working in _process. Script example:

extends MeshInstance3D

func _ready():
    var tween = create_tween().set_loops()
    tween.tween_property(self, "position", Vector3(4, 0, 0), 2.0).set_ease(Tween.EASE_IN_OUT).set_trans(Tween.TRANS_QUAD)
    tween.tween_property(self, "position", Vector3(-4, 0, 0), 2.0).set_ease(Tween.EASE_IN_OUT).set_trans(Tween.TRANS_QUAD)
Lasuch69 commented 6 months ago

Hello! I want to point out that this change makes huge difference, however spring arm is still causing a bit of jitter when camera is up against the wall.

I am more than happy to implement spring arm smoothing once this PR is merged.

P5ina commented 6 months ago

@Lasuch69 Thanks for pointing this out, I will investigate into this. I think I know how to fix this.

ramokz commented 6 months ago

This PR does seem to solve both your point of jittery animations happening in the _process, like your tween example, and the issue described in #173. Have been trying to find a way that meant you didn't need to use the smoothing-addon, but it might not be a feasible thing to automatically solve for this addon alone. Though there is still a jitter issue when you disable the damping and have a PhysicsBody a target. The obvious solution would be to use the visual representation (mesh/sprite) node as the Follow Target, but wonder if there's a way to still allow the PhysicsBody to be used as a target. Obviously, it's a fairly minor thing and won't lose any sleep over it.

This current solve does also mean that the requirement of the smoothing-addon needs to be communicated, especially to those just picking up the addon / Godot, as to not cause people to think something is fundamentally broken. Which is probably my biggest concern.

P5ina commented 6 months ago

@ramokz I think, we can make a switch to be able to use both _process and _physics_process. But I still think that using physics process for camera is a very bad idea and crutch but not a solution for physics objects.

P5ina commented 6 months ago

I think I know how to fix this. We can make a smoothing inside the camera script, like it's done in smoothing addon. I'll implement this.

Lasuch69 commented 6 months ago

I don't think that we should care about if target is smoothed or not, as it isn't really the responsibility of the camera. If you want to fix jitter you need to smooth the target anyway. I would suggest making smoothing just for the spring arm and animation in the _process and that's all.

We would end up with double smoothing, which might introduce "lag".

Edit: Another way it could be done is to make camera smoothing optional.

ramokz commented 6 months ago

Inclined to agree that it's fair to expect the developer to be responsible for keeping visual representation of nodes smoothed when they're nested within a physics based node, given it's a known issue and should have a better built-in solution eventually. From reading, Godot 3.5 and 3.6 have 3D and 2D support respectively, so is just pending a 4.x integration. So happy to leave the PR in its current state and merge it in.

The one thing I do want to avoid is the experience friction point when people update the addon to then suddenly start seeing jitter that wasn't present before. For some, the solution would require them to add either an additional add-on, or add a few lines of code themselves — not a big deal in of itself, but will likely be confusing for people who don't know. For that reason, I would prefer this change being merged as part of the 0.7 release, as that will introduce wider breaking changes. So that will provide an opportunity to walk through everything that has changed and how to make a scene give the expected results in the release notes. Equally, docs and example scenes will also need to be updated as part of that.

ramokz commented 6 months ago

Thanks for bringing up the discussion and submitting this PR :100: