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

Create AudioStreamPlayer when dropping AudioStream #92004

Merged
merged 1 commit into from
May 21, 2024

Conversation

timothyqiu
Copy link
Member

Closes godotengine/godot-proposals#9699

  • Create AudioStreamPlayer if dropped in between nodes in the Scene dock
  • Create AudioStreamPlayer2D if dropped into 2D editor
  • Create AudioStreamPlayer3D if dropped into 3D editor
Peek.2024-05-16.13-21.mp4

@timothyqiu timothyqiu added this to the 4.x milestone May 16, 2024
@timothyqiu timothyqiu requested review from a team as code owners May 16, 2024 05:25
@passivestar
Copy link
Contributor

Hey, thanks for implementing this

I have two questions:

  1. Is there a reason not to check if the parent node is derived from Node2D/Node3D when dropping into the scene dock to instantiate an appropriate player like I mentioned in the proposal? If you're making a prefab you may want to be able to add an AudioStreamPlayer3D to it at (0,0,0) position (say you're prototyping an enemy and you want to quickly add a positional sound effect to it for when it dies). With the current approach you'll have to reset the position manually in the inspector after you drop

I can also think of some potential cases where you won't have the viewport available at all, only docks

  1. Would it be hard to cast a ray when dragging it into the 3d editor to be able to place it on colliders? That's probably how people would expect this to work because that's how it works for scenes. It'll make it much more useful for quick level prototyping

@timothyqiu
Copy link
Member Author

timothyqiu commented May 16, 2024

Is there a reason not to check if the parent node is derived from Node2D/Node3D when dropping into the scene dock to instantiate an appropriate player like I mentioned in the proposal?

Oh, using the parent node as a basis is a good idea. But for many 2D games, it's more likely to use plain AudioStreamPlayer instead of AudioStreamPlayer2D. There should be a way to toggle the behavior.

godotengine/godot-proposals#9699 is about dropping onto a node. That action is taken by property setter.

Would it be hard to cast a ray when dragging it into the 3d editor to be able to place it on colliders?

It already is.

I can also think of some potential cases where you won't have the viewport available at all, only docks

AFAIK, the debugger does not support assigning resource properties at runtime. i.e., you can create the audio stream player node, but its stream property won't be synced. That's a current limitation.

@passivestar
Copy link
Contributor

There should be a way to toggle the behavior

A modifier key perhaps?

AFAIK, the debugger does not support assigning resource properties at runtime. i.e., you can create the audio stream player node, but its stream property won't be synced. That's a current limitation

I believe it does support that, but I'll open a proposal in the future if that's not the case. I've been doing some R&D related to hot-reloading at runtime (with a GDScript plugin) and it doesn't seem hard to achieve those things anyway

It already is.

My bad, I see now that I made a mistake while testing

@timothyqiu
Copy link
Member Author

timothyqiu commented May 16, 2024

  • Made the 3D viewport preview not fixed size, so it's clear how it interacts with the environment.
  • For the Scene dock, holding Shift while dropping creates AudioStreamPlayer2D for Node2D parent and creates AudioStreamPlayer3D for Node3D parent.
    • Note that if a node does not have any child, you can't drop the new node as its child. That's a limitation of Tree (it only supports dropping above node / below node / on node, no horizontal detection). When moving nodes inside the tree, dropping node A onto node B makes A the child of B. But for files, this action is already taken. Currently broken :P
Peek.2024-05-16.16-35.mp4

@passivestar
Copy link
Contributor

Note that if a node does not have any child, you can't drop the new node as its child.

But I can though, wdym?

Screen.Recording.mp4

@timothyqiu
Copy link
Member Author

Looks like I accidentally broken the "drop to set property" feature 😂

@passivestar
Copy link
Contributor

passivestar commented May 16, 2024

Looks like I accidentally broken the "drop to set property" feature 😂

Can you leave it "broken" when another modifier key is held? It's kinda super convenient :D

