yomotsu / camera-controls

A camera control for three.js, similar to THREE.OrbitControls yet supports smooth transitions and more features.
https://yomotsu.github.io/camera-controls/
MIT License
1.89k stars 241 forks source link

Edit: 'out' the prop of 'getPosition' is now optional #506

Open kimgh06 opened 2 months ago

kimgh06 commented 2 months ago

Hello,

I just edited a little of codes of 'CameraControls.ts' file,

getPosition( out: _THREE.Vector3, receiveEndValue: boolean = true ): _THREE.Vector3 {...}

to

getPosition( out?: _THREE.Vector3, receiveEndValue: boolean = true ): _THREE.Vector3 {...}

The previous code returns 'out' and sets 'out' twice.

if I try to get the position on your purpose, I shall try this:

import { Vector3 } from 'three'
let camPosition: Vector3 = new Vector3();
camControlRef.current.getPosition(camPosition);

However, actually i try to get the position of a camera like this:

import { Vector3 } from 'three'
const camPosition: Vector3 = camControlRef.current.getPosition();

but this code works and return the position of a camera.

And It has a problem using 'pmndrs/drei' package depending on this package with TypeScript, It occur type error "Expected 1-2 arguments, but got 0".

So, I have an idea how about being it optional for using the function in one line.

I hope you read this as soon as possible and answer.

yomotsu commented 2 months ago

Thank you for bringing this to my attention. Your point is well understood. However, I would prefer to make out a required value, similar to the classes in three.js. For example, see the following code: https://github.com/mrdoob/three.js/blob/a2e9ee8204b67f9dca79f48cf620a34a05aa8126/src/math/Box3.js#L116-L126.

Therefore, we could consider removing the line below:

const _out = !! out && out.isVector3 ? out : new THREE.Vector3() as _THREE.Vector3;

(This line was primarily included for backward compatibility)

What do you think?

kimgh06 commented 2 months ago

I'm sorry for the delay. OK, I had a mistake to explain what happens.

There's a problem when using TypeScript. I use 'Vector3' of '@react-three/fiber' that only using as a type for 'react-three-fiber'. And, the 'Vector3' of '@react-three/fiber' is already using to send position of 'mesh', which can not accept 'Vector3' of 'three.js' as a type in TypeScript.

However, to use the function, I have to use 'Vector3' of 'three.js'. image

If I am on your purpose, I should make a 'Vector3' object in my code and put it to 'getPosition' function as 'out'. But I would like to code it with TypeScript.

So, They conflict in my code.

yomotsu commented 2 months ago

What do you think about using "rename-import" for your imports? This approach could help your avoid conflicts

import { Vector3 as ThreeVector3 } from 'three';

or type import

import { type Vector3 } from '@react-three/fiber';
kimgh06 commented 1 month ago

It's a good propose, but that's not what I am saying.

I just would like to use like this without a type error:

const camPosition: Vector3 = camControlRef.current.getPosition(); // <- But now it occurs a type error of "Expected 1-2 arguments, but got 0." that means "out" and "receiveEndValue" which is optional

1. image 2. image

So It's my propose that I want to make "out" to be opional.

yomotsu commented 1 month ago

I mean...

const _vec3 = new Vector3();
const camPosition = camControlRef.current.getPosition( _vec3 );

or

const camPosition = new Vector3();
camControlRef.current.getPosition( camPosition );

or

const camPosition = camControlRef.current.getPosition( new Vector3() );

or

let camPosition = new Vector3();
camPosition = camControlRef.current.getPosition( camPosition );

does the above work for you?