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

feat: 246 positional audio #365

Conversation

damienmontastier
Copy link
Contributor

@damienmontastier damienmontastier commented Mar 14, 2024

Issue #246 (Positional Audio)

Local Playgroundpnpm run playground

Local Documentationpnpm run docs:dev


@alvarosabu, @JaimeTorrealba

This is my first PR on Cientos and I wanted to get your feedback so that I can improve the code/operation as much as possible in line with user expectations. 🤙


Description

Component based on the Three.js PositionalAudio class. It includes a helper that makes it easier to visualise the sound emission. This helper is based on PositionalAudioHelper class.

Feedbacks

  • Unfortunately @tresjs/leches is not available without passing through <TresLeches> in the template, which greatly limits its use for example with a helper during development. In the case of this Positional Audio composant, it would be cool if you could set parameters directly via the GUI. Maybe create a single instance of <TresLeches> and then add elements inside it? Maybe there's a way out of this, if you need help with that or want to contribute to @tresjs/leches.

Copy link

stackblitz bot commented Mar 14, 2024

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

@damienmontastier damienmontastier mentioned this pull request Mar 14, 2024
4 tasks
Copy link
Contributor Author

@damienmontastier damienmontastier left a comment

Choose a reason for hiding this comment

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

I've added this part to counter a problem with the three.js package. I'm going to open an issue, but in the meantime I've fixed it with this part of code 🤙

@JaimeTorrealba
Copy link
Member

JaimeTorrealba commented Mar 15, 2024

Hi @damienmontastier no problem, I wouldn't use extend for this one, it is more an abstraction. Similar to

  • Is still defineExpose not working for you? Please let me know (I check your code, and it seems ok) it should work we have all our component working with it

  • Leches is under heavy development, and we the core team agree to re-think it. But as in every pkg any help is highly appreciated

  • When I saved it, my code gets automatically linter. No idea what config do you have to do sorry (we will ask Alvaro later)

  • I've never run at the same time :). But could handle to do it

Is this component ready to review I've seen many new commits coming into???

@andretchen0
Copy link
Contributor

@damienmontastier

About pnpm lint: is there an efficient way to ensure that the lint modifies the code each time it is saved ? (on VSCode)

Fwiw, for the time being, you could set up your own git hook to run the linter. (At least for me, pnpm run lint --fix is relatively slow and I wouldn't want it to run every time I saved.)

Coincidentally, I just opened a similar issue on Tres for this. It's a proposal to run the linter before git pushes files to Github and halt pushes that include linter errors. (With an opt-out.)

@damienmontastier
Copy link
Contributor Author

damienmontastier commented Mar 15, 2024

Hey @JaimeTorrealba !

  • Strange, I tried again and defineExpose works... I'm going to adapt my code!

No, it's not ready for review yet, I need to add an example and correct my lint errors.

———

@andretchen0 !

It could indeed be interesting! When I see 100 lint errors to correct by hand, I get scared haha... after all, we're all a bit lazy! 😆

——

I also noticed another potential problem: when I use my core component <PositionalAudio> in the doc, the onUnmounted is never called in this file, so I had to add a dispose() function to the defineExpose() and call it in the parent... Do you have any solutions?

@JaimeTorrealba
Copy link
Member

JaimeTorrealba commented Mar 15, 2024

@damienmontastier. Please let me know when this is ready :)

  • Regarding the dipose() that could be another issue. Many components use the onUnmouted to dispose their instance, I'll check it later
  • You could add a close word in the description of this PR to link it to the issue more info
  • Also, you could put this PR in "draft"mode so we know when is ready (optional)

😄

@damienmontastier damienmontastier marked this pull request as draft March 15, 2024 17:03
@andretchen0
Copy link
Contributor

andretchen0 commented Mar 15, 2024

When I see 100 lint errors to correct by hand, I get scared haha

Maybe you know, but just in case it's helpful:

The linter can fix many common errors if you do pnpm run lint --fix

Just close or save any open, edited files in the editor before you run the linter. Otherwise, the editor will throw 'overwrite' warnings when you eventually save those files.

@damienmontastier
Copy link
Contributor Author

When I see 100 lint errors to correct by hand, I get scared haha

Maybe you know, but just in case it's helpful:

The linter can fix many common errors if you do pnpm run lint --fix

Just close or save any open, edited files in the editor before you run the linter. Otherwise, the editor will throw 'overwrite' warnings when you eventually save those files.

Finally, I found a way to lint on code save with the VSCode extension ESLint 🤙 Thanks 👌

@damienmontastier damienmontastier marked this pull request as ready for review March 19, 2024 07:45
@damienmontastier
Copy link
Contributor Author

Hey! PR over! I've just got a problem and I can't seem to solve it, so if you've got any ideas... On the component documentation there are two demos and there seems to be a sound problem, the sounds seem to crackle... When you comment on one of the two demos everything works perfectly... I can't figure out why...

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 @damienmontastier. I just drop some little reviews, nothing too big.

Sorry for asking you to move the assets, but in cientos we should not have assets (except images). I'll update the contribution guide adding this definition.

docs/.vitepress/theme/components/PositionalAudioDemo.vue Outdated Show resolved Hide resolved
docs/guide/abstractions/positional-audio.md Show resolved Hide resolved
docs/guide/abstractions/positional-audio.md Outdated Show resolved Hide resolved
docs/guide/abstractions/positional-audio.md Outdated Show resolved Hide resolved
docs/guide/abstractions/positional-audio.md Outdated Show resolved Hide resolved
docs/public/positional-audio/sound1.mp3 Outdated Show resolved Hide resolved
playground/public/positional-audio/sound.mp3 Outdated Show resolved Hide resolved
src/core/abstractions/PositionalAudio.vue Outdated Show resolved Hide resolved
docs/guide/abstractions/positional-audio.md Show resolved Hide resolved
src/core/abstractions/PositionalAudio.vue Show resolved Hide resolved
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 @damienmontastier. I just drop some little reviews, nothing too big.

Sorry for asking you to move the assets, but in cientos we should not have assets (except images). I'll update the contribution guide adding this definition.

And big thanks for this.

docs/guide/abstractions/positional-audio.md Outdated Show resolved Hide resolved
@alvarosabu alvarosabu added p3-significant High-priority enhancement (priority) feature New feature or request labels Apr 2, 2024
@damienmontastier
Copy link
Contributor Author

Hey @alvarosabu @JaimeTorrealba ! I've changed quite a few things, it looks like it's a good version, let me know! ✌️

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 job!!!

@JaimeTorrealba JaimeTorrealba merged commit 611aaaf into Tresjs:main Apr 30, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request p3-significant High-priority enhancement (priority)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants