Skip to content

Conversation

@swagween
Copy link
Owner

extract functionality into an API-friendly header file, and re-work MediaPlayer to eliminate redundancy

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest moving MediaPlayer out of the library and into the executable: that will also get rid of the ImGui / gvdi dependency. The lib can then only link to capo and libxmp, and games/engines linking to it will be free to use their own windowing/rendering systems.

Copy link
Owner Author

Choose a reason for hiding this comment

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

good idea, done!

@swagween swagween requested a review from karnkaul June 25, 2025 07:05
@swagween swagween marked this pull request as ready for review June 25, 2025 07:50
Comment on lines 40 to 42
m_source->play();
m_status = m_source->is_playing() ? MediaStatus::playing : MediaStatus::stopped;
m_source->set_looping(looping);
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Call set_looping() before calling play(), just in case it finishes playing before you manage to set it to loop 😛
  2. IMO m_status is unnecessary, and risks going out of sync: it may be playing now but could have stopped in the next second


void Jukebox::stop() {
m_source->stop();
m_source->set_cursor({}); // seek to start
Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep in mind that this does nothing for XM streams.

Comment on lines 56 to 58
void Jukebox::update() {
if (m_status == MediaStatus::playing && !m_source->is_playing()) { m_status = MediaStatus::stopped; }
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Getting rid of m_status (and instead just interpreting m_source->is_playing()) will also enable getting rid of this update() function entirely.

Copy link
Owner Author

Choose a reason for hiding this comment

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

yeah, agreed that status is kind of useless now. I was holding on to it just in the case of standard compressed audio format + tracking whether the stream is paused or stopped, but I'm just going to get rid of it.

Comment on lines +3 to +4
add_library(jukebox-app)
add_library(jukebox-app::lib ALIAS jukebox-app)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is also a library, is there no example executable anymore? What are we supposed to run after building / during development?

Copy link
Owner Author

Choose a reason for hiding this comment

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

the executable is in app/src

@swagween swagween merged commit e79ac2b into main Jun 26, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants