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

WebGL #312

Merged
merged 31 commits into from
Aug 1, 2024
Merged

WebGL #312

merged 31 commits into from
Aug 1, 2024

Conversation

float3
Copy link
Collaborator

@float3 float3 commented May 13, 2024

No description provided.

@float3 float3 requested a review from pema99 May 13, 2024 15:16
@float3 float3 marked this pull request as ready for review May 13, 2024 15:16
@float3
Copy link
Collaborator Author

float3 commented May 15, 2024

@pema99 I only really need you to look at these changes
image
but you're welcome to look at the rest as well

@@ -210,6 +214,25 @@ public partial class AudioLink : MonoBehaviour
private int _Samples3R;
// ReSharper restore InconsistentNaming

#if UNITY_WEBGL
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file is already pretty crowded. Consider moving the WebGL specific stuff to a new file, like we do for the DataAPI already - AudioLink is a partial class

Copy link
Collaborator Author

@float3 float3 Jul 25, 2024

Choose a reason for hiding this comment

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

do we have to? is this blocking?

Copy link
Collaborator

Choose a reason for hiding this comment

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

not blocking, but i think it would be nicer. Consider for a follow up

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure

return _waveformSamplesRight;
}

public void SyncLeft(Action<float[]> FetchSamplesLeft)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not just regular setters?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

idk I think fundale wrote that

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe change it? looks like a simple change to make

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@fundale any reason you wrote it like this?

Copy link
Contributor

Choose a reason for hiding this comment

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

@fundale any reason you wrote it like this?

Not particularly,
A setter might work, I just did it the same way as this shim:
https://github.com/Lakistein/Unity-Audio-Visualizers

Copy link
Collaborator Author

@float3 float3 Jul 29, 2024

Choose a reason for hiding this comment

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

hmmm i tried today, my attempt didn't work ce64511, i'm confused about how the this syntax works

Copy link
Collaborator

@pema99 pema99 Jul 29, 2024

Choose a reason for hiding this comment

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

if (audioSource.isPlaying)
{
    audioLinkWebPeer.WaveformSamplesLeft = FetchAnalyzerLeft(WebALID, new float[4096], 4096);
    audioLinkWebPeer.WaveformSamplesRight = FetchAnalyzerRight(WebALID, new float[4096], 4096);
}

Should probably be

if (audioSource.isPlaying)
{
    FetchAnalyzerLeft(WebALID, audioLinkWebPeer.WaveformSamplesLeft, 4096);
    FetchAnalyzerRight(WebALID, audioLinkWebPeer.WaveformSamplesRight, 4096);
}

@pema99
Copy link
Collaborator

pema99 commented Jun 18, 2024

@float3 left some comments. Sorry it took so long

@pema99
Copy link
Collaborator

pema99 commented Jul 24, 2024

Still wanna land this? There are a bunch of open comments

@float3
Copy link
Collaborator Author

float3 commented Jul 25, 2024

@pema99 this branch is kinda horrible

@float3
Copy link
Collaborator Author

float3 commented Jul 25, 2024

should we just land this and do some clean up on dev, I lowkey don't wanna redo all these changes for a new PR, especially since I want fundale to have attribution

@float3
Copy link
Collaborator Author

float3 commented Jul 25, 2024

but I kinda fucked it up

@float3 float3 merged commit 1ea982e into master Aug 1, 2024
1 check passed
@float3 float3 deleted the web branch August 1, 2024 16:36
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.

3 participants