Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

kimgh06
Copy link

@kimgh06 kimgh06 commented May 2, 2024

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
Copy link
Owner

yomotsu commented May 2, 2024

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
Copy link
Author

kimgh06 commented May 2, 2024

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
Copy link
Owner

yomotsu commented May 3, 2024

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
Copy link
Author

kimgh06 commented May 4, 2024

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

image
2.
image

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

@yomotsu
Copy link
Owner

yomotsu commented May 4, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants