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

Timestretch is optional #54

Closed

Conversation

chrisbc
Copy link
Contributor

@chrisbc chrisbc commented Jun 14, 2023

PR functional impact and testing guide ...

The description of the Pull request must also contain information on what functional areas have been touched and should be tested, ideally including a small test manual

useful for less beat-centric music e.g. ambient, this allows user to enable/disable audio time-stretching, either:

  • for all audio clips in the song => with shift+SYNC-SCALING button . This it works in Clip, Song and Arranger Views.
  • for an individual audio clip => with either clip/sample/stretch menu, or shift+Sample1/Mode shortcut)

See also Make time stretching optional for AudioClips

Implementation notes

This feature is implemented in the AudioClip::render method where most of the pitch-shift/time-stretch action happens. the pre-exiting goto justDontTimestretch; is simply used to good effect. Ottherwise this just defines new menu/shortcut options with more verbose strings for OLED model. And it writes new attribtues to SongXML files (see below)

CommunityFeatures.XML changes

None needed - as this doesn't change default deluge behaviour. Potentially this could be added to protect against accidental updates to SONG.XML files, but these are backwards compatible already. Open to suggestions tho.

Song XML changes

Two XML attributes are added to SONG XML files for and elements (timeStretchingEnabled (bool) . These attributes will be ignored by earlier firmware. The default time-stretching setting is on to remain consistent with earlier versions.

This has been added to Wiki docs )

User tests

  • clip/sample menu set/reset timestretch for individual clip 7SEG/OLED shows STRE/Timestretch -> On/Off
  • shortcut menu for individual clip 7SEG/OLED shows on|off/Time-stretch: On/Off
  • set timestretch on/off for song 7SEG/OLED shows TSon|TSof/Time-stretch: On/Off
  • roundtrip upgrade old SONG file adds new attributes.
  • roundtrip modified song XML loads OK on Official 4.1 firmware. TODO ...check what happens if new song XML is written to with old Firmware? I expect the attributes will be stripped out.

Notes:
There's a conflict with PR #211 over the reassignment of the original SYNC_SCALING button. Here, I'm leaving it alone, and using Shift+SYNC-SCALING, while Danny is proposing to relegate legacy function the same shifted button. One alternative is to let Danny's mod override the legacy function via a CommunityFeatures menu switch, and not not touch the shifted option.

@chrisbc chrisbc changed the title Timestretch is optional [Draft] Timestretch is optional Jun 14, 2023
@sichtbeton
Copy link
Contributor

Would your new function be compatible with this?

#141

@chrisbc
Copy link
Contributor Author

chrisbc commented Jul 5, 2023

Would your new function be compatible with this?

#141

In principle I think so ... I am very interested in this feature too, so will do an integration branch on my fork soon with both these features to see if they play nicely.

@chrisbc chrisbc changed the title [Draft] Timestretch is optional Timestretch is optional Jul 21, 2023
@chrisbc chrisbc marked this pull request as ready for review July 21, 2023 04:16
@weavermedia
Copy link
Contributor

Notes: There's a conflict with PR #211 over the reassignment of the original SYNC_SCALING button. Here, I'm leaving it alone, and using Shift+SYNC-SCALING, while Danny is proposing to relegate legacy function the same shifted button. One alternative is to let Danny's mod override the legacy function via a CommunityFeatures menu switch, and not not touch the shifted option.

Conflict with my PR #211 noted 👍

Given that FILL mode is intended to be a performance feature I would like it to be on a primary, non-shift button if at all possible. I like the idea of having the sync scaling button serve as either sync scaling or fill activation (if I understand your idea correctly) via a community menu setting. Which, as you say, will leave the shift function untouched.

@PaulFreund
Copy link
Collaborator

Since not all tasks are marked as done and there are currently conflicts it might make sense to convert this PR to a draft PR until you are positive it is ready and can ber merged.

@weavermedia
Copy link
Contributor

@chrisbc I added a community feature setting for the sync scaling button action in PR #211 so I'm clear of shift+sync scaling now 😀

@sichtbeton
Copy link
Contributor

the conflicts in here are too big to resolve in the next days I guess. but I must say, having this in the first community release would be awesome (there is a feature cutoff proposed by the end of this week). but as in the community guidelines: everybody at their own pace. just wanted to let you know.

in any case: super cool feature!

@m-m-adams
Copy link
Collaborator

Is there still ongoing work on this PR?

@m-m-adams m-m-adams added the merge conflict github reports a merge conflict that should be resolved. label Aug 8, 2023
@PaulFreund PaulFreund added the stale PR was not updated in two weeks and will be closed soon. A new PR can be created to pick up the work label Aug 25, 2023
@PaulFreund
Copy link
Collaborator

Hey, a quick heads up :) After two weeks without any activity from the author of a PR we tag it with the "stale" label and close it after another week of inactivity as per our Contributor guidelins. This is not to create pressure or discourage anyone but to keep the list of Pull requests maintainable.

We are very happy about your contribution and nothing will be lost. You can pick up the work at any time by reopening the PR or creating a new one.

@PaulFreund PaulFreund closed this Sep 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge conflict github reports a merge conflict that should be resolved. stale PR was not updated in two weeks and will be closed soon. A new PR can be created to pick up the work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants