Beat timeline from bpm - #568
Conversation
FelipeDefensor
left a comment
FelipeDefensor
left a comment
There was a problem hiding this comment.
Good idea and solid implementation.
Only a few code style issues, and most where there to begin with, but I think adding a new option crosses the threshold and makes it worth the refactor worth it. Let me know if the comments are not clear enough.
The only necessary thing missing are tests, and I think the existing ones for filling the timeline via other methods can easily be adapted.
There are a couple edge cases that could optionally be better covered, like the "freeze" resulting from inputting a huge BPM or the 0 beats that result from, say, choosing 1 BPM in a 10-second media. Your call whether to handle them.
| self._by_interval_edit.value() if checked_option == 1 | ||
| else self._by_bpm_edit.value() if checked_option == 2 | ||
| else self._by_amount_edit.value() |
There was a problem hiding this comment.
Since we have more than two options now, I think it is worth replacing this with a dict lookup.
| def __init__(self): | ||
| def get_result(): | ||
| checked_option = self._options.checkedId() % 2 | ||
| checked_option = self._options.checkedId() % 3 |
There was a problem hiding this comment.
Maybe we can use the legth of a dict instead of hard-coding the number here?
| self._by_amount_edit.setRange(1, 2147483647) | ||
| self._by_amount_edit.setSuffix(BEAT_TIMELINE_BY_AMOUNT_SUFFIX) | ||
| self._by_amount_edit.setValue(1) | ||
| self._by_bpm_edit.setRange(1, 2147483647) |
There was a problem hiding this comment.
Extract 2147483647 into a constant. I think it is used elsehwere in the codebase, so maybe tilia.consts is the place for it.
| @@ -655,6 +655,7 @@ def delete_components(self, components: list[TC]) -> None: | |||
| class FillMethod(Enum): | |||
There was a problem hiding this comment.
Making this into a StrEnum would probably make the code-style fixes fill_with_beast easier.
| for i in range(math.floor(duration / value)): | ||
| self.create_component(ComponentKind.BEAT, i * value) | ||
| elif method == BeatTimeline.FillMethod.BY_BPM: | ||
| interval_value = float(60) / float(value) |
There was a problem hiding this comment.
No need for the float wrappers here, / always returns a float.
|
@FelipeDefensor I went through and addressed your comments in the latest commit. Rather than use a dict, I used the enum values to map from the FillMethod to the int value, let me know if that's alright. |
FelipeDefensor
left a comment
FelipeDefensor
left a comment
There was a problem hiding this comment.
Thanks for the changes! Looks good now. Just missing the tests, and then I can approve.
| BY_AMOUNT_RANGE = 2147483647 | ||
| BY_BPM_RANGE = 300 |
There was a problem hiding this comment.
Thanks, but the idea was slightly different here.
I think 2147483647 is the max value for a QRadioButton, so my idea was to extract this here into something named Q_RADIO_BUTTON_MAX and refactor the remaining uses in the codebase. No need to do that, but if you don't, both of these variables should live in fill_beat_timeline.py, as they are specific to that module.
2 participants
Summary
Adds option to fill beat timeline using the BPM
Test plan
Create a beat timeline, click on the menu option to fill the beat timeline with beats. 3 options should show up, including fill by bpm.