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.

Sign up for GitHub

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

Jump to bottom

Add support for Virtual Audio Cable #2483

Merged
merged 1 commit into from
Jan 9, 2025
Merged

Add support for Virtual Audio Cable #2483

merged 1 commit into from
Jan 9, 2025
+8 −1

Conversation

DevanWolf
Copy link
Contributor

No description provided.

@DevanWolf DevanWolf mentioned this pull request Oct 30, 2024
Closed
@zmerp
Copy link
Member

zmerp commented Oct 30, 2024

Why did you change even the wiki? Do you think this software is superior to VB Cable?

@The-personified-devil
Copy link
Collaborator

Actual implementation is missing, see the rust errors

The-personified-devil
The-personified-devil requested changes Oct 31, 2024
View reviewed changes
wiki/Installation-guide.md Outdated Show resolved Hide resolved
@The-personified-devil
Copy link
Collaborator

The code lgtm, but is there anyone else that can test the pr?

@zmerp
Copy link
Member

zmerp commented Nov 4, 2024 •
edited
Loading

Before merging, @DevanWolf still needs to explain why Virtual Audio Cable is being replaced as the preferred solution rather than VB Cable

The-personified-devil
The-personified-devil requested changes Nov 5, 2024
View reviewed changes
alvr/audio/src/lib.rs Outdated Show resolved Hide resolved
@The-personified-devil
Copy link
Collaborator

Before merging, @DevanWolf still needs to explain why Virtual Audio Cable is being replaced as the preferred solution rather than VB Cable

It isn't really, it's just put first in the popup, but eh

@zmerp
Copy link
Member

zmerp commented Nov 5, 2024 •
edited
Loading

It isn't really, it's just put first in the popup, but eh

I meant also in the setup wizard

@The-personified-devil
Copy link
Collaborator

I meant also in the setup wizard

That's what I meant

@zmerp
Copy link
Member

zmerp commented Nov 11, 2024

@DevanWolf ping

The-personified-devil
The-personified-devil previously requested changes Dec 7, 2024
View reviewed changes
wiki/Installation-guide.md Outdated Show resolved Hide resolved
@zmerp
Copy link
Member

zmerp commented Dec 17, 2024 •
edited
Loading

@DevanWolf You haven't actually changed the order of options in the settings. I would like please if you can tell me your experience with Virtual Audio Cable vs VBCable, if you think one is superior than the other and why.

@The-personified-devil The-personified-devil dismissed their stale review December 19, 2024 12:23

I have no clue about this at this point, that's windows stuff

@lucas11222
Copy link

@DevanWolf You haven't actually changed the order of options in the settings. I would like please if you can tell me your experience with Virtual Audio Cable vs VBCable, if you think one is superior than the other and why.

ive seen the issue and it says: "The creator knows VB-Audio (that I don't recommend) but forgot that Virtual Audio Cable (Lite) by Muzychenko exists.
The IN/OUT audio device are named Virtual Cable 1, whereas VB-CABLE is named "CABLE Input" and "CABLE Output"." from #2123

@zmerp
Copy link
Member

zmerp commented Jan 5, 2025

@lucas11222 ok cool. So do you have direct experience with that, about stability/latency etc?

@lucas11222
Copy link

@lucas11222 ok cool. So do you have direct experience with that, about stability/latency etc?

nop, i just put that he say in the issue, idk why he didint put it in the pull request.

DevanWolf
DevanWolf commented Jan 6, 2025
View reviewed changes
Copy link
Contributor Author

@DevanWolf DevanWolf left a comment •
edited
Loading

Choose a reason for hiding this comment

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

All corrected and checked.

@zmerp
Copy link
Member

zmerp commented Jan 6, 2025

@The-personified-devil let's just merge?

@The-personified-devil
Copy link
Collaborator

@zmerp I mean if it doesn't do any preferring you don't want in the selection logic (if if even does any automatic detection) (I don't fully understand the windows code but fwiw it looks fine). So a fine by me ig.

@zmerp
Copy link
Member

zmerp commented Jan 6, 2025

Ah right, since VAC is still the first in the list it will be picked first

zmerp
zmerp requested changes Jan 6, 2025
View reviewed changes
alvr/audio/src/lib.rs Outdated Show resolved Hide resolved
alvr/audio/src/lib.rs Outdated Show resolved Hide resolved
@The-personified-devil
Copy link
Collaborator

@zmerp I couldn't bear this any longer

@zmerp zmerp merged commit aafcb2a into alvr-org:master Jan 9, 2025
8 checks passed
@lucas11222
Copy link

this pr take a LOT OF TIME for merging

@zmerp
Copy link
Member

zmerp commented Jan 9, 2025

@lucas11222 Because the original author wasn't cooperating

@lucas11222
Copy link

@lucas11222 Because the original author wasn't cooperating

i know but a year... thats a lot

@The-personified-devil
Copy link
Collaborator

i know but a year... thats a lot

It's been less than 4 months

@zmerp
Copy link
Member

zmerp commented Jan 10, 2025 •
edited
Loading

i know but a year... thats a lot

Look at other projects, for example the Rust compiler. PRs might take multiple years to review. And to be honest since the author wasn't cooperating much we could have closed the PR as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucas11222 lucas11222 lucas11222 left review comments

@The-personified-devil The-personified-devil The-personified-devil left review comments

@zmerp zmerp zmerp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DevanWolf @zmerp @The-personified-devil @lucas11222