Add finer zoom, 12.5%

[zonkmachine]

Closes issue #2

I added an extra 12% in front of the setup loop. I did this in three places for the Song Editor, Piano Roll and the Automation Editor. The Automation Editor has a second zoom model that I a. didn't grasp, b. failed to test even unmodified... the other (left) is changed though and works fine.

[tresf]

@zonkmachine why is this closed?

[zonkmachine]

why is this closed?

I couldn't stand the defeat... I'm back in the saddle though.

[zonkmachine]

The selected zoom and the real differs a bit and this becomes a bit brutal on 12%
I believe the rounding off in action is done here:
https://github.com/LMMS/lmms/blob/master/src/gui/editors/SongEditor.cpp#L598:L606
There is integer math in there with truncation involved. Maybe it's not a problem though. If we pick 10% for instance, the visual end zoom is the same but more expected with the lower value. You'd only need this once in a while if you work on longer songs. at 140 bpm and 4/4 beat this covers 35 minutes on my screen while the minimum setting today, 25%, is ~9 minutes.

[zonkmachine]

@unfa is one of the only artists here who makes tracks this long and maybe would like to have a say in this.

[zonkmachine]

Here's how the math goes:

DEFAULT_PIXELS_PER_TACT = 16

setPixelsPerTact( zfac.left( zfac.length() - 1 ).toInt() *
                    DEFAULT_PIXELS_PER_TACT / 100 );

zoom

12 * 16 / 100 = 1.92 <-- Truncated to 1!
25 * 16 / 100 = 4
50 * 16 / 100 = 8
100 * 16 / 100 = 16

[zonkmachine]

The end result of13% is truncated to 2 which is equivalent to 12.5% and on target.

[zonkmachine]

This now works if people are cool with the odd value 13%.

[Umcaruje]

This now works if people are cool with the odd value 13%.

Could we use 12,5% or would that be too much of a problem?

[zonkmachine]

Could we use 12,5% or would that be too much of a problem?

I don't think it would be a whole lot of problem. You would have to test explicitly for that string and substitute for 13 or something that works. I'll try it later...

[zonkmachine]

It looks better in the menu. The background grid in the Piano Roll is a bit too much on 12,5%. Like a haze.

[zonkmachine]

I added the assert just to mirror the PianoRoll.cpp and AutomationEditor.cpp .
As the only variable is the zooming factor I think there is no risk of reaching 0 unless you go down to ~5% zoom.

[grejppi]

English uses a decimal point, not a comma. EDIT: ninja'd

Also, would it be possible to avoid hard-coding, comparing, and parsing strings?

Something like...

// in a header file somewhere
const int NUM_ZOOM_LEVELS = 7;
const float ZOOM_LEVELS[] = {
    8.0f, 4.0f, 2.0f, 1.0f, 0.5f, 0.25f, 0.125f
};

// in the constructor
for (int i = 0; i < NUM_ZOOM_LEVELS; ++i)
{
    m_zoomingModel->addItem(QString("%1\%").arg(ZOOM_LEVELS[i] * 100));
}

// in zoomingChanged()
setPixelsPerTact(ZOOM_LEVELS[m_zoomingModel->value()]);