Or perhaps do what we discussed in the proposal and check if the node has a "stream" property to assign to?

@KoBeWi
Copy link
Member

KoBeWi commented May 16, 2024

Or perhaps do what we discussed in the proposal and check if the node has a "stream" property to assign to?

Valid properties are only checked at the time of drop, to avoid performance problems when hovering.

@passivestar
Copy link
Contributor

Valid properties are only checked at the time of drop, to avoid performance problems when hovering.

Doesn't it work for this PR? There's no preview in the scene dock or anything like that

@timothyqiu
Copy link
Member Author

I cleaned up the drop code a bit.

Now scene tree dock always tries to do a property drop first. If no property can be set, then the file is dropped as a new node.

This has a side-effect: Say a node has a PackedScene property and you're dropping a scene file onto that node. It instantiates the scene as child before, and now it sets the property.

I think this change is okay because:

  • It makes dropping behavior more consistent.
  • No special case handling is involved.
  • This situation is relatively rare.

If this concerns, maybe we can add an editor setting like docks/scene_tree/prefer_drop_as_property to swap the priority.

@passivestar
Copy link
Contributor

I found a bug. When dropping a scene right under a node that has a PackedScene property it would assign it to the property. I believe it's an issue with the Tree, it doesn't correctly show what's gonna happen when you drop. A couple of pixels higher/lower makes a difference regardless of what visual indication it shows

Screen.Recording.mp4

@timothyqiu
Copy link
Member Author

When dropping a scene right under a node that has a PackedScene property it would assign it to the property.

Fixed. Turns out to be my mistake, sorry :P

@KoBeWi
Copy link
Member

KoBeWi commented May 17, 2024

Audio dropping is missing header.
image
vs
image

Comment on lines +3308 to +3311
}
if (!valid_properties.is_empty()) {
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove else if?

Copy link
Member Author

Choose a reason for hiding this comment

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

These two branches both returns now. So the else is redundant.

editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
@timothyqiu
Copy link
Member Author

Audio dropping is missing header.

Currently, the header is shown if the file list contains texture. I added the same for audio streams in the recent update. e.g.:

  • Adding Sprite2D
  • Adding Sprite2D and AudioStreamPlayer2D

Note that scene-dropping does not have this header either. I'm out of ideas about how to describe the combinations 😛 Probably worth another PR.

@Nodragem
Copy link
Contributor

Audio dropping is missing header.

Hello, I open a proposal on making the dropping tooltips consistent between object types and 2D/3D Viewport a few days ago: godotengine/godot-proposals#9752

I am going to start working on it this weekend and open a PR. So maybe we could add the Audio dropping tooltip to that PR rather than to this one? to avoid merge conflict?

editor/scene_tree_dock.h Outdated Show resolved Hide resolved
editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
editor/plugins/node_3d_editor_plugin.h Outdated Show resolved Hide resolved
editor/plugins/node_3d_editor_plugin.cpp Outdated Show resolved Hide resolved
editor/plugins/canvas_item_editor_plugin.h Outdated Show resolved Hide resolved
editor/plugins/canvas_item_editor_plugin.cpp Outdated Show resolved Hide resolved
- Create AudioStreamPlayer if dropped in between nodes in the Scene dock
- Create AudioStreamPlayer2D if dropped into 2D editor
- Create AudioStreamPlayer3D if dropped into 3D editor
@akien-mga akien-mga modified the milestones: 4.x, 4.3 May 21, 2024
@akien-mga akien-mga merged commit ee1f898 into godotengine:master May 21, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@timothyqiu timothyqiu deleted the drop-audio branch May 21, 2024 09:27
@patwork
Copy link
Contributor

patwork commented May 28, 2024

@timothyqiu hello, after this commit I'm getting Error loading audio stream from icon.svg. when I'm dragging default godot icon.svg into scene tree...

err

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create an AudioStreamPlayer when an AudioStream is dropped into the scene
7 participants