Repeat patterns in arrangement by stretching #92
Conversation
YuriSizov
added
enhancement
|
Nice, thank you for working on this! I'll check this in more detail in a few days, but wanted to leave two superficial notes:
Also eventually every new shortcut must end up on the help pages, both the shortcut list in-app, and the list in the docs. If you want to take a look into addressing that, that'd be appreciated! Written docs themselves and the interactive guide I can update myself later. But you can also wait and not waste your time in case I have more technical notes here :) |
|
On A, yeah, it was mostly just I needed something. There's some case ("A for adding" was in my head), but if you've got a better suggestion, I'm all ears. |
There was a problem hiding this comment.
Hey again! Sorry for a bit of a delay, I was hoping to be done with another project sooner. I checked your pull request and it's on a good track. The logical part of setting the data looks fine to me, albeit in need of some cleanup. The preview needs more work, though. I've outlined the issues in review comments.
Let me know if you're still up to it to continue working on it. It's fine if you're no longer available too, it's a good start and I can get back to it myself when I'm working on the next release. It's not going to be for another couple of months, so there is no rush!
One last general style thing, if you're going to continue. I prefer to have one commit per PR. So you if you can squash everything together, that would be great. You can squash and force-push to the same git branch, and it'll update the PR automatically. That's not a git or GitHub thing, just my preference for this project.
That one commit must have a clear and succinct title that describes the feature in present tense without too much detail, e.g. "Implement stretching to repeat patterns in arrangement". In the body of the commit message you can write "Closes #88" to link the PR to the issue, so it gets closed automatically when the PR is merged.
And yeah, on the second thought, let's call it stretching. It feels more natural in the end.
|
|
||
| var drag_data := DraggedPattern.new() | ||
| drag_data.pattern_index = pattern_idx | ||
| drag_data.starting_pos = _get_cell_position(_get_cell_at_position(at_position)) |
There was a problem hiding this comment.
This needs to be adjusted for the current scroll offset, so we can correctly handle scrolling as we stretch the pattern. We should remember the absolute position in the timeline here, and then, when we draw, we would deduct the actual offset for the time to visualize the range.
| elif Input.is_key_pressed(KEY_A): | ||
| _set_pattern_over_range(pattern_data.starting_pos, _at_position, pattern_data.pattern_index) |
There was a problem hiding this comment.
As mentioned in earlier, we should use a modifier key here. I think Ctrl/Cmd is safe for use. Alt doesn't make sense semantically, and Shift would overlap with the marquee selection which we have in notes and might have with patterns in the future.
| # Same hack as later on. Hey, it works! | ||
| if on_control.get("stretched"): | ||
| var starting_pos = on_control.get("starting_pos") |
There was a problem hiding this comment.
So this is where I'd suggest a completely different approach. While abusing drag previews is tempting for this, its purpose is mainly to show the dragged content next to the cursor. For the indication on the hovered elements we should do the drawing inside of those elements instead. In this case that would be the PatternMapOverlay control.
Besides breaking the responsibilities, your approach also leads to visual glitches as you move the mouse. Since controls are pixel-aligned, the trick below to cancel mouse position out of the drawing position is not stable and there is jitter. But really, this is simply not a good way to organize such feature.
Inside of the drawing method of PatternMapOverlay we should replace the note cursor with a different indicator when stretching happens. Regarding the visuals themselves, I think ideally we'd want to explicitly show the repeated pattern across all those bars. But it might be tricky, since PatternMapOverlay does not normally draw patterns. Worst case scenario — it can be the same note cursor look, a grey-white frame, but repeated on each bar.
This is also where you'd use the absolute starting position and adjust it to the current scroll offset to avoid drawing content that is not visible on screen.
| var stretched: bool = false | ||
| var reflect_transient_state: bool = false | ||
| var starting_pos: Vector2 = Vector2(-1, -1) | ||
|
|
||
| func _process(delta: float) -> void: | ||
| queue_redraw() |
There was a problem hiding this comment.
All these changes will have to go, of course, since it's not the preview that should do the drawing.
| current_arrangement.set_pattern(bar_index, cell.y, value) | ||
| ) | ||
|
|
||
| func _set_pattern_at_position(at_position: Vector2, pattern_idx: int) -> void: |
There was a problem hiding this comment.
If I didn't miss anything, this method is only used in _set_pattern_at_cursor, which is a one-line method. There is no point introducing this abstraction at this time, you should put all this directly in _set_pattern_at_cursor.
|
|
||
| Controller.state_manager.commit_state_change(arrangement_state) | ||
|
|
||
| func _set_pattern_at_position_uncommitted(arrangement_state, at_position: Vector2, pattern_idx: int) -> void: |
There was a problem hiding this comment.
arrangement_state is missing a type in the method signature.
|
|
||
| var cell := _get_cell_at_cursor() | ||
| func _set_pattern_over_range(starting_pos: Vector2, ending_pos: Vector2, pattern_idx: int) -> void: | ||
| var _add_head_position = starting_pos |
There was a problem hiding this comment.
Local variables shouldn't be prefixed with an underscore.
| if Input.is_key_pressed(KEY_ALT): | ||
| _clone_pattern_at_cursor(pattern_data.pattern_index) | ||
| elif Input.is_key_pressed(KEY_A): | ||
| _set_pattern_over_range(pattern_data.starting_pos, _at_position, pattern_data.pattern_index) |
There was a problem hiding this comment.
We don't really need to rely on _at_position. Same as other related methods here we can accept the position "at cursor". But if you think a generalized method for an arbitrary range is a better fit, that's also fine. However, _at_position needs to be renamed to at_position then. By convention it's called _at_position because it's an unused argument. But now it would be used.
| return | ||
| _set_pattern_at_position(get_local_mouse_position(), pattern_idx) | ||
|
|
||
| var cell := _get_cell_at_cursor() | ||
| func _set_pattern_over_range(starting_pos: Vector2, ending_pos: Vector2, pattern_idx: int) -> void: |
There was a problem hiding this comment.
We follow the codestyle recommendation to separate different methods using two empty lines throughout the project.
| func _set_pattern_over_range(starting_pos: Vector2, ending_pos: Vector2, pattern_idx: int) -> void: | ||
| var _add_head_position = starting_pos | ||
|
|
||
| # Jump to the next row; no point adding the pattern where it already is |
There was a problem hiding this comment.
One last codestyle nitpick, but please put a period at the end of code comments. Also it should likely say "column", and not "row" :)
|
I just wanted to write and let you know that I won't be able to work on this until August. The end of the semester was too busy and my summer job is literally in the middle of the desert lol. |
|
@Lucien-Aibel-Champlain Ah no worries! I hope to work on the next release before that so I'll probably complete your work myself then. But you still get the full credit! Thanks for working on this to begin with :) |
|
Alright! Sorry to leave a mess, and feel free to take some credit; it's still work done, and anyway I've already got the college credit I was in it for. But I will try and come back to pay it forward somehow. |
2 participants
Working on issue #88, holding "a" while dragging will put the pattern in all rows between where it started and where it was dropped. Has a preview graphic (a little rough) and works correctly with the undo tree.