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

Drag and drop audio effect resources to audio bus #62802

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Drag and drop audio effect resources to audio bus #62802

wants to merge 1 commit into from
+81 −31

Conversation

Nolkaloid
Copy link
Contributor

@Nolkaloid Nolkaloid commented Jul 7, 2022 •
edited by akien-mga

Finished #42288

  • Effect resources without a name are displayed as <filename>
  • Name of effect resources is now updated instantly in the bus editor

Bugsquad edit:

@akien-mga akien-mga added this to the 4.0 milestone Jul 7, 2022
@akien-mga akien-mga mentioned this pull request Jul 8, 2022
Closed
@akien-mga akien-mga requested a review from a team July 8, 2022 07:51
reduz
reduz previously requested changes Aug 8, 2022
View reviewed changes
editor/editor_audio_buses.cpp Outdated Show resolved Hide resolved
@Nolkaloid Nolkaloid force-pushed the audio_effect-dnd branch 2 times, most recently from 008406e to fc4038e Compare September 22, 2022 04:10
@YuriSizov YuriSizov modified the milestones: 4.0, 4.1 Feb 10, 2023
@YuriSizov YuriSizov requested a review from a team February 10, 2023 19:06
@YuriSizov YuriSizov modified the milestones: 4.1, 4.2 Jun 14, 2023
Calinou
Calinou requested changes Aug 8, 2023
View reviewed changes
Copy link
Member

@Calinou Calinou left a comment •
edited

Choose a reason for hiding this comment

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

Tested locally (rebased on top of eca6f0e), it mostly works as expected. However, the usability issue outlined by KoBeWi above is still present. It needs fixing before this can be merged.

@Nolkaloid Are you available to perform the required changes?

@Nolkaloid
Copy link
Contributor Author

I'll try to looking into it, as soon as I find some free time !

@Nolkaloid Nolkaloid requested a review from a team as a code owner August 19, 2023 21:15
@Nolkaloid
Copy link
Contributor Author

@Calinou I found some time this evening to make the change. Should be OK.

@Nolkaloid Nolkaloid force-pushed the audio_effect-dnd branch 3 times, most recently from bf36477 to 4f0525f Compare August 19, 2023 21:32
@KoBeWi
Copy link
Member

KoBeWi commented Aug 23, 2023 •
edited

Below Add Effect is still a valid location and the effect lands above.

godot.windows.editor.dev.x86_64_hFYsonBYjI.mp4

It would be nice to fix it if it can be fixed easily. If not, it's fine.

EDIT:
The existing drag and drop also has this problem, so it's not important.

@Nolkaloid
Copy link
Contributor Author

@KoBeWi To fix that, the best option would be to move the "Add effect" entry outside the Tree.
I think that could be done in another PR.

@AThousandShips AThousandShips modified the milestones: 4.2, 4.3 Oct 26, 2023
@fire fire requested a review from a team January 23, 2024 19:16
Calinou
Calinou reviewed Apr 11, 2024
View reviewed changes
Copy link
Member

@Calinou Calinou left a comment •
edited

Choose a reason for hiding this comment

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

Tested locally, the UX works now, but I have issues with drag-and-dropped effects having no effect on the actual audio playback.

This does not occur when using a built-in resource created in the audio bus editor with the exact same parameters (both are 10-band equalizers with -20 dB on all bands).

Testing project: spectrum.zip

EQ10 is the built-in effect.

No effect on audio:

image

Has an effect on audio:

image

The ordering doesn't matter.

@Nolkaloid
Drag and drop audio effect resources to audio bus
e51a7c4 
Co-authored-with: Waranoi <31479779+Waranoi@users.noreply.github.com>

Fix UndoRedoManager singleton change

Made dropping under effects no-op
@Nolkaloid
Copy link
Contributor Author

Nolkaloid commented May 25, 2024 •
edited

@Calinou The effect seems to be properly applied. Note that in your example project the effect "new_audio_effect_eq_10.tres" has all the bands set to 0dB, so it has logically no effect what so ever. Or am I missing the point ?

@Nolkaloid Nolkaloid requested a review from Calinou June 3, 2024 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akien-mga akien-mga akien-mga left review comments

@KoBeWi KoBeWi KoBeWi approved these changes

@reduz reduz reduz left review comments

@Calinou Calinou Awaiting requested review from Calinou

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
enhancement topic:audio topic:editor usability
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.

Allow drag and dropping audio effects on an audio bus
7 participants
@Nolkaloid @KoBeWi @Calinou @akien-mga @reduz @YuriSizov @AThousandShips