Project: document codebase, outline restructuring and code guidelines #2889
Conversation
|
Hey @RJ-Eckie thanks for opening this draft PR. You've made some good observations already :)! I'll provide some thoughts directly as comments to the commit line items you added. Cheers! |
| - None of the views inherit from view.h, which could be assumed would be the case from how the files are named. Instead all views inherit from `TimelineView` | ||
| - `ClipNavigationTimelineView` is a `TimelineView` and only implements a few methods. `TimelineView` itself is only used as a base class for `ArrangerView`. This could probably be simplified into one class | ||
| - **Proposal:** | ||
| - Combine `TimelineView` and `ClipNavigationTimelineView` to a new `View` class. This is a `RootUI` and will remain the base class of all other views. Shared behavior for views lives here |
There was a problem hiding this comment.
In principal I agree with this refactoring approach of having a base view class from which all other view classes inherit.
To improve understanding of the implications of all this it would probably be worth searching for instances of getRootUI(), getCurrentUI(), changeRootUI(), &instrumentClipView, &automationView, etc. as it will give a better appreciation of how embedded those concepts are throughout the codebase.
In terms of refactoring view classes, it's definitely something we've talked about a lot. InstrumentClipView is the original big "catch all" class for interacting with melodic instrument and kit instrument clips. A lot of code could definitely be factored out to make the view code easier to comprehend.
If I give an example of automation view, which I initially built by taking a copy of instrument clip view and building the automation view functionality on top of, it's now a very huge beast of a class that needs to be refactored as it's just too big to manage now.
I want to refactor its current state so that automationView is a base class that other classes that take care of automationView specific functionality can inherit from / override.
e.g. Note Velocity Editor and Parameter Automation Editor are both part of the automationView class but they should be pulled out into their separate classes.
When I started looking at it I started looking at splitting things out like this:
There was a problem hiding this comment.
I definitely intend to spend some more time getting an understanding of the UI classes and how everything is implemented, before starting any refactoring work. Documenting the UI folder is next on the list. I feel we have a lot to gain here in both accessibility and possibly stability/speed.
code_docs/DirectoryReference.md
Outdated
| |-|- | ||
| | `ArrangerView final` | is a `TimelineView` representing the arranger view | ||
| | `AudioClipView final` | is a `ClipView` for audio clips | ||
| | `AutomationView final` | is a `ClipView` that handles the automation layer of a clip |
There was a problem hiding this comment.
Funny enough this is revealing of a change that needs happen in the refactoring I eventually want to do. AutomationView is no longer just a clip automation view but it's also an arranger automation view. So to a reader / new contributor I can see how this is confusing.
There was a problem hiding this comment.
This needs a solution in one of a few ways, to reuse behavior in different scenarios:
- View as base class, ScrollingView as something like a template that can be implemented on views that work with the arranger
- View as base class with a toggle to allow arranger-like scrolling where necessary
- We could go absolutely wild and implement free scrolling on all views, while we're at it. (I honestly don't see any reason against it? Should of course be toggle-able to classic behavior.)
|
@seangoodvibes Thanks for all the feedback! |
|
Hey @RJ-Eckie thought I would share this page with you. For the user facing documentation website we're building (which we also would like to eventually integrate your developer documentation into), we started putting together terms to explain the various structures in the Deluge. https://seangoodvibes.github.io/DelugeDocs/contributing/deluge-style-guide/ While we were working on this, we did discuss wanting to align the terminology that users are familiar with with the terminology in the code wherever possible so that as you said, a user wanting to become a code contributor could easily make that transition. |
Oh man I literally just spent X hours writing exactly this 😂 |
|
@seangoodvibes Check out Modes vs Views vs Layouts which could be an update to your docs |
|
|
||
| 1. **Modes** describe mutually exclusive fundamental ways in which the device behaves. Currenly this would only apply to **Song Mode** and **Arranger Mode**. At any time, either one or the other is active - never both. This allows us to rename the Song Mode layouts to **Rows View** and **Grid View**. *This could be implemented without changing anything else.* | ||
| 2. **Views** exclusively describe ways of **viewing** and **interacting** with a **mode**. Currently this would only apply to **Rows View**, **Grid View** and **Performance View**. *(One could imagine a grid view for arranger mode, where the arrangement plays top to bottom, which would make all views compatible with both modes.)* | ||
| 3. **Editors:** clip and automation view operate fundamentally different from the more similar aforementioned views. To distinguish them from those views, they are labeled **Clip Editor**, **Clip Automation Editor** and **Arranger Automation Editor**. |
There was a problem hiding this comment.
I like where this is going. We're definitely on the same page regarding Modes (Song vs Arranger) and the Views to view and interact with the modes.
Regarding the editor concept. I think this term makes sense to me. You are basically editing a component of the project.
Regarding Automation View tho, it's a bit complex of a beast in the sense, it has an "Overview" component which basically acts as a "Report" / "Launchpad" so to speak which can bring you into the Editor.
Also the automation view currently has two UI's for editing parameter automation as well as note velocity. Those are definitely editor's and I agree with the term editor there:
- Parameter Automation Editor
- Note Velocity Editor
We'll also eventually have other editors too:
- Note Probability Editor
- Note Iteration Editor
- Note Expression Editor
So I'm not entirely sure how to present that to a user.
The way I see it, it's a UI for interacting with and editing various parameters, whether they be modulation or note parameters.
I think the term Automation is familiar to users when it comes to editing parameters like this.
When it comes to editing clips, we basically have 3 editing layouts:
- audio clip
- melodic instrument clip (synth, midi, cv)
- kit clip
There was a problem hiding this comment.
Re: automation view - see following commit
Re: editing clips - from a programmer's perspective, layouts would be a good label here. From the user's perspective, I would just call it "Editing audio clips in the clip editor." As they don't have any say over what layout they get, we might as well not label it
Hah sweet. I wrote some comments. I like where this going. And I think this is a good opportunity to align on terms. |
|
Also on the website I linked to above, if you click on the Documentation tab, you can see where I was going with breaking down the Project structures and User Interface structures: https://seangoodvibes.github.io/DelugeDocs/documentation/project/project/ Happy to receive any feedback on any of this. It's very much earlyyyyy stages. |
|
Thinking about it some more, on the editor concept Technically speaking, arranger view and song view are also editors. They are editing tracks and clips, sections (for song mode) and other project wide stuff. I think keeping the view term probably makes sense here imo. Because you are "viewing" what has been previously edited. You're not always editing either. Sometimes you're just playing back what's there, or playing the instrument without editing. We could have: Arranger View: View, Play, Edit arrangement Song View: View, Play, Edit song, Record to arrangement
Automation View: View, Play, Edit, Record automation sequences
Audio Clip View: View, Play, Edit Audio Loops Instrument Clip View: View, Play, Edit, Record note sequences
Keyboard View: Play instrument, Record note sequences
Performance View: View performance parameters, edit parameter latches, edit layout, record to arrangement
|
You can look at this from different perspectives. You can try to most accurately describe what you are doing/can do in a view, and you can try to name things in a way that distinguishes views in a way that is intuitive and helps you to navigate the device more intuitively. I'm specifically trying to do the latter. The point is not to say: this is a mode so you can't edit anything. The point is to say: this is a mode, which is mutually exclusive from another mode. The point is also not to say: this is a view so the only thing you can do here is view. The point is to say: this is a view, and it is analogous with all other things called view. In other words: you can expect similar behavior from things that are named similarly. Reading that whole list of views with wildly different behavior and scopes inside each of them makes me feel like we're not actively helping the user to understand what's going on |
|
@seangoodvibes We crossed eachother (again) so here's my comments with the proposal for refactoring the automation view: Even when what's being edited feels different (note velocity versus cutoff frequency,) ultimately each of them is mapping a parameter over time in an interface that is largely identical. So I would try to unify all of that into one area of functionality, which I would call Automation Editor. I would not give editing certain parameters (such as probability or note repeat) a different name. Described in a bit more detail in the most recent commit |
I think we're in alignment here on calling what you're editing "automation" and there being a single automation editor class. note velocity and parameter editing would be layouts of the automation editor. |
I guess my point and question here was:
|
| `AutomationEditorLaunchpad` presents different sets of parameters to edit depending on where/how it is launched. Instrument Clip View => [audition pad] + [song] could show Note Automation. | ||
|
|
||
| Considerations: | ||
| - Note automation like probability could be automated on clip *and* note level. (Which parameters should be automatable and how is a different discussion.) |
There was a problem hiding this comment.
Do you mean like edit the probability of all notes vs a single note? yea that's been discussed as well.
There's several scenarios here:
- edit the probability of a step (all notes in that step)
- edit the probability of a step (all notes in that step) across all rows (e.g. pitches / drums) in a clip
- edit the probability of all notes in a row
- edit the probability of all notes in a clip
There was a problem hiding this comment.
Exactly. I was mainly pointing out that having different levels of launchpad (clip level and note level) creates a clear place to store both levels of automation. This differs from the current implementation where velocity automation of note rows is mapped to a pad in the launchpad on clip level
There was a problem hiding this comment.
Exactly. I was mainly pointing out that having different levels of launchpad (clip level and note level) creates a clear place to store both levels of automation. This differs from the current implementation where velocity automation of note rows is mapped to a pad in the launchpad on clip level
Ah that's a good point about the launchpad mixing the row level velocity editor shortcut with the clip level parameter shortcut.
This happened mostly because we have not implemented the concept of turning Affect Entire on / off like Kit clips. The idea was to have two layers (affect entire - clip level, and not affect entire - row level)
|
|
||
| Considerations: | ||
| - Note automation like probability could be automated on clip *and* note level. (Which parameters should be automatable and how is a different discussion.) | ||
| - Some parameters will probably need a new property to be saved in, which the `AutomationEditor` will translate to a note/synth parameter depending on its editing layout |
There was a problem hiding this comment.
Correct the type of parameter will dictate the type of editing layout that should be opened.
| Considerations: | ||
| - Note automation like probability could be automated on clip *and* note level. (Which parameters should be automatable and how is a different discussion.) | ||
| - Some parameters will probably need a new property to be saved in, which the `AutomationEditor` will translate to a note/synth parameter depending on its editing layout | ||
| - The shortcut for calling the Launch Pad doesn't feel very consistent currently as it's either on the song or clip button. Ideally this would live on the same button wherever it can be called |
There was a problem hiding this comment.
So we have two shortcuts:
- clip in clip views. pressing the clip button to enter the launch pad / overview was selected because it felt analogous to switching between song view and arranger view. I like how this feels.
- using the clip button didn't make sense while in arranger view so we opted for shift + song
You can also enter automation editor via the menu as well. With a parameter menu open, pressing song while in the arranger menu opens the automation editor for the arranger parameter and pressing clip while in the. clip menu opens the automation editor for the clip parameter.
There was a problem hiding this comment.
The current shortcuts definitely aren't bad choices! But with automation editing becoming larger and more important in sequencing, I feel it would be great to have a single spot for you muscle memory to move towards, without having to check first what view you're in
Edit: I'm trying out some button combinations and I guess I just feel differently about this, especially as the clip button is currently unused in arranger mode. This allows for a single clip press to open arranger automation, and holding a clip + clip button to directly open clip automation. The consistency between interfaces, of result of a button-press, makes it make sense to me
There was a problem hiding this comment.
The current shortcuts definitely aren't bad choices! But with automation editing becoming larger and more important in sequencing, I feel it would be great to have a single spot for you muscle memory to move towards, without having to check first what view you're in
Edit: I'm trying out some button combinations and I guess I just feel differently about this, especially as the clip button is currently unused in arranger mode. This allows for a single clip press to open arranger automation, and holding a clip + clip button to directly open clip automation. The consistency between interfaces, of result of a button-press, makes it make sense to me
Oh yea just checked and you're right the clip button isn't used. We could definitely move the shortcut to clip then for consistency. Would be a lot faster way to get into arranger automation view also. I'll run the suggestion by folks on the discord and see what they think.
|
Ok understood. Regarding Song and Arranger as Modes, I don't think the UI should necessarily be called mode because Mode is an actual term in the codebase. See PlaybackMode class. I think we need a way to distinguish between a UI and the underlying architecture that the UI is interfacing with. Song View interfaces with the Song playback mode, but it also interfaces with the Arrangement playback mode when you are recording to the Arrangement and it also interfaces with the Clips when you are pressing the clips - you can record automation to a clip from Song View. And eventually if we ever add a keyboard to Song View (as someone wanted to do), it would be another way of writing sequences to a Clip. Let me know what you think. I'm personally not entirely sold on renaming Clip View to Clip Editor just to differentiate the naming from Song / Arranger / Performance View. |
I'll need to spend some more time with the UI code to say anything sensible about this. It makes sense to maybe first do cleanup, which would make renaming things easier without causing clashes. A lot of the restructuring doesn't rely on what we're naming stuff either so this doesn't need to be decided today. |
Yep sounds good. I think working on cleaning things up would definitely help clarify things. |
…e/DelugeFirmware into code-docs-and-guidelines
| - Modulation | ||
| - ... | ||
| - Any clip type | ||
| - Sequencing |
There was a problem hiding this comment.
Don't mind my comments, I'm thinking aloud here:
- I previously thought sequencing was just for Instrument Clip types but now I'm changing my tune.
- The sequencer in the Deluge produces an output by "processing the current playhead position" - for audio clips that means rendering the audio sample it contains and then processing that through the global clip modulation settings at that same position (e.g. sample + filter).
- For instrument clips, for the sequencer to generate sound, you must first place note (event) triggers across the timeline. The triggers create sound. Where modulation happens in the sound generation path varies depending on the type of modulation (e.g. it could at a voice level, or applied globally after). But modulation is also sequenced because it takes the modulation values at the same position as the note trigger.
- For kit clips we go even further by having sound generated at the kit row level and then additional modulation at the clip level (affect entire parameters).
- All this to say, while an audio clip is not traditionally what we think of as a sequencer, technically it is still sequencer with the audio sample having a single trigger at the start of the clip.
| ##### RootUI | ||
| It is unclear what the exact purpose of `RootUI` is. It basically just implements a list of virtual functions. The only way `RootUI` is used, is as the base-class of `TimelineView`, which we discussed above should be reworked into base-class `View`. This means these virtual function declarations should probably just move to `View` so we can lose `RootUI` altogether. | ||
|
|
||
| `getRootUI()` is declared in a bunch of places, which should probably be refactored to `getView()`, but more investigation into the entire UI management is needed before committing to any changes. `ui.cpp` does hold a few methods dealing with `rootUI`. |
There was a problem hiding this comment.
rootUI is not just used with Views.
If you look at the SoundEditor UI, which is the main class that handles the menu system, the root UI is used to check what the underlying view is which dictates whether and how you can interact with the pads of the underlying root UI. The current UI while in the menu would be the sound editor UI.
This exists because while in the menu you can still interact with the underlying UI in certain situations. E.g. using the audition pads, or the keyboard (in keyboard view) while in a parameter menu.
You'll see a lot of checks to see what the root UI is while you're in the menu.
These checks are used in other places as well that check to see if getRootUI() == getCurrentUI() or if getCurrentUI() == &soundEditor
There was a problem hiding this comment.
Yeah I probably could have worded this a bit more precisely, but currently I'm kind of writing it for my own understanding with the intention of ultimately refining it for "public consumption"
So what I'm trying to get around here is, what is the function of RootUI existing in the context of the entire codebase? Do we need RootUI to exist or can it be absorbed into other classes for simplification? Especially because it adds so little in terms of implementation.
When I say RootUI is only used with views, what I mean to say is: views are the only classes that inherit from RootUI. And having already discussed that views can use a good refactor anyway, we could easily absorb RootUI into the new View class without really touching any of the other code.
At line 87 I do mention the getRootUI() method, which would need to get refactored into View.
There was a problem hiding this comment.
I think the context might be the following:
The UI system in the Deluge actually has the capability of having multiple levels.
That is to say that you could start with a base (root UI), and then enter another UI, and then enter another UI.
So the root UI always points to the lowest level UI that if you back out all the way of the current UI it will bring you to the root UI.
To your point, I think a view will always be a root UI as you would not "back out" of one view to go to another view. You can however swap a root UI at level (as is done in the menu when accessing automation view when instrument clip view is the root UI).
Ultimately I think view would inherit from UI.
The UI class keeps track of the hierarchy of UI's that have been opened.
To me getRootUI, changeRootUI belongs in the UI class.
There was a problem hiding this comment.
To your point, I think a view will always be a root UI as you would not "back out" of one view to go to another view. You can however swap a root UI at level (as is done in the menu when accessing automation view when instrument clip view is the root UI).
Agreed
To me getRootUI, changeRootUI belongs in the UI class.
Yes, makes a lot more sense
|
@seangoodvibes Looking at the choice between Song/Arranger View and Song/Arranger Mode: I've made a start refactoring In code, these concepts are distinct: there is I'm currently refactoring |
Hey so are you sure switching between arranger view and song view immediately switches the mode? I don't think it does. I think the mode switch only happens when playback switches / starts. I believe it happens in the playback class "playback_handler.cpp" which has a function to "decide the current playback mode" If you're in song view and turn on playback (from off), it's turning on session / song playback mode. With session / song playback mode on, if you go into arranger view, the song playback mode is still playing. You can get it to switch to arrangement playback mode using <> + play or by turning off playback and press play again. The vice versa applies to starting playback in arranger view and going into song view. I also agree with the statement that the playback mode is still active when you go into a clip so you can tweak clips that get processed by that active playback mode. As for changing the Session and Arrangement classes to SongMode and ArrangerMode I think we can do that since looking at the headers it's pretty clear that their sole function is to manage the playback mode. An aside: there's also a class called Song which is used to represent the entire Project. To avoid further confusion with SongMode I think that should be refactored to Project as well. And then the Song Menu would become the Project Menu. Which would then require a refactoring of the save / load song UI to rename that to save / load project. |
I'm currently limiting my branch to refactoring |
Sounds good and yes agreed I want to talk about Arranger / Song View separately from the Arranger / Song modes in the user documentation but explain how they're connected. Re refactoring song to project, later is totally fine, was just raising it so it's on our radar :). |
|
@seangoodvibes I'm getting into the weeds a little bit with keeping track of how to refactor things. I would like to have a central place to make decisions and a central place to keep track of things we agreed on. I was thinking a central code cleanup thread in discussions, and a folder in the repo specifically for agreed-upon decisions (possibly in This pr is just all my personal ideas. But as I'm finding out that everything is touching everything in the code, I feel like a different strategy is needed to to tackle refactoring. Thoughts? |
When it comes to refactoring, it's best to break it up into smaller sequential pieces. In the case of refactoring session view for example, I'd have one PR to literally just rename sessionView to songView. That could be a simple find-replace PR. Then build from there. You can do one refactoring, open a PR, then start another PR but keep it draft until the first one is merged. the next PR could be to rename the Session class to SongMode. then another PR to rename the Arrangement class to ArrangerMode. etc etc I think a document in docs/dev/roadmap/ would be good. Opening a PR for that document would give us a formal way to provide feedback and approval of your plan A discussions thread would be good too I think. |
Agreed. After spending some initial organizing-time with sessionView, I quickly found out that to do any meaningful inheritance cleanup you're going to need to start at the very bottom. Before I start on anything like that I'd need to know that we're in alignment with where we want to go with this. And when working low-level, taking it in very small steps would especially be a good idea. I'll see what parts of my sessionView-work I can submit here and will set up some kinda roadmap structure pr after |
|
@seangoodvibes I guess my only concern about making small prs is that I'd not be able to do any more restructuring before the pr is merged, which can take a few days. Especially with restructuring stuff, you'd constantly be building on top the previous step you just wrote. Is there a practical way around that? |
They don't have to be tiny PR's but at the very least the commit history should make it easy to review the PR. Ultimately when it comes to restructurings, you have to be prepared for feedback that you may not expect. Also we're not a big team and approvals can take time unfortunately. That's why I suggested communicating with us on discord as it's the really the place where us dev's give each other real time feedback on whether we're on the right track. But I understand if that's not for you. It will just be a bit more difficult to get aligned on direction I think. |
I understand it's more useful to keep communication in a central place. I'll give it some thought. |
|
@seangoodvibes Ok here's where I stand: I strongly prefer developing in a way that benefits the community as a whole, but I'm experiencing it as very challenging in the current codebase. There are no docs for the code, and navigating the code itself can be very challenging and time-consuming. Last week I tried to solve an issue and I literally spent a full day analyzing code just to get a sense for what was happening, to finally submit a five-line solution in the evening. I'm also happy to contribute to a solution that would make development more accessible, but I've realized the initiative and guidelines for that solution should really come from people who know the project and the codebase, which of course isn't me. I'll leave it to the core devs to decide if and when that's something y'all want to do. I just can't see myself personally investing time in developing without a simultaneous push for more code structure and documentation. As it stands, it makes more sense for me to set up a personal branch where I can implement some of the silly features I'd really like to have on my device, and restructure stuff to make my own life easier, without breaking the community branch. If I do end up doing stuff that directly applies to the community branch as well I'm happy to still submit it. I want to make it explicitly clear I'm not criticizing anyone with this, the community firmware rocks as is and all your points have been 100% fair |
|
I mean, we would greatly welcome more code structure and documentation. It's been one of the weakest points of the project from the start: when the codebase was released, I think there was a total of eight paragraphs of explanation (which are in the wiki, now added to) about how some specific subsystems worked, and absolutely no directory structure for the core Deluge application code. None of us wrote the code, and it was largely released as it exists now, barring some slow-moving modernizations (like half of the codebase is on namespaces and I've never managed to finish it). If you want to see what has happened so far, you can go to the "official" branch and see how ours compares. As-is, there's only like four of us actively developing right now, all with very very limited time, and documenting everything when there's no guarantee it'll get used is very time consuming. That's not to say we don't want more documentation, just that we've found it more efficient on a per-feature/bug basis to ask another person who knows about that area first, or instead learn about the relevant bits of the codebase as-needed by reverse engineering and analysis like you described, or rewrite subsystems so they make more sense, while including documentation as we do (see the PIC driver). We do also have a Doxygen setup that works, but no output from our CI or hosting anywhere, which I do think is something that should be done. So we do want major refactorings and documentation, it just also has to be balanced with the few things that we have learned about the codebase, and how the hardware runs with regards to things like code size and speed. |
To echo what Katherine said, I think there is a wrong assumption here with regard to the "core devs." We don't know that much more than you do. We have built up some understanding of the codebase and we continue to build up that understanding as we work on features and fixes. None of us know the entire codebase. We know some of the areas we've worked on and have a general understanding of other areas. We've all learned on the fly, off the seat of our pants. To get the codebase to the place you describe is a long road, one which we all aspire to travel so that we can ultimately land at our destination. We are chipping away at this bit by bit as our very limited time, energy, and motivation permits. We welcome anyone that wishes to help us along the way. We understand the codebase is not fully documented and is not accessible to everyone, but keep in mind we started off in worser shoes and the codebase is much easier to understand and digest now than it was at the beginning. Does it mean it's perfect? Not by any means. But it's better. All we can do, all we can ask for is for folks to help contribute to making it better. Lastly, as Katherine mentioned, our most important consideration these days is not new features, but improving the codebase as is through refactoring, optimization, and other performance tweaks. The performance of the Deluge is directly related to the size of the codebase firmware. With release 1.2 we've pushed that firmware size to its limit and we now need to look at how we can potentially reduce it. I think refactoring the UI code, including the views and menus is probably where we will get our biggest wins in that regard. I think that's the most valuable area that anyone can contribute to at the moment. Anywho, hope this clears things up a bit. All the best, Sean |
|
Thanks for the feedback. I have checked out the official firmware and I realize the progress is substantial. I may have had a wrong image of a "core team" indeed. To repeat myself a little bit: I've not really worked on larger / communal projects before so I'm kind of finding out how everything works and what works for me. Without wanting to throw a pity party: I have a lot of stuff to deal with currently, and guarding my own boundaries and limitations is my first priority in that regard. Which for example leads to Discord not being an option at this time. I really appreciate everyone's work on the firmware. At this time I think its best for me to go off on my own and see whether I can get some larger things to work, that I have in mind - as opposed to being part of the granular updating process. If we can adapt or merge that to the community brach at some later point, all the better. |
I'm making a start documenting the repo so that it's easier for people (including myself) to work on bugfixes and new features. As I'm working through the code doing this, it makes sense to take notes of where stuff could be organized in a more logical or easy-to-understand way. This may also lead to suggestions for coding guidelines to make/keep code structured within files.
This branch may be at any state of disarray at any moment in time - just posting it here for reference