(Disclaimer: I have not tested the above snippets of code nor have any idea if they're correct, but the general idea should be there.)

[zonkmachine]

English uses a decimal point, not a comma. EDIT: ninja'd

I ninja'd it too... :(

[grejppi]

PS: about my comment above, I've edited the code snippets in it a couple of times for obvious logic errors (I really should be sleeping right now!), but I think I'm finished now...

[zonkmachine]

Something like...

Nein! We go .toFloat() on all of them strings, calculate like hell and then back to int...

[michaelgregorius]

I agree with @grejppi that using floats instead of QStrings would look much better in the code. I also like in his example code that the conversion to QString is only done for the combo boxes. In all other places it should not be needed.

However, I would change the zoom levels data structure to an std::vector:

const std::vector<float> m_zoomLevels = { 8.0f, 4.0f, 2.0f, 1.0f, 0.5f, 0.25f, 0.125f };

This way you would not need to adjust an extra variable like NUM_ZOOM_LEVELS in case you add more zoom levels because at all the other places in the code you would simply use m_zoomLevels.size() to get the number of zoom levels or even better use the new for loops provided by C++11:

for (float const & zoomLevel : m_zoomLevels)
{
    m_zoomingModel->addItem(QString("%1\%").arg(zoomLevel * 100));
}

Last but not least I'd like to add that I think that 16 pixels per tact is too small anyway. In my opinion the default zoom level of the song editor does not really communicate "Here is a broad canvas to realize your musical ideas!". I also find it "unnatural" that the height of one tact is bigger than it's width. The reason is that musical notation expands from the left to the right so more space should be provided in that dimension.

[zonkmachine]

You're obviously right, both of you, and it cleans up the code like nothing else. Trying it out now.

[zonkmachine]

Got an ouchie!
This was on the linux builds. Mac and Win passed. I'm on Ubuntu/Mint and didn't see any complaints.

/home/travis/build/LMMS/lmms/include/SongEditor.h:127:4: error: a brace-enclosed initializer is not allowed here before ‘{’ token

/home/travis/build/LMMS/lmms/include/SongEditor.h:127:57: sorry, unimplemented: non-static data member initializers

/home/travis/build/LMMS/lmms/include/SongEditor.h:127:57: error: in-class initialization of static data member ‘m_zoomLevels’ of non-literal type

[grejppi]

@zonkmachine @michaelgregorius code in LMMS tends to prefer QVector and friends over the STL. Nothing big, just a consistency thing.

You might want to consider making the vector static:

// Class.h

class Class {
    // ...
    static const QVector<float> s_zoomLevels;
    // ...
};

// Class.cpp

const QVector<float> Class::s_zoomLevels({ 8.0f, 4.0f, 2.0f, 1.0f, 0.5f, 0.25f, 0.125f });
// ...

[zonkmachine]

You might want to consider making the vector static:

That was pretty much how I spent my night. ;-)
I changed to a QVector but I keep getting the same messages from the linker in the end:

CMakeFiles/lmmsobjs.dir/core/Song.cpp.o:(.bss+0x8): multiple definition of `PianoRoll::m_zoomLevels'
CMakeFiles/lmmsobjs.dir/core/Mixer.cpp.o:(.bss+0x0): first defined here
CMakeFiles/lmmsobjs.dir/core/Song.cpp.o:(.bss+0x0): multiple definition of `SongEditor::m_zoomLevels'
CMakeFiles/lmmsobjs.dir/core/DataFile.cpp.o:(.bss+0x80): first defined here
...

[tresf]

I changed to a QVector but I keep getting the same messages from the linker in the end:

Well, that's quite a different message I think... In this case, AutomationEditor::m_zoomXLevels and m_zoomXLevels are declared twice in the same scope... one as a private variable, one as a signal, no?

[zonkmachine]

one as a signal,

No, it's after the signals outside of the class.

[tresf]

No, it's after the signals outside of the class.

Then we must be looking at different code. m_zoomXLevels seems odd as a signal anyway, is there a reason why you have it there?

[zonkmachine]

Then we must be looking at different code. m_zoomXLevels seems odd as a signal anyway, is there a reason why you have it there?

There was a commented out initialisationtion statement in the class and there was a wrongly indented definition of m_zoomXLevels but it was outside of the Class declaration, after the 'signals'.
That's changed anyway.

Initiations moved to .cpp files

A secret society just got booked horribly wrong...

[Umcaruje]

@zonkmachine I tested it out, but noticed a bug:

When +scrolling to zoom in/out in the song editor, it only goes down to 25%, but not on 12.5%. When I scroll over the combo box, this appeares not to be an issue.

[zonkmachine]

Thanks, so that's what the SongEditor::wheelEvent does!
Back to the dungeon for some deep research...

[zonkmachine]

I stealthily copied over the solution from PianoRoll.cpp
Thanks for the review.

[zonkmachine]

Mouse wheel zoom and combo box zoom now works in opposite directions...

[zonkmachine]

Mouse wheel zoom and combo box zoom now works in opposite directions...

When you set zooming by scrolling the combo box or by Ctrl+Scrolling the track the wheel directions aren't the same. I thought I messed that one up but it's actually there in master - Fixed

[Umcaruje]

@zonkmachine could you please rebase so we get this PR merged?

[zonkmachine]

Rebased and tested.

[Umcaruje]

Tested out, works perfectly, @grejppi gave the code an ok, code looks good to me as well.
Merging 🎉

[zonkmachine]

@Umcaruje Thanks for review, merge and keeping the spirit up...