search

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.93k stars 62 forks source link

Phantom camera's global_position getting incorrectly(?) floored when it's limit is set #268

Closed audeck closed 2 months ago

audeck commented 2 months ago

Issue description

I've been playing around with 2D phantom cameras recently (great initiative, btw!) and ran into weird behavior. Using lawnjelly/smoothing-addon, simple cameras with no limit work as expected.

However, once I set a limit target (and, quickly looking through the code, I imagine the same thing happens when limit sides are set), the camera's global_position kept getting rounded down to integer values, causing visual jitter while following a node with a position that wasn't rounded to integers.

After dissecting phantom_camera_2d.gd, it seems the _set_limit_clamp_position function, called from _process through _interpolate_position on every frame here returns a Vector2i, resulting in the aforementioned rounding down. After changing the return type to Vector2, everything works as expected.

As someone with little to no experience using this plugin, I don't whether:

  1. I'm doing something wrong and the _set_limit_clamp_function is getting called incorrectly every frame (and otherwise works as expected), or
  2. the return type is indeed wrong.

That is mainly why I'm creating an issue, rather than submitting a PR (the single-commit PR would also change literally one (1) character). Thank you.

Steps to reproduce

  1. Create a node that is able to move at non-integer increments (a player character, etc.)
  2. Setup a basic Camera2D (no smoothing, etc.) w/ a PhantomCameraHost
  3. Add a PhantomCamera2D following said node on simple mode
  4. Set any limits to said PhantomCamera2D

(Optional) Minimal reproduction project

No response

ramokz commented 2 months ago

Can't quite replicate it, however I think your rationale and finding makes sense.

While the Limit sides have to be integers, that doesn't mean the positioning of the target_position (on line 563) needs to be too, as it will still be clamped to the side definitions anyway.

In fact, the target position is currently always an integer when a limit target is applied, which is definitely not right. _set_limit_clamp_position() is also casting a regular Vector2 to a Vector2i, which is kinda silly too.

Some quick tests don't show any issues with what you suggest. If it solves your problem, then I am happy to get that change in.

Feel free to make the PR and get the contribution credit; otherwise, I'm happy to make that quick change and cite this issue.

audeck commented 2 months ago

Submitted #269. Thanks!