The-DevBlog / bevy_third_person_camera

A third person camera crate written for Bevy.
Apache License 2.0
75 stars 17 forks source link

`ThirdPersonCamera` field `focus` has no use #14

Closed MickHarrigan closed 6 months ago

MickHarrigan commented 10 months ago

I have been looking through and modifying this code a bit for an upcoming PR I plan on opening and in that looking I found that the focus field of ThirdPersonCamera has no bearing on the plugin.

It is never written to except on creation, and when that happens the value is either ignored or doesn't really affect anything. This is most noteworthy when moving around and the focus doesn't move with the character.

Not sure what the plan would be to do from here. Just thought I would bring light to it.

The-DevBlog commented 7 months ago

@MickHarrigan Hey Mick! So sorry, I have not been monitoring the issues on this repo. Is this still something you are looking at? I'd be glad to help.

The-DevBlog commented 7 months ago

@MickHarrigan I appreciate you brining this up though. TBH I used chat GPT to write some of that and it used that focus field that you are mentioning. I'll go ahead and look into it and keep this issue open while I do.

MickHarrigan commented 7 months ago

Hey sorry it took me so long to respond, this just kept slipping out of my head.

But what I was referencing was that the focus field was never actually written to and was used in many locations with it just being Vec3::Zero. I only found this as I was doing some changes to the library to see if changing the focus would do anything which is why I noticed that it wasn't actually affecting it.

There were some other things such as adding the focus in one line, then subtracting it again later:

// lib.rs

fn sync_player_camera(
    player_q: Query<&Transform, With<ThirdPersonCameraTarget>>,
    mut cam_q: Query<(&mut ThirdPersonCamera, &mut Transform), Without<ThirdPersonCameraTarget>>,
) {
    // ... excluded for this

    // Adding in cam.focus
    let desired_translation =
        cam.focus + rotation_matrix.mul_vec3(Vec3::new(0.0, 0.0, cam.zoom.radius)) + offset;

    // cam.focus is subtracted here, with no real need
    // Update the camera translation and focus
    let delta = player.translation - cam.focus;
    cam_transform.translation = desired_translation + delta;
}

Overall just some oversights that I was bringing up at the time and have since been distracted with other things.

The-DevBlog commented 7 months ago

Ah okay. Thanks for the catch. Also, feel free to remove it and open a PR and I can approve!

The-DevBlog commented 6 months ago

Just released a new version and added this fix. Thanks again!