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

fix: camera controls ignored props #385

Merged

Conversation

whereiswolf
Copy link
Contributor

@whereiswolf whereiswolf commented Apr 18, 2024


Feel free to point out at any mistakes and discrepancies from conventions. I have a strong React background, and some aspects of Vue are kinda new to me 😜

Copy link

stackblitz bot commented Apr 18, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link
Member

@JaimeTorrealba JaimeTorrealba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey this work perfect, is very professional, thanks for this one.

But remember to update the docs for the component (adding the new props and maybe and example of the type that you can use, better be really explicit for the final user).

@whereiswolf
Copy link
Contributor Author

whereiswolf commented Apr 19, 2024

@JaimeTorrealba Thanks for the comment, I completely agree, let me work on that 😄
By the way, I was thinking about re-exporting the CameraControls.ACTION enum, so that the devs don't have to import it directly from camera-controls when they want to override mouseButtons or touches. Any thoughts on that?

@JaimeTorrealba
Copy link
Member

I agree with you, I would do it @whereiswolf (maybe you can export it in the defaultExpose ¿?)

@whereiswolf
Copy link
Contributor Author

Hmmm 🤔 Wouldn't it cause sort of a "circular" dependency? Also, I don't think it solves the case, because in order to type the ref we'd still need to import some types.

<script lang="ts" setup>
  ...
  const cameraControlsRef = shallowRef<{ value: ???, ACTION: ??? }>()
</script>

<template>
  <CameraControls
    ref="cameraControlsRef"
    :mouse-buttons="{ left: cameraControlsRef.ACTION.ZOOM }"
  />
</template>

It seems like it's a bit more problematic, we can't just export it as a type, because the ACTION enum would be unusable.

import { CameraControls } from 'camera-controls'
...
export type CameraControlsType = typeof CameraControls

It would actually make sense to re-export the whole CameraControls class from camera-controls, but we'd need to come up with a clever name, otherwise we run into naming collision 😅 This way it would be:

<script lang="ts" setup>
  import { CameraControls, CameraControlsSOMETHING } from '@tresjs/cientos'
  // CameraControlsSOMETHING is CameraControls from camera-controls
  ...
  const cameraControlsRef = shallowRef<{ value: CameraControlsSOMETHING }>()
</script>

<template>
  <CameraControls
    ref="cameraControlsRef"
    :mouse-buttons="{ left: CameraControlsSOMETHING.ACTION.ZOOM }"
  />
</template>

@whereiswolf
Copy link
Contributor Author

@JaimeTorrealba quick summary of the recent changes:

  • Updated CameraControls docs with "User input config" section.
  • Added logic to check the camera type, to set the default values in the same way as camera-controls does.
  • Added CameraControlsClass re-export of camera-controls (if you have a better name suggestion please lmk 😄 )

If anything's unclear or should be changed, I'm happy to work on that 😉

...touches,
})

export { default as CameraControlsClass } from 'camera-controls'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late I was off this weekend.

I love the result. The only thing I am not sure is the name "CameraControlsClass" could that create confusion for some users? (since ThreeJs is basically made in classes)

But I don't have a better name, maybe CameraControlsPointers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I wasn't sure about that either, what about ThreeCameraControls or BaseCameraControls? Maybe I miss something, but Pointers adds more ambiguity in my opinion 😛.

As one said:

There are only two hard things in Computer Science: cache invalidation and naming things.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like BaseCameraControls :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated 😉

@JaimeTorrealba
Copy link
Member

Hmmm 🤔 Wouldn't it cause sort of a "circular" dependency? Also, I don't think it solves the case, because in order to type the ref we'd still need to import some types.

<script lang="ts" setup>
  ...
  const cameraControlsRef = shallowRef<{ value: ???, ACTION: ??? }>()
</script>

<template>
  <CameraControls
    ref="cameraControlsRef"
    :mouse-buttons="{ left: cameraControlsRef.ACTION.ZOOM }"
  />
</template>

It seems like it's a bit more problematic, we can't just export it as a type, because the ACTION enum would be unusable.

import { CameraControls } from 'camera-controls'
...
export type CameraControlsType = typeof CameraControls

It would actually make sense to re-export the whole CameraControls class from camera-controls, but we'd need to come up with a clever name, otherwise we run into naming collision 😅 This way it would be:

<script lang="ts" setup>
  import { CameraControls, CameraControlsSOMETHING } from '@tresjs/cientos'
  // CameraControlsSOMETHING is CameraControls from camera-controls
  ...
  const cameraControlsRef = shallowRef<{ value: CameraControlsSOMETHING }>()
</script>

<template>
  <CameraControls
    ref="cameraControlsRef"
    :mouse-buttons="{ left: CameraControlsSOMETHING.ACTION.ZOOM }"
  />
</template>

100% agree :) my suggestion was terrible hahaha

Copy link
Member

@JaimeTorrealba JaimeTorrealba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work man!

@JaimeTorrealba JaimeTorrealba merged commit 47eb747 into Tresjs:main Apr 22, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

CameraControls component doesn't support mouseButtons and touches props
2 participants