Add noise node by ilyamore88 · Pull Request #29 · codex-team/codex.music

ilyamore88 · 2019-11-21T16:06:20Z

Closes #26

neSpecc · 2019-11-21T16:09:38Z

src/modules/AudioContextManager.ts

+   * Get current time of audio context
+   * @return {Number}
+   */
+  public getCurrentTime(): number {


можно сделать геттер currentTime

neSpecc · 2019-11-21T16:11:10Z

src/modules/Noise.ts

+/**
+ * Class represents noise node
+ */
+export default class Noise {


Suggested change

export default class Noise {

export default abstract class Noise {

neSpecc · 2019-11-21T16:11:59Z

src/modules/Noise.ts

+   * @param destination {AudioDestinationNode} - audio context destination
+   */
+  public connect(destination: AudioDestinationNode): void {
+    if (this.isConfigured) {


зачем везде эти проверки?

neSpecc · 2019-11-21T16:12:19Z

src/modules/Noise.ts

+  /**
+   * Configure noise node
+   */
+  private configure(): void {


приваты идут ниже пабликов

neSpecc · 2019-11-21T16:12:39Z

src/modules/Noise.ts

+    this.buffer = audioContextManager.getAudioContext().createBuffer(1, 4096, audioContextManager.getAudioContext().sampleRate);
+    this.buffersChannelData = this.buffer.getChannelData(0);
+
+    for (let i = 0; i < 4096; i++) {


нужен коммент, или вынести в отдельный метод

…team/codex.music into noise-node � Conflicts: � dist/index.js

…to noise-node � Conflicts: � dist/index.js

neSpecc · 2019-11-26T17:02:20Z

src/modules/noises/Noise.ts

+  /**
+   * Noise node status
+   */
+  private isConfigured: boolean = false;


избыточное свойство

src/modules/noises/Noise.ts

neSpecc · 2019-11-26T17:03:41Z

src/modules/noises/Noise.ts

+    this.bufferSourceNode.loop = true;
+
+    /**
+     * Add bandpass filter


не написано, для чего

neSpecc · 2019-11-26T17:05:03Z

src/modules/noises/Noise.ts

+    if (frequency) {
+      this.setNoiseFrequency(frequency);
+    } else {
+      this.setNoiseFrequency(1000);


просто внеси 1000 как дефолтное значение параметра

neSpecc · 2019-11-26T21:22:59Z

src/modules/noises/Noise.ts

+  constructor(frequency?: number) {
+    this.configure();
+    if (frequency) {
+      this.currentFrequency = frequency;


дублирование кода

neSpecc · 2019-11-26T21:23:10Z

src/modules/noises/Noise.ts

+  private currentFrequency: number = 1000;
+
+  /**
+   * Destination for noise node


коммент не понятен

neSpecc · 2019-11-26T21:24:05Z

src/modules/noises/Noise.ts

+  /**
+   * Current frequency of noise node in hertz
+   */
+  private currentFrequency: number = 1000;


это свойство сейчас нигде не используется

neSpecc · 2019-11-26T21:26:36Z

src/modules/noises/Noise.ts

+  /**
+   * Configure noise node
+   */
+  private configure(): void {


может этот код лучше в конструктор перенести? тк тут определяются основные свойства, а приватный метод где-то внизу. Пусть лучше будет в конструкторе наверху

neSpecc · 2019-11-26T21:28:10Z

src/modules/noises/Noise.ts

+    if (newDestination) {
+      this.destination = newDestination;
+    }
+    if (this.destination) {


я не вижу в коде, где назначается this.destination

neSpecc · 2019-11-26T21:33:05Z

src/modules/noises/Noise.ts

+    /**
+     * Add bandpass filter for filtering required noise frequency
+     */
+    this.bandpass = new BandPassFilter();


это можно сразу при объявлении свойства назначить

ilyamore88 added 2 commits November 21, 2019 19:00

add noise node class

b08fb28

add method foe getting current time

65cb160

neSpecc suggested changes Nov 21, 2019

View reviewed changes

ilyamore88 added 12 commits November 21, 2019 19:26

getter for current time

7262692

move public methods up

9e761af

add docs for filling buffer

ff6cbb0

delete checks

7e0fea7

add getter for audio destination node

522517a

fix noise node

9217054

add demo

da8e786

fix bug with discontinuity

7d9d5fb

add new noises

b690ee0

Merge branches 'master' and 'noise-node' of https://github.com/codex-…

678f619

…team/codex.music into noise-node � Conflicts: � dist/index.js

Merge branch 'master' of https://github.com/codex-team/codex.music in…

b253e65

…to noise-node � Conflicts: � dist/index.js

refactor noise class

9e1485f

neSpecc suggested changes Nov 26, 2019

View reviewed changes

ilyamore88 added 2 commits November 26, 2019 23:37

add docs

af8084e

delete setter for noise destination and set it in connect method

0284e74

neSpecc reviewed Nov 26, 2019

View reviewed changes

simplify connect method

5e570b0

	export default class Noise {
	export default abstract class Noise {

Conversation

ilyamore88 commented Nov 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants