Feature/Audio Filters #809

SplitPixl · 2021-09-15T23:11:27Z

This pull request...

Fixes a bug
Introduces a new feature
Improves an existing feature
Boosts code quality or performance

Description

Adds the filter command, with a handful of subcommands.

Purpose

Audio Funny

Relevant Issue(s)

N/A

SplitPixl · 2021-09-15T23:12:09Z

This is still a dirty implementation, i did a bad job of keeping Vctr's code seperate from just the filter commands. This will need some cleanup. I do not think this is ready to merge in the state it is in.

MichailiK · 2021-10-06T13:02:52Z

In before-hand: I'm sure just by scrolling through the diff you can easily spot things to fix, like the commented pom dependencies, the IntelliJ project file (which should be removed), the code that's commented out etc.

MichailiK

My apologies if I may sound aggressive, offensive or similar in this review. There are too many problems with this pull request. I have doubts that this feature will ever become a thing... not without this being rewritten, at the very least. This pull request seems to be using third party code too, which is something I'm not too fond of. Currently, there is no way to approve this. I'd recommend rewriting this with the master branch (and not in your fork) and not using other people's code. Regardless, here are some of the issues with this pull request:

MichailiK · 2021-10-07T19:35:31Z

.gitignore

@@ -2,6 +2,7 @@
 /target/
 /Playlists/
 /test/
+.idea


Do not modify the gitignore

Suggested change

.idea

.idea is to be excluded tho.

MichailiK · 2021-10-07T19:35:41Z

JMusicBot.iml

@@ -0,0 +1,88 @@
+<?xml version="1.0" encoding="UTF-8"?>


Remove this file

MichailiK · 2021-10-07T19:36:48Z

pom.xml

+        <dependency>
+            <groupId>com.github.natanbc</groupId>
+            <artifactId>lavadsp</artifactId>
+            <version>0.7.5</version>
+        </dependency>


I have not checked, but is this dependency needed?

MichailiK · 2021-10-07T19:37:14Z

pom.xml

+        <dependency>
+            <groupId>org.jetbrains.kotlin</groupId>
+            <artifactId>kotlin-stdlib-jdk8</artifactId>
+            <version>${kotlin.version}</version>
+        </dependency>


This project does not use Kotlin.

Suggested change

<dependency>

<groupId>org.jetbrains.kotlin</groupId>

<artifactId>kotlin-stdlib-jdk8</artifactId>

<version>${kotlin.version}</version>

</dependency>

MichailiK · 2021-10-07T19:38:11Z

pom.xml

-            <groupId>org.hamcrest</groupId>
-            <artifactId>hamcrest-core</artifactId>
-            <version>1.3</version>
+            <groupId>org.jetbrains.kotlin</groupId>
+            <artifactId>kotlin-test</artifactId>
+            <version>${kotlin.version}</version>


This project does not Kotlin. Revert this to the original state.

Suggested change

<groupId>org.hamcrest</groupId>

<artifactId>hamcrest-core</artifactId>

<version>1.3</version>

<groupId>org.jetbrains.kotlin</groupId>

<artifactId>kotlin-test</artifactId>

<version>${kotlin.version}</version>

<groupId>org.hamcrest</groupId>

<artifactId>hamcrest-core</artifactId>

<version>1.3</version>

MichailiK · 2021-10-07T20:25:40Z

src/main/java/com/jagrosh/jmusicbot/commands/dj/VolumeCmd.java

            else
            {
+                nvolume = Math.abs(nvolume);


This doesn't look like it will change anything.

Suggested change

nvolume = Math.abs(nvolume);

MichailiK · 2021-10-07T20:25:54Z

src/main/java/com/jagrosh/jmusicbot/commands/general/SettingsCmd.java

@@ -61,7 +61,8 @@ protected void execute(CommandEvent event)
                        + "\nCustom Prefix: " + (s.getPrefix() == null ? "None" : "`" + s.getPrefix() + "`")
                        + "\nRepeat Mode: **" + (s.getRepeatMode() ? "On" : "Off") + "**"
                        + "\nDefault Playlist: " + (s.getDefaultPlaylist() == null ? "None" : "**" + s.getDefaultPlaylist() + "**")
-                        )
+                        + "\nForbidden Audio: **" + (s.getForbiddenAudio() ? "On" : "Off") + "**"


Rename this setting.

MichailiK · 2021-10-07T20:26:20Z

src/main/java/com/jagrosh/jmusicbot/commands/general/SettingsCmd.java

@@ -61,7 +61,8 @@ protected void execute(CommandEvent event)
                        + "\nCustom Prefix: " + (s.getPrefix() == null ? "None" : "`" + s.getPrefix() + "`")
                        + "\nRepeat Mode: **" + (s.getRepeatMode() ? "On" : "Off") + "**"
                        + "\nDefault Playlist: " + (s.getDefaultPlaylist() == null ? "None" : "**" + s.getDefaultPlaylist() + "**")
-                        )
+                        + "\nForbidden Audio: **" + (s.getForbiddenAudio() ? "On" : "Off") + "**"


Make the text bold only if its turned on.

Suggested change

+ "\nForbidden Audio: **" + (s.getForbiddenAudio() ? "On" : "Off") + "**"

+ "\nForbidden Audio: " + (s.getForbiddenAudio() ? "**On**" : "Off")

MichailiK · 2021-10-07T20:27:58Z

src/main/java/com/jagrosh/jmusicbot/utils/ScalarFunctions.java

@@ -0,0 +1,5 @@
+package com.jagrosh.jmusicbot.utils;


Missing license header. This somewhat looks like someone else's code, though.

Suggested change

package com.jagrosh.jmusicbot.utils;

/*

* Copyright 2021 John Grosh <john.a.grosh@gmail.com>.

*

* Licensed under the Apache License, Version 2.0 (the "License");

* you may not use this file except in compliance with the License.

* You may obtain a copy of the License at

*

* http://www.apache.org/licenses/LICENSE-2.0

*

* Unless required by applicable law or agreed to in writing, software

* distributed under the License is distributed on an "AS IS" BASIS,

* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

* See the License for the specific language governing permissions and

* limitations under the License.

*/

package com.jagrosh.jmusicbot.utils;

MichailiK · 2021-10-07T20:28:20Z

src/main/java/com/jagrosh/jmusicbot/utils/VectorSupport.java

@@ -0,0 +1,115 @@
+package com.jagrosh.jmusicbot.utils;


Missing license header. This looks like someone else's code, though.

Suggested change

package com.jagrosh.jmusicbot.utils;

/*

* Copyright 2021 John Grosh <john.a.grosh@gmail.com>.

*

* Licensed under the Apache License, Version 2.0 (the "License");

* you may not use this file except in compliance with the License.

* You may obtain a copy of the License at

*

* http://www.apache.org/licenses/LICENSE-2.0

*

* Unless required by applicable law or agreed to in writing, software

* distributed under the License is distributed on an "AS IS" BASIS,

* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

* See the License for the specific language governing permissions and

* limitations under the License.

*/

package com.jagrosh.jmusicbot.utils;

MichailiK · 2021-10-07T20:36:55Z

pom.xml

+<!--        <dependency>-->
+<!--            <groupId>com.jagrosh</groupId>-->
+<!--            <artifactId>JagTag</artifactId>-->
+<!--            <version>0.5</version>-->
+<!--        </dependency>-->
+        <dependency>
+            <groupId>com.mashape.unirest</groupId>
+            <artifactId>unirest-java</artifactId>
+            <version>1.4.9</version>
+            <type>jar</type>
+        </dependency>
+        <dependency>
+            <groupId>club.minnced</groupId>
+            <artifactId>discord-webhooks</artifactId>
+            <version>0.3.0</version>
+        </dependency>
+<!--        <dependency>-->
+<!--            <groupId>de.dfki.mary</groupId>-->
+<!--            <artifactId>voice-cmu-slt-hsmm</artifactId>-->
+<!--            <version>5.2</version>-->
+<!--        </dependency>-->


Remove commented & unused dependencies.

Suggested change











<dependency>

<groupId>com.mashape.unirest</groupId>

<artifactId>unirest-java</artifactId>

<version>1.4.9</version>

<type>jar</type>

</dependency>

<dependency>

<groupId>club.minnced</groupId>

<artifactId>discord-webhooks</artifactId>

<version>0.3.0</version>

</dependency>

MichailiK · 2021-10-07T20:37:17Z

pom.xml

+<!--        <dependency>-->
+<!--            <groupId>org.hamcrest</groupId>-->
+<!--            <artifactId>hamcrest-core</artifactId>-->
+<!--            <version>1.3</version>-->
+<!--            <scope>test</scope>-->
+<!--        </dependency>-->


Remove commented dependency.

Suggested change

Sanduhr32

What is this mess?

Sanduhr32 · 2021-10-08T07:28:24Z

src/main/java/com/jagrosh/jmusicbot/audio/SplitPixlAudioPlayer.java

+    /**
+     * Destroy the player and stop playing track.
+     */
+    public void destroy() {


"Destructor" in java? U sure u doing this correctly?

Sanduhr32 · 2021-10-08T07:31:36Z

src/main/java/com/jagrosh/jmusicbot/audio/filter/BitCrushPcmAudioFilter.java

+            for (int i = 0; i < array.length; i++) {
+//                if (i % 2 == 0 && i % fac2 != 0)
+//                    array[i]=0;
+                array[i] = array[(i / fac2) * fac2];


mathematically this doesn't make any sense to me.
Why not i-1? or i++ next line?
Did you mean array[i] = array[i] / fac2 * fac2?

Sanduhr32 · 2021-10-08T07:35:05Z

src/main/java/com/jagrosh/jmusicbot/audio/filter/KaraokeConfig.kt

+
+    init {
+    }
+}


This is kotlin, which means splits pom changes are required. I don't see a reason why this code is in kotlin tho.

Sanduhr32 · 2021-10-08T07:35:29Z

src/main/java/com/jagrosh/jmusicbot/audio/filter/KaraokeConfig.kt

+        return String.format("Karaoke")
+    }
+
+    init {


Empty init is redundant.

Sanduhr32 · 2021-10-08T07:36:10Z

src/main/java/com/jagrosh/jmusicbot/audio/filter/LowPassConverter.java

@@ -0,0 +1,46 @@
+/*
+ * Copyright 2020 natanbc


I believe Natan is fine with this.

Sanduhr32 · 2021-10-08T07:40:27Z

src/main/java/com/jagrosh/jmusicbot/utils/ScalarFunctions.java

@@ -0,0 +1,5 @@
+package com.jagrosh.jmusicbot.utils;
+
+class ScalarFunctions implements VectorSupport.FilterFunctions {


Is this empty class needed? Afaik if you implement an interface you actually have to implement the methods of it.

Sanduhr32 · 2021-10-08T07:41:08Z

src/main/java/com/jagrosh/jmusicbot/utils/VectorSupport.java

@@ -0,0 +1,115 @@
+package com.jagrosh.jmusicbot.utils;


This code is written by natanbc from his github repo of lavaplayer filters.

Banaanae · 2022-10-09T04:58:21Z

another funny would be a max volume option in the settings
so we can go above 150

SplitPixl added 7 commits February 20, 2021 13:44

Probably a little bit messy

7b619d8

Merge branch 'master' into feature/filters

973f326

more

675beda

more!

2a183dc

more funny

75905c6

Clean up

6bf64d4

More clean up

9fd001a

Merge branch 'master' into feature/audio-filters

9ddff2d

SplitPixl marked this pull request as draft September 16, 2021 18:40

This was linked to issues Oct 6, 2021

Bass-boosting #968

Open

[Feature Request] change pitch and speed #475

Open

MichailiK self-assigned this Oct 6, 2021

MichailiK self-requested a review October 6, 2021 13:00

MichailiK requested changes Oct 7, 2021

View reviewed changes

Sanduhr32 reviewed Oct 8, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/Audio Filters #809

Feature/Audio Filters #809

SplitPixl commented Sep 15, 2021

SplitPixl commented Sep 15, 2021 •

edited

Loading

MichailiK commented Oct 6, 2021 •

edited

Loading

MichailiK left a comment

MichailiK Oct 7, 2021

Sanduhr32 Oct 8, 2021

MichailiK Oct 7, 2021

MichailiK Oct 7, 2021

MichailiK Oct 7, 2021

MichailiK Oct 7, 2021

MichailiK Oct 7, 2021

MichailiK Oct 7, 2021

MichailiK Oct 7, 2021

MichailiK Oct 7, 2021

MichailiK Oct 7, 2021

MichailiK Oct 7, 2021

MichailiK Oct 7, 2021

Sanduhr32 left a comment

Sanduhr32 Oct 8, 2021 •

edited

Loading

Sanduhr32 Oct 8, 2021

Sanduhr32 Oct 8, 2021

Sanduhr32 Oct 8, 2021

Sanduhr32 Oct 8, 2021

Sanduhr32 Oct 8, 2021

Sanduhr32 Oct 8, 2021

Banaanae commented Oct 9, 2022

	+ "\nForbidden Audio: " + (s.getForbiddenAudio() ? "On" : "Off") + ""
	+ "\nForbidden Audio: " + (s.getForbiddenAudio() ? "On" : "Off")

-package com.jagrosh.jmusicbot.utils;
+/*
+ * Copyright 2021 John Grosh <john.a.grosh@gmail.com>.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.jagrosh.jmusicbot.utils;

		@@ -0,0 +1,5 @@
		package com.jagrosh.jmusicbot.utils;

		class ScalarFunctions implements VectorSupport.FilterFunctions {

Feature/Audio Filters #809

Are you sure you want to change the base?

Feature/Audio Filters #809

Conversation

SplitPixl commented Sep 15, 2021

This pull request...

Description

Purpose

Relevant Issue(s)

SplitPixl commented Sep 15, 2021 • edited Loading

MichailiK commented Oct 6, 2021 • edited Loading

MichailiK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sanduhr32 left a comment

Choose a reason for hiding this comment

Sanduhr32 Oct 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Banaanae commented Oct 9, 2022

SplitPixl commented Sep 15, 2021 •

edited

Loading

MichailiK commented Oct 6, 2021 •

edited

Loading

Sanduhr32 Oct 8, 2021 •

edited

Loading