pygame-community / pygame-ce

🐍🎮 pygame - Community Edition is a FOSS Python library for multimedia applications (like games). Built on top of the excellent SDL library.
https://pyga.me
936 stars 155 forks source link

``pygame.music.Music`` implementation #3108

Open bilhox opened 2 months ago

bilhox commented 2 months ago

Hello everyone,

As discussed on discord and on #3058 , I want to introduce a new pygame object that will help users to manipulate their musics in a better way, and also in a more pythonic way. The goal of this issue to collect feedbacks on how it should be implemented before opening a PR for its implementation.

Implementation

pygame.Music is an alias of pygame.music.Music. pygame.music should be an alias for pygame.mixer_music.

class Music:
    def __init__(self, path):

        self.metadata : dict
        # or
        self.title : str
        self.album : str
        self.artist : str
        self.copyright : str

        self.position : float # Settable / Gettable (set/get from where it should/is play(ing))
        self.duration : float # Read-only (returned time is in seconds just like SDL Mixer 2.6.0 functions use)
        self.playing : bool # Read-only (Could be pygame.mixer.music.is_playing(Music) ?)
        self.fade_in : float # Gettable / Settable (the value is in seconds)
        self.fade_out : float # Gettable / Settable (the value is in seconds)
        self.paused : bool # Gettable / Settable
        self.ended : bool # Read-only (True only if the position >= duration or music stopped)

        # If one day it's doable to play multiple musics at the same time, we could have
        self.volume : float # Gettable / Settable

    # For each method below, their equivalent in pygame.mixer.music has the same behaviour

    def play() -> None: ... 
    def rewind() -> None: ...
    def stop() -> None: ...

# New module functions
pygame.mixer_music.get_music() -> pygame.Music | None # Returns the music loaded

path is not necessary a str but support all types that pygame supported before.

I don't like how pygame.mixer.music.set|get_endevent works, it's not something the user have to setup. This is why I propose a better way IMO, when Music.stop() or pygame.mixer.music.stop() is called, or when the music ended peacefully, a pygame.MUSICENDED is sent to the event queue. This event would have a single attribute, music which is the music object that ended.

I highly support the metadata attributes than an actual metadata dict attribute, imo it doesn't look cool to write. Btw, what about an attribute for the music format type (ogg, wav, mp3 ...) ?

Why the need of this feature ?

For many reasons :

Updates

damusss commented 2 months ago

Yes. Please enlighten my feedback with yours, oh majestic comment reader.

Most important detail

I think this object should be contained in the pygame.music module. This should be a "modern" api, disconnected from the old api. having it in a new module means:

Relevant details

bilhox commented 2 months ago

I think this object should be contained in the pygame.music module. This should be a "modern" api, disconnected from the old api.

I wrote pygame.Music which is an alias to pygame.mixer.music.Music or pygame.music.Music. I should mention it in the original message.

damusss commented 2 months ago

I wrote pygame.Music which is an alias to pygame.mixer.music.Music or pygame.music.Music

I specified the alias should in fact be of pygame.music.Music. Get pygame.mixer_music.Music out of here :kekw:

bilhox commented 2 months ago

This is because this object completely removes the need for mixer_music.

No, it's not the goal. All functions available right now will still update the music object and do what they do. Also I forgot to talk about a really useful function which I'll add to the original message. And yes, we're aiming for a pygame.music alias 😭 .

damusss commented 2 months ago

No, it's not the goal. All functions available right now will still update the music object and do what they do. Also I forgot to talk about a really useful function which I'll add to the original message. And yes, we're aiming for a pygame.music alias 😭 .

I don't understand, why would someone use mixer_music if this object exists? apart from volume, this looks complete to me. and I think get/set volume should reside in pygame.music

I don't understand another thing, pygame.music would not be an alias, it would be a new module. unless we have different ideas.

aatle commented 2 months ago

My thoughts and observations on the modules (note I haven't really used mixer or music modules in my projects yet):

Nowhere in the docs is the "pygame.mixer_music" module name mentioned specifically; the music module is only publicly exposed via the pygame.mixer.music submodule.

The current implementation where pygame.mixer.music is just an alias to pygame.mixer_music (as opposed to being a real submodule) has import problems. You cannot directly import functions from the music module.

import pygame.mixer.music  # ModuleNotFoundError
from pygame.mixer.music import play  # ModuleNotFoundError

This same problem may occur if you do an alias variable pygame.music for pygame.mixer.music.

Three options for music module:

  1. pygame.music is a new alias of pygame.mixer.music, which itself is an alias of the undocumented pygame.mixer_music. Note that the original implementation hadn't just decided to use the name pygame.music in the first place.
  2. No new module or module alias is added. The new class Music is placed inside the original pygame.mixer.music module next to all of the module functions. (Similar to pygame.mixer.Sound/Channel, which also have aliases pygame.Sound/Channel.)
  3. A new, separate module named pygame.music is created for the new Music class. It's best if this is an independent module with no functions, like pygame.rect.Rect and pygame.surface.Surface. (Here, Music will eventually be available from pygame module directly.) May be a bit confusing (with pygame.mixer.music and pygame.mixer_music existing).

In any case, I agree that Music class should have all of the functionality of the existing music module.

aatle commented 2 months ago

Opinions about implementation:

bilhox commented 2 months ago

ended property: what is the difference from the playing property?

Good question, playing is set to False if the music is paused or if it ended. ended however is set to True if the music ended or the music was stopped.

I think volume should be a property (especially to complete the full functionality). The global volume would automatically be set to local volume inside play(), and both would be kept in line with each other while playing.

I don't support this idea for now, as we can't play 2 musics at the same time. It wouldn't make sense in my opinion to have a local and a global volume.

get_music() function seems strange; I say we exclude it for now. It's not guaranteed that the loaded music has an object.

When a music is loaded using a path like before, if you call this function, it's supposed to build you a music object and return it to you. This allows you to edit the music when you don't have access to it in a certain part of your code.

MUSICENDED is definitely more intuitive to use. I think it should indeed trigger for both natural end and manual stop, but also include a stopped bool attribute for differentiating the two. This aligns better with how events are used in general.

I agree.

damusss commented 2 months ago

@aatle @bilhox playing should be True even if the music is paused, if you care about it you can check for paused aswell, in my opinion, which would remove the need of ended. regarding the position, @aatle , we don't need to worry about formats as it's handled by Mix_GetMusicPosition. the mixer_music.get_pos implementation does NOT use that function, that's why it's broken. @bilhox mixer and sounds already have both global volume and local sound volume. it's not that absurd to me, I absolutely think one should be able to customize a music's local volume. it's consistent and it makes sense because maybe one music file is natively louder than another, you'd set one's volume to 1 and the other to 0.8 so they sound the same, while keeping the global volume to 1.

important question to both, why would end event be raised when you stop the music manually? what is the benefit? what if I want my endevent code only to run when it ends gracefully? I don't like "stopped" as an event attribute, it's unintuitive and ugly api-wise in my opinion. what other parts of pygame raise events on manual operation, to justify this?

bilhox commented 2 months ago

mixer and sounds already have both global volume and local sound volume.

This is because with mixer you can play sounds and channels at the same time. It is not the case for musics.

bilhox commented 2 months ago

why would end event be raised when you stop the music manually?

This is the behaviour of ENDEVENT, and I still find this behaviour understandable. If you stop the music, you can't unstop it, and continue from where you stopped. If you want to play the music after it got stopped, you have to restart the music which definitely means, you ended the music playback.

damusss commented 2 months ago

This is because with mixer you can play sounds and channels at the same time. It is not the case for musics.

damusss commented 2 months ago

This is the behaviour of ENDEVENT, and I still find this behaviour understandable. If you stop the music, you can't unstop it, and continue from where you stopped. If you want to play the music after it got stopped, you have to restart the music which definitely means, you ended the music playback.

If this is how ENDEVENT works, then I can understand it, for the sake of consistency. thanks for letting me know!

bilhox commented 2 months ago

we don't need to worry about formats as it's handled by Mix_GetMusicPosition.

While you're mentionning it, IMO I would not, for the sake of maintainability, create a system for the less than 0.5% users using SDL mixer with a version lower than 2.6.0. (You can check the code behind the current implementation, it's a bit terrible) It's not like people are supposed to have this version now, especially when the recent versions of pygame use SDL Mixer 2.8.0 since a pretty long time.

aatle commented 2 months ago

Good question, playing is set to False if the music is paused or if it ended. ended however is set to True if the music ended or the music was stopped.

Hmm, if I understand correctly, ended is derivable from playing and paused. If it's derivable from existing properties, then I don't think it should be implemented, agreeing with damusss. (It can always be added later if it is exceptionally common.)

get_music() function seems strange; I say we exclude it for now. It's not guaranteed that the loaded music has an object.

When a music is loaded using a path like before, if you call this function, it's supposed to build you a music object and return it to you. This allows you to edit the music when you don't have access to it in a certain part of your code.

I still disagree with the get_music() function: if it builds a music object in the call, then it may create a duplicate music object (if user has a separate one), and these cannot be synced. The function would make more sense if Music class were the only way to play music. The existing API will get excluded. It is the responsibility of the user to make sure their own instance of Music is accessible to code that will edit it.

playing should be True even if the music is paused, if you care about it you can check for paused aswell, in my opinion, which would remove the need of ended.

Though, the same can be said for if playing is False when paused. <- I think this should be the behavior because it aligns with the existing behavior (from docs), and is more intuitive.

aatle commented 2 months ago

Regarding volume, this property will still be useful even when only one music can be played at a time. First, it completes the core functionality for music, so that music functions don't have to be used with Music objects for this. Second, it is a more intuitive interface and enables volume to be easily manipulated.

However, the global and local volumes would indeed have to be synced, so perhaps it could be a class property instead, the balanced middle ground. Unfortunately, there may be implementation problems here.

Note: currently, a rounding issue with get_volume()/set_volume() needs to be settled before this can be implemented.

damusss commented 2 months ago

@aatle playing, paused, position, volume would all be properties (some read only) as they use sdl under the hood

hold on, why class property? instance property!

aatle commented 2 months ago

I meant a property that just delegates to global volume, all Music object's volume would be the same.

damusss commented 2 months ago

I meant a property that just delegates to global volume, all Music object's volume would be the same.

what's the utility of that? mixer music volume already exists. it makes more sense as an instance property

aatle commented 2 months ago

What I mean is an instance property that is always the global volume, to mitigate any problems with having both local and global volumes.

bilhox commented 2 months ago

As currently, pausing / stopping is done for any playblack, I would like to remove temporarly paused setter and stop method from the list, you can still get the exact same behaviour using pygame.mixer_music.pause/unpause/stop.

I'll be taking in account the suggestion for volume.

EDIT: same for rewind

aatle commented 2 months ago

Shouldn't the pause, stop, and rewind functionality be available in the Music class? Yes, it's possible to get the behavior using the global pygame.mixer.music functions, but so is the entirety of the Music class.

Note: paused would be readonly, with a pause() and unpause() method for setting it

bilhox commented 2 months ago

I found a solution for the problems above, therefore, I think when a new music is playing while another (Music or loaded with the old system) is already playing, the other music is stopped (by stopping all playbacks using Mix_HaltMusic), and the new music is played.

Also, I noticed that we can add effects on the music stream using the callback function used by the old system to increment the position. Might be interesting to play with it if you have any famous effects in your mind.

bilhox commented 2 months ago

Updates about the implementation:

damusss commented 2 months ago

2 questions:

bilhox commented 2 months ago

why no setter???

What setters ?

what happens when an unsupported format tries to get the duration? (what kind of error?) how performat is the duration getting? what kind of sdl function does it use?

I use Mix_MusicDuration (SDL 2.6.0 function) for this, for wav and midi files, it returns a random const value (not even -1.0 for example).

damusss commented 2 months ago

of course, the position setter! you didn't tell me what kind of other errors are raised for other unsupported formats, unless you already covered them all, in that case it's ok if it's mentioned in the docs

bilhox commented 2 months ago

the position setter

Ah this one, yes I'm curently reimplementing it as I found a solution for it as well.

aatle commented 2 months ago

So, can you give a summary of all of the properties and methods, and which may raise pygame.error for unsupported formats?

bilhox commented 2 months ago

So, can you give a summary of all of the properties and methods, and which may raise pygame.error for unsupported formats?

It'll not raise an error, the user didn't make a mistake so it doesn't make sense. Therefore, they will get -1.0 if it's not supported. I'm pretty much done for a first version, so except a PR soon.

aatle commented 2 months ago

Therefore, they will get -1.0 if it's not supported.

Okay. What about property setters though? Are they guaranteed to work?