/live/clip/get/notes should return all notes#87
/live/clip/get/notes should return all notes#87StrongBearCeo wants to merge 7 commits intoideoforms:masterfrom
Conversation
… appear after clip.end_marker - Add clip start_marker, end_marker get / set
|
ideoforms
left a comment
ideoforms
left a comment
There was a problem hiding this comment.
Nice work! I've added a few questions and comments inline.
| | /live/clip/duplicate_loop | track_id, clip_id | | Duplicates clip loop | | ||
| | /live/clip/get/notes | track_id, clip_id | track_id, clip_id, pitch, start_time, duration, velocity, mute, [pitch, start_time...] | Query the notes in a given clip. | | ||
| | /live/clip/get/notes | track_id, clip_id, [from_time, from_pitch, time_span, pitch_span] | track_id, clip_id, pitch, start_time, duration, velocity, mute, [pitch, start_time...] | Query the notes in a given clip. | | ||
| | /live/clip/get/notes_range | track_id, clip_id, [from_time, from_pitch, time_span, pitch_span] | track_id, clip_id, min_start_time, max_end_time, min_pitch, max_pitch | Query the note range within a given clip. This is helpful in conjunction with /live/clip/get/notes to paginate and handle a large number of notes | |
There was a problem hiding this comment.
I think a neater approach here would be just to have the single /live/clip/get/notes endpoint, with the final 4 parameters as optional (min_start_time, max_end_time, min_pitch, max_pitch) - would that work? That would then also have symmetry with /live/clip/remove/notes.
There was a problem hiding this comment.
@ideoforms This api was added as a temporary fix for #88, before PR #89 was made
If there are too many notes, /live/clip/get/notes would fail. To address this, /live/clip/get/notes_range was introduced to determine the range of notes, and later /live/clip/get/notes could be called with optional parameters [from_time, from_pitch, time_span, pitch_span] for pagination purposes.
After merging PR #89, this API may no longer be required. However, it would still be beneficial to retain it for those who prefer to implement pagination.
There was a problem hiding this comment.
Thanks, will have another think on this once #89 is merged 👍
README.md
Outdated
| | /live/clip/set/loop_start | track_id, clip_id, loop_start | track_id, clip_id, loop_start | Set clip's loop start | | ||
| | /live/clip/get/loop_end | track_id, clip_id | track_id, clip_id, loop_end | Get clip's loop end | | ||
| | /live/clip/set/loop_end | track_id, clip_id, loop_end | track_id, clip_id, loop_end | Set clip's loop end | | ||
| | /live/clip/get/start_marker | track_id, clip_id | track_id, clip_id, start_marker | Get clip's marker start | |
There was a problem hiding this comment.
Is this a floating-point value in beats? If so, could you add this to the help text in the usage column?
(I realise the same criticism applies to loop_start, loop_end etc - laziness on my part! feel free to amend all of the above)
abletonosc/clip.py
Outdated
| self.logger.info("Trying to get notes from an empty clip") | ||
| return () | ||
| # Define default values | ||
| estimated_min_from_time = -16000 |
There was a problem hiding this comment.
default_min_from_time would be a better variable name here
|
|
||
|
|
||
| # Check if parameters are provided in the params tuple | ||
| if len(params) == 4: |
There was a problem hiding this comment.
Should flag an error if params has a length other than 4, as otherwise this would silently fail
There was a problem hiding this comment.
@ideoforms In the else statement, default parameters were used. Would you prefer to throw an error instead?
|
I've just integrated support for this with the latest commit. |
2 participants
Fix #86