Add Sample-Based Play Position Control (m_playposition_samples) #13822
Conversation
|
I'd kindly suggest formating your PR messages with Markdown. Especially:
would improve readability :) (PS: You can edit existing comments/PR messages) |
|
Just as an example (feel free to copy): This PR introduces a new sample-based control for tracking the play position in samples ( Key Changes
Benefits
|
| @@ -464,6 +466,7 @@ void EngineBuffer::setNewPlaypos(mixxx::audio::FramePos position) { | |||
| if (kLogger.traceEnabled()) { | |||
| kLogger.trace() << m_group << "EngineBuffer::setNewPlaypos" << position; | |||
| } | |||
| m_playposition_samples->set(m_playPos.toEngineSamplePos()); | |||
There was a problem hiding this comment.
Do you intend to set m_playposition_samples to the old value before the new play position is applied?
Otherwise, I'm confused why you set m_playposition_samples here instead of at the bottom of setNewPlaypos.
Although, as mentioned in the other comments, m_playposition_samples probably shouldn't even be set here at all, but only in updateIndicators.
| @@ -167,6 +167,7 @@ EngineBuffer::EngineBuffer(const QString& group, | |||
|
|
|||
| m_pRepeat = new ControlPushButton(ConfigKey(m_group, "repeat")); | |||
| m_pRepeat->setButtonMode(mixxx::control::ButtonMode::Toggle); | |||
| m_playposition_samples = new ControlObject<double>(ConfigKey(m_group, "playposition_samples")); | |||
There was a problem hiding this comment.
For consistency reasons, m_playposition_samples should probably intialized directly after m_playposSlider to keep playposition and playposition_samples closer together.
mixxx/src/engine/enginebuffer.cpp
Lines 158 to 164 in edae0ca
There was a problem hiding this comment.
Additionally, I'd suggest the name m_playPositionInSamples instead to adhere to camelCase naming conventions and to make the name a bit more descriptive.
| m_playposition_samples->set(position.toEngineSamplePos()); | ||
|
|
There was a problem hiding this comment.
| m_playposition_samples->set(position.toEngineSamplePos()); |
m_playposition_samples should only be set in updateIndicators to ensure consistent behavior with the other indicators (see comment above).
| m_playposition_samples->set(position.toEngineSamplePos()); | ||
|
|
There was a problem hiding this comment.
| m_playposition_samples->set(position.toEngineSamplePos()); |
m_playposition_samples should only be set in updateIndicators to ensure consistent behavior with the other indicators.
| @@ -1457,6 +1464,7 @@ void EngineBuffer::updateIndicators(double speed, int iBufferSize) { | |||
| // called yet. | |||
| return; | |||
| } | |||
| m_playposition_samples->set(m_playPos.toEngineSamplePos()); | |||
There was a problem hiding this comment.
| m_playposition_samples->set(m_playPos.toEngineSamplePos()); | |
| // Update playposition_samples CO | |
| m_playposition_samples->set(m_playPos.toEngineSamplePos()); |
|
In general, what is the exact use case of exposing the sample-level play position to controller scripts? There's a reason why the play position indicators is only updated 15 times/second instead of thousands of times per second. In general, the sample-level position you obtain in a controller script will probably be outdated before you even have a chance to send it to a controler device... mixxx/src/engine/enginebuffer.cpp Lines 50 to 51 in edae0ca |
|
thankyou for suggesting the changes , i will improve my pull request for the upcoming commits |
|
I have made the changes suggested by you, but I don't know why when i commit the changes i have made to specific files, other commits also get added , kindly help me |
I'll try :) Push your changes as they are to some branch in your GitHub repository and I'll help & try investigate. |
This PR introduces a new sample-based control for tracking the play position in samples (m_playposition_samples) within the EngineBuffer class. This enhancement provides more granular access to the play position, simplifying position-based comparisons and calculations in scripts and plugins.
Key Changes
New Sample-Based Control:
Added ControlObject m_playposition_samples in EngineBuffer.
This control provides the play position in samples, facilitating precise playback and position tracking in scripts.
Integration with updateIndicators:
The updateIndicators method now updates m_playposition_samples with the exact play position in samples. This ensures m_playposition_samples is always synchronized with the play position, improving accuracy in playback tracking.
Script Simplification:
This new control simplifies script calculations that previously required manual conversions from fractional play positions to sample positions, streamlining position comparisons and enhancing usability for developers.
Benefits
Improved Accuracy: The new sample-based control provides a finer level of detail than the fractional play position.
Ease of Use for Scripts: Developers can directly access the play position in samples, reducing the need for conversions in scripts.
Backward Compatibility: This change does not affect existing fractional play position controls, maintaining compatibility with prior functionality.
Testing
Verified that m_playposition_samples accurately reflects the play position in samples during playback and seek operations.
Confirmed that m_playposition_samples updates consistently in sync with updateIndicators.