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

Rename AudioStreamPlayer{,2D,3D} or VideoPlayer for consistency #3624

Closed
dalexeev opened this issue Dec 2, 2021 · 32 comments Β· Fixed by godotengine/godot#55670
Closed

Rename AudioStreamPlayer{,2D,3D} or VideoPlayer for consistency #3624

dalexeev opened this issue Dec 2, 2021 · 32 comments Β· Fixed by godotengine/godot#55670

Comments

@dalexeev
Copy link
Member

dalexeev commented Dec 2, 2021

Describe the project you are working on

Not related.

Describe the problem or limitation you are having in your project

@dalexeev godotengine/godot#16863 (comment) (6 πŸ‘)

AudioStream β€” AudioStreamPlayer{,2D,3D}
VideoStream β€” VideoPlayer

We should probably rename VideoPlayer to VideoStreamPlayer.
Or we can rename AudioStreamPlayer{,2D,3D} to AudioPlayer{,2D,3D}.

godotengine/godot#46997 (11 πŸ‘ 4 πŸŽ‰)

@mhilbrunner

Thanks! Sorry its taking time to review due to the backlog situation.

Discussed this with other team members and there does not seem to be consensus for this change, so I'm closing it for now. Still, thank you for suggesting it :)

@madmiraal

@mhilbrunner what is the argument for not making this change? Has it been decided to rename VideoPlayer to VideoStreamPlayer instead?

@mhilbrunner

There was no clear consensus for the renames either way - some for it, some against. If we are going forward with these renames, it should go through the proposal process to demonstrate there is clear support first. Discussion on the pros/cons can happen in that proposal.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

We have to choose one of the options:

  1. πŸŽ‰ = Rename AudioStreamPlayer to AudioPlayerΒ godot#46997.
  2. πŸš€ = Rename VideoPlayer to VideoStreamPlayer.
  3. πŸ‘Ž = Let it be as it is.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Just renaming.

If this enhancement will not be used often, can it be worked around with a few lines of script?

No.

Is there a reason why this should be core and not an add-on in the asset library?

This is part of the renaming in 4.0 aimed at improving naming consistency.

@amatrelan
Copy link

Sorry if wrong place to as but isn't stream player like player what reads data from disk as it plays? (This is how it was teached to me long ago I studied game engines). I tried to look from documentation but there wasn't anything how to play audio file from preloaded audio file, so I guess this handles all in one / only way to play data?

@fire
Copy link
Member

fire commented Dec 3, 2021

If you have no project that needs this, why should it be done?

@Mickeon
Copy link

Mickeon commented Dec 3, 2021

@amatrelan The AudioStreamPlayer is called like so because it plays the audio from its AudioStream stream Resource.
There's surely some optimisations in the background to make sure that loading audio from disk isn't a burden, but it's not something most users are concerned about. For that reason, I think it does make sense to simplify the name of the class to AudioPlayer. For something so commonly used, It was pretty long-winded, anyhow.

@ellenhp
Copy link

ellenhp commented Dec 3, 2021

I'm not sure I understand what this accomplishes. Less typing I guess, but I can't remember the last time I typed "AudioStreamPlayer" other than while providing help for people in the Discord. I usually just have to type "audio" into the node type picker dialog and then I click on the one I want. Also correct me if I'm wrong but we're likely removing the video player in 4.0 so that solves our consistency problem.

@dalexeev
Copy link
Member Author

dalexeev commented Dec 3, 2021

Also correct me if I'm wrong but we're likely removing the video player in 4.0 so that solves our consistency problem.

I have not found any suggestions to remove VideoPlayer.

Less typing I guess, but I can't remember the last time I typed "AudioStreamPlayer" other than while providing help for people in the Discord. I usually just have to type "audio" into the node type picker dialog and then I click on the one I want.

In this case, nothing will change for you either.

@YuriSizov
Copy link
Contributor

I have not found any suggestions to remove VideoPlayer.

It wasn't so much suggested, as it was decided due to problems with supporting it. You can find Calinou mentioning the likelihood of it in various "VideoPlayer" related proposals, for example.

@ellenhp
Copy link

ellenhp commented Dec 3, 2021

In this case, nothing will change for you either.

That's not quite true though, because all my projects will break, and all of everybody else's projects will also break. Not badly, but there are already people using Godot 4 so it's something to consider. While I wouldn't hesitate to break everyone's projects to make a rename that would clear up some confusion, this rename seems like it doesn't really solve confusion as much as making things shorter.

I don't know. I think my stance on this is pretty neutral but I don't agree that it's zero-cost.

@dalexeev
Copy link
Member Author

dalexeev commented Dec 3, 2021

It wasn't so much suggested, as it was decided due to problems with supporting it.

I'm not sure if something doesn't work very well, then we can just delete it altogether.

There's surely some optimisations in the background to make sure that loading audio from disk isn't a burden, but it's not something most users are concerned about. For that reason, I think it does make sense to simplify the name of the class to AudioPlayer.

This argument works even if the VideoPlayer is removed. Sometimes the evolution is towards simplification, for example, we renamed Spatial to Node3D.

I don't know. I think my stance on this is pretty neutral but I don't agree that it's zero-cost.

Yes, I agree that this is not zero cost. But IMO this is a very small cost.

there are already people using Godot 4

These people are using an unstable version (even alpha testing has not started yet), so they should be prepared for the fact that backward compatibility can break several times before release.

@Calinou
Copy link
Member

Calinou commented Dec 3, 2021

Edit: Following reduz's comment below, I no longer think renaming AudioStreamPlayer nodes to AudioPlayer is a good idea.


Renaming AudioStreamPlayer to AudioPlayer seems good to me. Since streaming is something that happens automatically behind the scenes (and the user doesn't need to know about it), I feel like "Stream" isn't a necessary distinction here. There is also no more non-streamed audio playback since Godot 3.0.

Also correct me if I'm wrong but we're likely removing the video player in 4.0 so that solves our consistency problem.

The VideoPlayer node won't be removed in 4.0, but the core video decoders will be. There will still be a GDExtension interface that will be used in an official add-on to support video playback.

@amatrelan
Copy link

I agree, in this stage it's useless even think that master branch is not breaking things for backwards compatibility.

And to rename all AudioStreamPlayer to AudioPlayer is marginal on any size of project sed -i 's/AudioStreamPlayer/AudioPlayer/g' $(find . -type f) Or like in any editor that has find and replace functionality.

@ellenhp
Copy link

ellenhp commented Dec 4, 2021

Since streaming is something that happens automatically behind the scenes (and the user doesn't need to know about it),

We don't stream audio from disk though, do we? Some AudioStreams are actual streams like microphone input, but AudioStreamSample, AudioStreamOggVorbis and AudioStreamMP3 all just load the whole file into memory I think.

@Xrayez
Copy link
Contributor

Xrayez commented Dec 5, 2021

If Stream part is not essential to understanding the inner working of the class, nor there's any other class that would require distinguishing between several similar classes (and no classes reserved for future implementation), then being explicit for the sake of it is a bad idea in general.

However, if we rename the base class AudioStream to Audio, it could be potentially confused for a singleton (there's AudioServer for this). But current inconsistency outweighs the potential confusion.

We also have to keep in mind that Godot tries to follow the convention of naming classes according to the base. Yet AudioStreamPlayer does not inherit from AudioStream, therefore I think it's fine to rename AudioStreamPlayer to AudioPlayer.

Since streaming is something that happens automatically behind the scenes (and the user doesn't need to know about it),

We don't stream audio from disk though, do we? Some AudioStreams are actual streams like microphone input, but AudioStreamSample, AudioStreamOggVorbis and AudioStreamMP3 all just load the whole file into memory I think.

There's AudioStreamGenerator as well, we even implemented MIDI/SoundFont player for this in Goost: goostengine/goost#157

@reduz
Copy link
Member

reduz commented Dec 5, 2021

There is a clear difference between VideoPlayer and AudioStreamPlayer. VideoPlayer plays a video file, it does not do streaming.

AudioStream plays a stream of audio, which can be the an audio file or sound generated by a multitude of ways (microphone, sound generator, etc). An AudioStream is not necesarily a file. As such, AudioStreamPlayer makes more sense.

The problem is more because of VideoPlayer more limited nature. This class is very seldom used so we will eventually move the video decoding code to an external extension for Godot 4.0. As such, I think these classes should be revisited, determine if VideoStream actually meets that expectation and either rename VideoStream to VideoFile, or VideoPlayer to VideoStreamPlayer.

@Xrayez
Copy link
Contributor

Xrayez commented Dec 5, 2021

Regardless of file or stream-based approaches, I think this should rather be an implementation detail anyway, especially for the engine like Godot, where simplicity is preferred over complexity for the most part, if I'm not mistaken.

@akien-mga
Copy link
Member

I suggest renaming VideoPlayer to VideoStreamPlayer, which solves the ambiguity and is consistent with other classes:

  • AnimationPlayer plays Animation resources
  • AudioStreamPlayer plays AudioStream resources
  • VideoStreamPlayer plays VideoStream resources

Starting with Godot 3, and even more so with Godot 4, we've gone to great length to make the API more explicit, and I don't see much gain here in removing the explicitness. Moreover, VideoPlayer is used a lot less than AudioStreamPlayer, so renaming VideoPlayer has much less impact on users and documentation/tutorials.

@dalexeev
Copy link
Member Author

dalexeev commented Dec 5, 2021

I suggest renaming VideoPlayer to VideoStreamPlayer

This is also a good option, and this is what I originally suggested. The reverse was my second idea, but it got much more support. Both options are good, but the option that people support is doubly good.

I think people supported the simplification option because in this case the clarification is really unnecessary, the user does not care what the audio is: a file or a stream. AudioPlayer is still a clear name: that's exactly what this node does. Analogy: XMLParser can parse both file and buffer, but it is not called XMLFileParser or XMLBufferParser.

I think clarifications are only needed when their absence would cause confusion for the user. If the purpose of the node is clear without clarification, then it is superfluous.

And besides, we have other examples of abbreviations, like HScrollBar instead of HorizontalScrollBar, TextureRect instead of TextureRectangle, etc. Why not expand their names too, if "a little more clarity is never superfluous"?

@Xrayez
Copy link
Contributor

Xrayez commented Dec 5, 2021

By now there are several solutions:

  1. Rename AudioStreamPlayer β†’ AudioPlayer
  2. Rename VideoPlayer β†’ VideoStreamPlayer
  3. Don't rename anything, because according to Rename AudioStreamPlayer{,2D,3D} or VideoPlayer for consistencyΒ #3624 (comment), it makes sense as-is.

Therefore, I think there's no consensus here (yet).

@ellenhp
Copy link

ellenhp commented Dec 5, 2021

I love that people are so passionate about improving Godot audio, but in my opinion this is far from the biggest problem we have right now. There is at least one major crashing bug remaining in the 4.0 audio system that I don't really have time to fix, and there are several quality of life improvements we could make with a bit of effort. If you're interested in Godot audio, here are some bugs that need fixing right now that would be good for a new contributor.

godotengine/godot#54821, godotengine/godot#53718 and godotengine/godot#52853 come to mind. I'm happy to provide guidance on any of these, especially the last two (they should be addressed at the same time and may require some discussion).

godotengine/godot#55608 (and related issues) would be another good one to address for 4.0, but will be a bit more involved.

I'm not trying to derail discussion here, but I just want to recognize that we're all here because we want what's best for the engine, and I want to suggest some alternate ways we could spend our time. Because like @Xrayez said, we don't have a consensus and getting to a consensus will require some work on the part of all of us that we could be spending making the existing audio system behave correctly.

Edit: I know this isn't zero-sum, but I'm swamped by these bugs so I just wanted to ask for help :)

@dalexeev
Copy link
Member Author

dalexeev commented Dec 5, 2021

I love that people are so passionate about improving Godot audio, but in my opinion this is far from the biggest problem we have right now. There is at least one major crashing bug remaining in the 4.0 audio system that I don't really have time to fix, and there are several quality of life improvements we could make with a bit of effort.

I'm sorry if I brought some controversy to the community with my proposal, I didn't mean it. This was supposed to be just one of the many 4.0 renames that are already happening. I am not trying to be principled and do not rate this as any significant or even important contribution. It's just an opportunity to make Godot a little better. That which is within the limits of my small knowledge and capabilities.

But I don’t think this proposal is wasting the contributors' time in any way. In Open Source, people spend time on what they want to spend time on, no matter how important the problem is. You cannot get people to β€œdo more important things,” even if they are really more important. And sometimes it is difficult to assess the importance of the problem.

@ellenhp
Copy link

ellenhp commented Dec 5, 2021

I'm not trying to force anyone to do anything, merely asking for help. Sorry.

@madmiraal
Copy link

AudioStream plays a stream of audio, which can be the an audio file or sound generated by a multitude of ways (microphone, sound generator, etc). An AudioStream is not necesarily a file. As such, AudioStreamPlayer makes more sense.

If it's an audio file it's not streaming. From Wikipedia:

"Streaming media is multimedia that is delivered and consumed in a continuous manner from a source, with little or no intermediate storage in network elements. Streaming refers to the delivery method of content, rather than the content itself. "
"Distinguishing delivery method from the media applies specifically to telecommunications networks, as most of the traditional media delivery systems are either inherently streaming (e.g. radio, television) or inherently non-streaming (e.g. books, videotape, audio CDs)."

Therefore using AudioStreamPlayer doesn't make sense, and it should be renamed AudioPlayer, because it works for both streamed and non-streamed resources.

@amatrelan
Copy link

Streaming has multiple meanings and that's the "newest" one. Streaming in software can be also streaming from files, you read it as you play it, what also in a sense is Streaming.

Another way to take data is to load whole file in memory, what is generally used in Game engines in sound effects what can be played multiple times and kept in memory even after play ends, so it can be played again without any delay.

@Xrayez
Copy link
Contributor

Xrayez commented Dec 6, 2021

@ellenhp here's my proposal: lets rename the class as proposed here (AudioStreamPlayer β†’ AudioPlayer), and I'll help you with fixing those bugs.

@akien-mga
Copy link
Member

@ellenhp here's my proposal: lets rename the class as proposed here (AudioStreamPlayer β†’ AudioPlayer), and I'll help you with fixing those bugs.

You're very welcome to help fix audio issues, but proposing a barter like this to lobby for an unrelated proposal is not appropriate.
The proposal workflow is intended to document, discuss and evaluate the use cases and implementation for change proposals, not lobby maintainers.

@dalexeev
Copy link
Member Author

dalexeev commented Dec 6, 2021

You're very welcome to help fix audio issues, but proposing a barter like this to lobby for an unrelated proposal is not appropriate.

Fair. But in this case, the argument "there are more important problems" does not work either. This proposal should be considered on its own, whether there are more important issues or not. Since this comment, our discussion has veered in the wrong direction.

@reduz
Copy link
Member

reduz commented Dec 6, 2021

I explained it here, but I will paste the same comment here:

I will give a bit more depth on why this is likely not going to happen. For 4.0, we went strongly towards the direction of making node names more explicit on their function. Examples of this are:

All 3D nodes now end in 3D

  • Physics nodes had renames like RigidDynamicBody, SoftDynamicBody, etc.
  • Others have more explicit renames such as MeshInstance3D, VisibleOnScreenNotifier, etc.
  • This node plays an AudioStream, which can be either a file or something else (like user generated audio, microphone stream, etc). If confusion arises (which is unlikely given there is not any other node called even similarly), it's better in this case to improve the documentation of AudioStream to note that an AudioStream can be a file or another type of stream.

In any case, if you want to leave this proposal open I am fine, but for the time being you don't have my consensus to go forward with it.

@reduz
Copy link
Member

reduz commented Dec 6, 2021

@Xrayez

@ellenhp here's my proposal: lets rename the class as proposed here (AudioStreamPlayer β†’ AudioPlayer), and I'll help you with fixing those bugs.

This is a pretty bad attitude to have and I would suggest refraining from doing similar comments in the future. It's up to you to fix bugs or not, but do not condition your help to others based on their design choices.

@Xrayez
Copy link
Contributor

Xrayez commented Dec 6, 2021

Fair. But in this case, the argument "there are more important problems" does not work either. This proposal should be considered on its own, whether there are more important issues or not. Since this comment, our discussion has veered in the wrong direction.

@dalexeev yes, you are right. "Как аукнСтся, Ρ‚Π°ΠΊ ΠΈ откликнСтся" (for non-Russian speakers, this means: "What goes around, comes around").

@Mickeon
Copy link

Mickeon commented Dec 6, 2021

In this case, following @reduz 's comment, the conclusion of this proposal would be to rename VideoPlayer to VideoStreamPlayer, instead?
Not that I, or some other users concur. But this inconsistency shouldn't be kept as it is, now that there's a chance to fix it.

@akien-mga
Copy link
Member

I suggest renaming VideoPlayer to VideoStreamPlayer, which solves the ambiguity and is consistent with other classes:

  • AnimationPlayer plays Animation resources
  • AudioStreamPlayer plays AudioStream resources
  • VideoStreamPlayer plays VideoStream resources

Done: godotengine/godot#55670.

@dalexeev
Copy link
Member Author

dalexeev commented Dec 6, 2021

Democracy... has not won. πŸ˜„ But if this is the vision of the project maintainers, I will no longer argue and support this decision.

akien-mga added a commit to akien-mga/godot that referenced this issue Dec 6, 2021
It's a player for `VideoStream` resources, just like `AudioStreamPlayer` is a
player for `AudioStream` resources.

Closes godotengine/godot-proposals#3624.
@aaronfranke
Copy link
Member

aaronfranke commented Dec 6, 2021

Either way works, but I don't understand how adding/keeping "Stream" makes it more clear. I mean, sure it makes it more explicit that it plays streams of audio, but it doesn't reduce ambiguity or add clarity.

With things like StaticBody -> StaticBody3D, there is confusion with the similarly named StaticBody2D, so the 3D suffix differentiates it. With AudioStreamPlayer, if there aren't any non-stream audio player nodes, then what is the point of the "Stream" part? There aren't any non-stream audio players to disambiguate it from.

Similarly, if we wanted to rename StaticBody3D to PhysicsStaticBody3D, you could say that the "Physics" makes it more explicit because it's for physics, but since there aren't any non-physics StaticBody3Ds, how does it help with clarity and disambiguity?

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