rafalcieslak
commented
8 years ago Thanks for your contribution Robert. I've examined your patch, and I have some comments to share.
Before reviewing this branch I pushed some changes to my master branch - remember to git rebase your branch onto my master, in order to import commits from master and automatically re-apply your changes after what was added to the main branch.
There are several details in your patch that will require fixing before I can merge your branch, I'll be describing them below. Keep in mind that when fixing them you can just simply add new commits to your branch, and they will be automatically considered as a part of this pull request - github will even generate an updated diff.
Since vmodsynth is versioned using git, there is really no need to mark one's contributions with a comment on every line they wrote. Git stores commit history, and can easily find the author of any source line, should we need to track/blame them. Similarly, when you remove parts of source, go ahead and remove them entirely. Leaving them as comments brings no benefit (on can always see the previous, deleted version of any file in the changes history), and severely clutters the source in the long run. Same goes for CMakeLists.txt_backup file, it is unnecessary.
You do, however, deserve an entry in copyright notices, if you feel like it. For example, let's have a look at src/modules/v212.cpp. Clearly you wrote this file from scratch, therefore I suggest to change the copyright notice in that file from:
By Robert Gyllenberg 2016 contributing to vModsynt
This file is part of vModSynth.
Copyright (C) 2012, 2013 Rafał Cieślakto:
This file is part of vModSynth.
Copyright (C) 2016 Robert GyllenbergIf you are concerned that you basically copied a module template from a file I created, feel free to add one more line:
Based on vModSynth module template by Rafał Cieślak 2012, 2013In files like src/Engine.cpp you've marked your contribution in a comment in the middle of the source file. Such section-marking style eventually leads to scattered source code. Instead, state your contribution in the copyright notice at the beginning of the file. For example, in Engine.cpp you might want to add a new line in the notice, similar to this one:
Modified 2016 Robert Gyllenbert - implemented file saving and loading procedures.Keep in mind that there it is never necessary to explicitly mark your copyrights in a source file - you own those rights anyway, so there is never any harm in under-using copyright notices. So in files where you've barely changed anything it's totally fine if you don't leave your signature anywhere.
I see that some parts of the source code are oddly formatted, while others look good. In particular, in Engine.cpp the new source code uses 1-space indentation. Can we please stick to 4-spaces uniformly throughout the entire source?
It appears that the FindXml++.cmake script you're using is indeed having some trouble detecting libxml++ correctly. However, hard-coding the paths in CMakeLists is a very bad idea, because on various systems the libraries and header files may be installed in different directories. For example, the ubuntu variant that I use stores some libxml++'s headers in /usr/lib/x86_64-linux-gnu/libxml++-2.6/include/. The main task of cmake is to dynamically detect these paths on any system, so writing them by hand kills cmake's purpose. By the way, libxml++ documentation states that the only correct way of including it's headers is to #include <libxml++/libxml++>, otherwise it suffers from some weird internal problems.
I suppose writing cmake scripts can be quite tedious if one is not experienced with them. I'll swiftly prepare a variant that detects libxml++ correctly and will send it to you as a pull-request into your branch :-)
There is a place in your code where you need to determine whether a knob is a selector or not, and for that you introduced an extra subtype member field. That's unnecessary! You can determine the type with any of these methods:
if(typeid(*knobs[i]) == typeid(Selector)){
// This is a selector!
}or
if(dynamic_cast<Selector*>(knobs[i]) != nullptr){
// This is a selector!
}The changes you've made to v100 module are invalid, I suspect. There are two reasons why that if statement is important:
- I am trying my best to have all modules similar in their usage to the modules provided by synthesizers.com. They are a good point of reference, and in their case the main oscillator module switches into a different mode when VCF input is connected, in that mode it entirely ignores knobs that tune frequency.
- Keep in mind that all
dsp()functions need to be super-efficient, as they run should close to realtime. That main if statement helps in avoiding a large number of unnecessary computations.
The toolbar save/open icons are nice, but their placement confuses me a little. I suppose the most used buttons on the toolbar are the "+" and edit mode switch. Let's move save/open buttons to between "delete" and "zoom in" - they are far less frequently used, so they don't deserve the left-most spot.
Finally, about the save files. Firstly, I find it peculiar that you use an elegant, full-featured xml library for reading xml files, but you make no use of it when writing them. I suppose writing xml files manually is indeed a simple task, so it's a good starting point, but it would make much more sense (and would be easier to maintain in the future) to use libxml++ for writing as well!
Secondly, I see you chose to gather all knobs globally, and save the state of all knobs mixed together (so the std::vector you switched too actually works as a map from knob id into knob pointer). I guess this works okay, apparently. However, I believe a more natural approach would be to store knob values per each module separately. So instead of
<module ... >
<module ... >
<module ... >
<knob ... >
<knob ... >
<knob ... >
<knob ... >the save file would have a hierarchy:
<module ...>
<knob ... >
<knob ... >
<knob ... >
</module>
<module ...>
<knob ...>
</module>Why do I feel it is a better idea? Generally, a knob number within a module is always fixed. So, for example, the 3rd knob of v100 is always the pulse width knob, regardless of the patch state - while, on the contrary, in the solution you implemented the 9th global knob may be the knob of any module, and resolving which knob it actually is depends on the order of modules. This has various potentially bug-prone side-effects, for example, one cannot change an order of
Finally, your trick to save wire parameters by storing their coordinates is... unnecessarily complicated. Wouldn't it be much much easier to just store that "there is a wire from module 5 knob 1 to module 2 knob 3" ? Like this:
<wire src="5" src_knob="1" dst="2" knob="3"/>Not only this solution is simpler to implement and easier to maintain, but it is also more forward compatible (suppose a module in a future release is 10 px wider - again, all save files become invalid), and keeps the GUI information away from the save file, which should only describe the layout of the patch.
By the way, why are module names stored in the save file? I don't see them being used for anything.
Lastly, I am quite worried about that std::setlocale function you use at some point. You never revert the locale to it's previous setting, so it ends up permanently switched to "C", which is definitely undesirable!
Whoa, I guess that's all I have to say. Sorry for such a long reply, but I wanted to go into details in order to make sure that all my points are explained well. In case you need some help fixing any of above or wish to discuss some details, I'll be happy to help you!
robertgyllenberg
Hi,
I hereby submit the suggested code for file I/O in XML-format. Cheers,
/Robert